Bug 110612

Summary:

Enumerating an HTMLCollection needs to enumerate the named properties

Product:

WebKit

Reporter:

Boris Zbarsky <bzbarsky>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WORKSFORME

Severity:

Normal

CC:

abarth, annevk, ap, arko, arv, barraclough, cdumez, esprehn+autocc, haraken, japhet, ojan.autocc, rniwa, sam, webkit.review.bot

Priority:

Keywords:

HasReduction

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Test	none
proposed patch	none
Patch	none
patch1	none
Patch3	haraken: review+

Boris Zbarsky

Reported 2013-02-22 08:20:43 PST

Consider this testcase: <!DOCTYPE html> <form id="x"> <script> for (var x in document.forms) document.writeln(x); </script> EXPECTED RESULTS: "0 x item namedItem length" in some order ACTUAL RESULTS: "0 length item namedItem" Note the missing 'x'. Gecko and Presto get this right. Trident in IE9 enumerates the 'x' but not the '0', as far as I can tell.

Attachments
Test (241 bytes, text/html) 2013-02-22 21:37 PST, Ryosuke Niwa	no flags	Details
proposed patch (8.78 KB, patch) 2013-02-28 16:50 PST, Arko Saha	no flags	Details Formatted Diff Diff
Patch (16.60 KB, patch) 2013-03-01 17:32 PST, Arko Saha	no flags	Details Formatted Diff Diff
patch1 (16.18 KB, patch) 2013-03-01 21:28 PST, Arko Saha	no flags	Details Formatted Diff Diff
Patch3 (16.19 KB, patch) 2013-03-04 16:34 PST, Arko Saha	haraken: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2013-02-22 21:31:46 PST

Confirmed on WebKit nightly and released version of Chrome.

Ryosuke Niwa

Comment 2 2013-02-22 21:37:03 PST

Created attachment 189905 [details] Test

Arko Saha

Comment 3 2013-02-28 16:50:01 PST

Created attachment 190847 [details] proposed patch

Sam Weinig

Comment 4 2013-02-28 17:06:47 PST

Is this specified in WebIDL?

Ryosuke Niwa

Comment 5 2013-02-28 17:10:54 PST

Comment on attachment 190847 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=190847&action=review > Source/WebCore/bindings/js/JSHTMLCollectionCustom.cpp:104 > + Vector<String> propertyNameStrings; > + for (unsigned index = 0; index < collection->length(); ++index) { > + String attributeValue = toElement(collection->item(index))->getAttribute(HTMLNames::nameAttr); > + if (!attributeValue.isEmpty() && !propertyNameStrings.contains(attributeValue)) > + propertyNameStrings.append(attributeValue); > + > + attributeValue = toElement(collection->item(index))->getIdAttribute(); > + if (!attributeValue.isEmpty() && !propertyNameStrings.contains(attributeValue)) > + propertyNameStrings.append(attributeValue); > + } We shouldn't be iterating through nodes like this if we already have a cache (m_idCache and m_nameCache). If needed, we should modify the cache structure of HTMLCollection to satisfy this use case. > LayoutTests/fast/dom/htmlcollection-enumeration.html:25 > +shouldBeTrue("'0' in document.forms"); > +shouldBeTrue("'1' in document.forms"); > +shouldBeTrue("'2' in document.forms"); > +shouldBeTrue("'name1' in document.forms"); > +shouldBeTrue("'id1' in document.forms"); > +shouldBeTrue("'id2' in document.forms"); > +shouldBeTrue("'name2' in document.forms"); > +shouldBeTrue("'id3' in document.forms"); > +shouldBeTrue("'id4' in document.forms"); > +shouldBeTrue("'length' in document.forms"); > +shouldBeTrue("'item' in document.forms"); > +shouldBeTrue("'namedItem' in document.forms"); Please make sure to test other subclasses of HTMLCollection like HTMLAllCollection, HTMLPropertiesCollection, etc...

Arko Saha

Comment 6 2013-02-28 17:42:16 PST

