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.
Confirmed on WebKit nightly and released version of Chrome.
Created attachment 189905 [details] Test
Created attachment 190847 [details] proposed patch
Is this specified in WebIDL?
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...
(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.
(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.
> Is this specified in WebIDL? Yes. See http://dev.w3.org/2006/webapi/WebIDL/#property-enumeration
Created attachment 191082 [details] Patch Incorporated review comments.
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?
(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.
Created attachment 191095 [details] patch1 Incorporated review comments.
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.
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?
Created attachment 191344 [details] Patch3 Incorporated review comments.
(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.
Comment on attachment 191344 [details] Patch3 Looks great!
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.
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.
Note, I just changed http://dom.spec.whatwg.org/ around here. Named properties on HTMLCollection enumerate no longer. getOwnPropertyNames still gets them of course.
This is still broken on Safari Technology Preview :(
Actually, this works as expected since Object.getOwnPropertyNames(document.forms) returns [0, x]. In fact, our behavior matches that of Gecko.