Bug 110612 - Enumerating an HTMLCollection needs to enumerate the named properties
Summary: Enumerating an HTMLCollection needs to enumerate the named properties
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2013-02-22 08:20 PST by Boris Zbarsky
Modified: 2016-04-11 01:10 PDT (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 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.
Comment 1 Ryosuke Niwa 2013-02-22 21:31:46 PST
Confirmed on WebKit nightly and released version of Chrome.
Comment 2 Ryosuke Niwa 2013-02-22 21:37:03 PST
Created attachment 189905 [details]
Test
Comment 3 Arko Saha 2013-02-28 16:50:01 PST
Created attachment 190847 [details]
proposed patch
Comment 4 Sam Weinig 2013-02-28 17:06:47 PST
Is this specified in WebIDL?
Comment 5 Ryosuke Niwa 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...
Comment 6 Arko Saha 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.
Comment 7 Arko Saha 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.
Comment 8 Boris Zbarsky 2013-02-28 18:23:37 PST
> Is this specified in WebIDL?

Yes.  See http://dev.w3.org/2006/webapi/WebIDL/#property-enumeration
Comment 9 Arko Saha 2013-03-01 17:32:41 PST
Created attachment 191082 [details]
Patch

Incorporated review comments.
Comment 10 Ryosuke Niwa 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?
Comment 11 Ryosuke Niwa 2013-03-01 17:37:20 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?
Comment 12 Arko Saha 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.
Comment 13 Arko Saha 2013-03-01 21:28:07 PST
Created attachment 191095 [details]
patch1

Incorporated review comments.
Comment 14 Arko Saha 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.
Comment 15 Kentaro Hara 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?
Comment 16 Arko Saha 2013-03-04 16:34:33 PST
Created attachment 191344 [details]
Patch3

Incorporated review comments.
Comment 17 Arko Saha 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.
Comment 18 Kentaro Hara 2013-03-04 16:43:34 PST
Comment on attachment 191344 [details]
Patch3

Looks great!
Comment 19 Boris Zbarsky 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.
Comment 20 Boris Zbarsky 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.
Comment 21 Anne van Kesteren 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.
Comment 22 Ryosuke Niwa 2016-04-11 01:08:44 PDT
This is still broken on Safari Technology Preview :(
Comment 23 Ryosuke Niwa 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.