(In reply to comment #4) > Is this specified in WebIDL? http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-75708506 namedItem() method retrieves a Node using a name. it first searches for a Node with a matching id attribute. If it doesn't find one, it then searches for a Node with a matching name attribute. We need to enumerate through the named properties for id and name attribute.

Arko Saha

Comment 7 2013-02-28 17:43:53 PST

(In reply to comment #5) > We shouldn't be iterating through nodes like this if we already have a cache (m_idCache and m_nameCache). > If needed, we should modify the cache structure of HTMLCollection to satisfy this use case. > Will do. > Please make sure to test other subclasses of HTMLCollection like HTMLAllCollection, HTMLPropertiesCollection, etc... This patch only fixes HTMLCollection's enumeration. To fix other subclasses of HTMLCollection we need to use CustomGetPropertyNames and change their bindings respectively too. One approach of this problem may be add a new idl attribute and use it to respective subclass to generate common code automatically. It will allow us to generate common code for all classes and no need to write custom binding.

Boris Zbarsky

Comment 8 2013-02-28 18:23:37 PST

> Is this specified in WebIDL? Yes. See http://dev.w3.org/2006/webapi/WebIDL/#property-enumeration

Arko Saha

Comment 9 2013-03-01 17:32:41 PST

Created attachment 191082 [details] Patch Incorporated review comments.

Ryosuke Niwa

Comment 10 2013-03-01 17:37:18 PST

Comment on attachment 191082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191082&action=review The patch looks much better. I'll wait 'til JSC/V8 binding experts can comment on this patch. > Source/WebCore/html/HTMLCollection.cpp:734 > + if (!propertyNameStrings.contains(attributeValue)) m_nameCache is a hash map, right? Why do we need to check contains? > Source/WebCore/html/HTMLCollection.cpp:739 > + if (!propertyNameStrings.contains(attributeValue)) Ditto. > Source/WebCore/html/HTMLCollection.h:70 > + void getPropertyNames(Vector<String>& propertyNameStrings); Should we make this virtual?

Ryosuke Niwa

Comment 11 2013-03-01 17:37:20 PST

Arko Saha

Comment 12 2013-03-01 17:57:56 PST

(In reply to comment #11) > > Source/WebCore/html/HTMLCollection.cpp:734 > > + if (!propertyNameStrings.contains(attributeValue)) > > m_nameCache is a hash map, right? Why do we need to check contains? > Yes you are right, this check is not required. Will remove the check. > > Source/WebCore/html/HTMLCollection.h:70 > > + void getPropertyNames(Vector<String>& propertyNameStrings); > > Should we make this virtual? Will do.

Arko Saha

Comment 13 2013-03-01 21:28:07 PST

Created attachment 191095 [details] patch1 Incorporated review comments.

Arko Saha

Comment 14 2013-03-04 11:05:05 PST

Hi haraken, can you please review the patch once. I have made changes in CodeGenerator so that it can generate namedPropertyEnumerator code automatically for HTMLCollection interface and its subclasses. Thanks.

Kentaro Hara

Comment 15 2013-03-04 15:40:50 PST

Comment on attachment 191095 [details] patch1 View in context: https://bugs.webkit.org/attachment.cgi?id=191095&action=review Almost LGTM. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2223 > + if ($interfaceName =~ /^HTML\w*Collection$/) { We don't want to hard-code interface names in an ad-hoc manner. Can't you use CodeGenerator::InheritsInterface($interface, "HTMLCollection")? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2626 > + if ($interfaceName =~ /^HTML\w*Collection$/) { Ditto. > Source/WebCore/html/HTMLCollection.cpp:734 > + for (NodeCacheMap::iterator it = m_nameCache.begin(); it != m_nameCache.end(); ++it) > + propertyNameStrings.append(it->key); > + for (NodeCacheMap::iterator it = m_idCache.begin(); it != m_idCache.end(); ++it) > + propertyNameStrings.append(it->key); Just to confirm: This order does not matter, right? i.e. The spec does not specify the order of enumeration, right?

Arko Saha

Comment 16 2013-03-04 16:34:33 PST

Created attachment 191344 [details] Patch3 Incorporated review comments.

Arko Saha

Comment 17 2013-03-04 16:36:32 PST

(In reply to comment #15) > We don't want to hard-code interface names in an ad-hoc manner. Can't you use CodeGenerator::InheritsInterface($interface, "HTMLCollection")? > Done. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2626 > > + if ($interfaceName =~ /^HTML\w*Collection$/) { > > Ditto. Done. > > Source/WebCore/html/HTMLCollection.cpp:734 > > + for (NodeCacheMap::iterator it = m_nameCache.begin(); it != m_nameCache.end(); ++it) > > + propertyNameStrings.append(it->key); > > + for (NodeCacheMap::iterator it = m_idCache.begin(); it != m_idCache.end(); ++it) > > + propertyNameStrings.append(it->key); > > Just to confirm: This order does not matter, right? i.e. The spec does not specify the order of enumeration, right? Yes you are right, the spec doesn't specify the order of enumeration.

Kentaro Hara

Comment 18 2013-03-04 16:43:34 PST

Comment on attachment 191344 [details] Patch3 Looks great!

Boris Zbarsky

Comment 19 2013-03-04 18:18:51 PST

The spec does in fact specify the order of enumeration. The relevant spec bits are http://dom.spec.whatwg.org/#interface-htmlcollection where it defines the "supported property names" (note that the definition is of an ordered set) and http://dev.w3.org/2006/webapi/WebIDL/#property-enumeration which defines that "supported property names" are enumerated in the order given in their definition.

Boris Zbarsky

Comment 20 2013-03-04 18:20:18 PST

And also, you probably want to watch out for cases when the collection contains an element with some id="x" and another element with a name="x". Not sure what the attached patch ends up doing for those, but it should of course only enumerate "x" once.

Anne van Kesteren

Comment 21 2014-02-12 07:55:49 PST

Note, I just changed http://dom.spec.whatwg.org/ around here. Named properties on HTMLCollection enumerate no longer. getOwnPropertyNames still gets them of course.

Ryosuke Niwa

Comment 22 2016-04-11 01:08:44 PDT

This is still broken on Safari Technology Preview :(

Ryosuke Niwa

Comment 23 2016-04-11 01:10:46 PDT

Actually, this works as expected since Object.getOwnPropertyNames(document.forms) returns [0, x]. In fact, our behavior matches that of Gecko.

Note You need to log in before you can comment on or make changes to this bug.