document.getElementsByTagName should return an HTMLCollection

• Component: DOM
• Status: RESOLVED FIXED

[Boris Zbarsky]

See http://dom.spec.whatwg.org/#document

Note that the list WebKit returns isn't quite what a NodeList should be anyway; it's got various HTMLCollection-like features (e.g. named access to nodes).

[Alexey Proskuryakov]

DOM 3 Core says it should be a NodeList, not an HTMLCollection. Perhaps the new spec should be fixed to not introduce arbitrary incompatibilities?

[Anne van Kesteren]

ap, that would not describe WebKit's implementation.

[Boris Zbarsky]

WebKit doesn't make it what DOM3 calls a NodeList, as I already said.  The spec was changed to HTMLCollection based in part on what WebKit contributors said WebKit was willing to implement and what it was implementing right now.

Note that both Trident and Gecko return an HTMLCollection here.

[Alexey Proskuryakov]

> WebKit doesn't make it what DOM3 calls a NodeList, as I already said.

Yes, however the object returned by getElementsByTagName is a NodeList, which is observable.

[Boris Zbarsky]

Yes, but given that both Gecko and Trident return HTMLCollections there there are not very likely to be web compat issues with changing the toString and prototype.  Many fewer issues than all the other options for aligning the different implementations, for sure.

[Alexey Proskuryakov]

Was Gecko ever in compliance with DOM 3 Core in this regard? If so, when did it change?

[Boris Zbarsky]

I don't believe it was, no.  We used to use the same object for nearly all nodelists and htmlcollections (the one exception, I think, was .childNodes), and it always claimed to be an HTMLCollection.

[Chris Dumez]

Created attachment 259639 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 259639 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259641 [details]
WIP patch

[WebKit Commit Bot]

Attachment 259641 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259646 [details]
WIP patch

[WebKit Commit Bot]

Attachment 259646 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 259650 [details]
WIP patch

[WebKit Commit Bot]

Attachment 259650 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 259650 [details]
WIP patch

Attachment 259650 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/86974

New failing tests:
platform/mac/fast/dom/wrapper-classes-objc.html

[Build Bot]

Created attachment 259657 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 259670 [details]
Patch

[WebKit Commit Bot]

Attachment 259670 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 259670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259670&action=review

> Source/WebCore/bindings/objc/DOMHTML.mm:149
> +    return kit(WTF::getPtr(createFragmentFromMarkup(*core(self), markupString, [baseURL absoluteString])));

If writing by hand, I think we can just write .get(). The WTF::getPtr function exists so we can conveniently write code that handles a type without knowing whether it’s RefPtr<X> or X* in template code or automatically generated code.

> Source/WebCore/dom/NodeRareData.h:181
> +    void removeCachedCollectionWithQualifiedName(HTMLCollection* collection, const AtomicString& namespaceURI, const AtomicString& localName)

This function should take a reference rather than a pointer.

> Source/WebCore/dom/TagCollection.cpp:50
> +    , m_loweredLocalName(localName.lower())

Would be better to use an ASCII lowering function here rather than a full Unicode lowering function.

> Source/WebCore/editing/markup.cpp:680
> +    Ref<HTMLBodyElement> fakeBody = HTMLBodyElement::create(document);
> +    Ref<DocumentFragment> fragment = DocumentFragment::create(document);

Use auto here?

> Source/WebCore/editing/markup.cpp:689
> +    for (auto& attachment : descendantsOfType<HTMLAttachmentElement>(fragment)) {
> +        attachment.setFile(File::create(attachment.fastGetAttribute(webkitattachmentpathAttr)).ptr());
> +        attachment.removeAttribute(webkitattachmentpathAttr);
>      }

Any chance that the document will be mutated while iterating, perhaps if there are mutation event handlers involved? Is the old code’s behavior identical to the new code’s behavior in a case like that?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:447
> +        for (auto& plugInImageElement : descendantsOfType<HTMLPlugInImageElement>(*frame->document())) {

Why not just call this local “element”.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:449
> +            const URL& loadedURL = plugInImageElement.loadedUrl();
> +            String otherMimeType = plugInImageElement.loadedMimeType();

Do we need these local variables? I think the code will read better without them.

> Source/WebKit/mac/WebView/WebFrame.mm:871
> -    return kit(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent).get());
> +    return kit(WTF::getPtr(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent)));

Why this change? Is this a Ref now? Maybe use ptr() instead of WTF::getPtr().

[Chris Dumez]

Comment on attachment 259670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259670&action=review

>> Source/WebCore/bindings/objc/DOMHTML.mm:149
>> +    return kit(WTF::getPtr(createFragmentFromMarkup(*core(self), markupString, [baseURL absoluteString])));
> 
> If writing by hand, I think we can just write .get(). The WTF::getPtr function exists so we can conveniently write code that handles a type without knowing whether it’s RefPtr<X> or X* in template code or automatically generated code.

Ok, will do.

>> Source/WebCore/dom/NodeRareData.h:181
>> +    void removeCachedCollectionWithQualifiedName(HTMLCollection* collection, const AtomicString& namespaceURI, const AtomicString& localName)
> 
> This function should take a reference rather than a pointer.

Will do.

>> Source/WebCore/dom/TagCollection.cpp:50
>> +    , m_loweredLocalName(localName.lower())
> 
> Would be better to use an ASCII lowering function here rather than a full Unicode lowering function.

Ok, will change to localName.convertToASCIILowercase()

>> Source/WebCore/editing/markup.cpp:680
>> +    Ref<DocumentFragment> fragment = DocumentFragment::create(document);
> 
> Use auto here?

Ok, will do.

>> Source/WebCore/editing/markup.cpp:689
>>      }
> 
> Any chance that the document will be mutated while iterating, perhaps if there are mutation event handlers involved? Is the old code’s behavior identical to the new code’s behavior in a case like that?

I have just checked. I thought we were safe because WebKit does not implement attribute-level DOM mutation events. However, by looking at the code, I see that the DOMSubtreeModified can be fired synchronously :(
Therefore, using descendantsOfType() is not safe.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:447
>> +        for (auto& plugInImageElement : descendantsOfType<HTMLPlugInImageElement>(*frame->document())) {
> 
> Why not just call this local “element”.

Ok.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:449
>> +            String otherMimeType = plugInImageElement.loadedMimeType();
> 
> Do we need these local variables? I think the code will read better without them.

Will drop them.

>> Source/WebKit/mac/WebView/WebFrame.mm:871
>> +    return kit(WTF::getPtr(createFragmentFromMarkup(*document, markupString, baseURLString, DisallowScriptingContent)));
> 
> Why this change? Is this a Ref now? Maybe use ptr() instead of WTF::getPtr().

Yes, it returns a Ref<> now, I'll use .ptr().

[Chris Dumez]

Created attachment 259683 [details]
Patch

[WebKit Commit Bot]

Attachment 259683 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/NodeRareData.h:51:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/NodeRareData.h:52:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 259683 [details]
Patch

Attachment 259683 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/87861

New failing tests:
http/tests/security/storage-blocking-strengthened-plugin.html
http/tests/security/storage-blocking-loosened-plugin.html

[Build Bot]

Created attachment 259689 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[WebKit Commit Bot]

Comment on attachment 259683 [details]
Patch

Clearing flags on attachment: 259683

Committed r188809: <http://trac.webkit.org/changeset/188809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

GTK folks, I don't know how to fix the build and the DOM bindings after this. At least webkit_dom_document_get_elements_by_tag_name, webkit_dom_document_create_node_iterator, and webkit_dom_document_create_tree_walker (from our stable API) have disappeared. Probably more; those are just the functions we use in DOMNodeTest.cpp and DOMNodeFilterTest.cpp.

[Carlos Garcia Campos]

(In reply to comment #28)
> GTK folks, I don't know how to fix the build and the DOM bindings after
> this. At least webkit_dom_document_get_elements_by_tag_name,
> webkit_dom_document_create_node_iterator, and
> webkit_dom_document_create_tree_walker (from our stable API) have
> disappeared. Probably more; those are just the functions we use in
> DOMNodeTest.cpp and DOMNodeFilterTest.cpp.

I fixed this in r188854, using getElementsByTagNameForObjC and getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings or AsHTMLCollection or something like that.

[Chris Dumez]

(In reply to comment #29)
> (In reply to comment #28)
> > GTK folks, I don't know how to fix the build and the DOM bindings after
> > this. At least webkit_dom_document_get_elements_by_tag_name,
> > webkit_dom_document_create_node_iterator, and
> > webkit_dom_document_create_tree_walker (from our stable API) have
> > disappeared. Probably more; those are just the functions we use in
> > DOMNodeTest.cpp and DOMNodeFilterTest.cpp.
> 
> I fixed this in r188854, using getElementsByTagNameForObjC and
> getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings
> or AsHTMLCollection or something like that.

Thanks for the fix.

'ForBindings' wouldn't be correct as JS bindings don't use it (only ObjC bindings, and not I assume the GTK ones). AsHTMLCollection wouldn't either as it returns a NodeList.
Maybe rename to getElementsByTagNameAsNodeList() would be a better name though, now that it is used by more than the ObjC bindings. Feel free to make this change.

[Antti Koivisto]

Maybe something like getElementsByTagNameAsNodeListForLegacyBindings

[Carlos Garcia Campos]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > GTK folks, I don't know how to fix the build and the DOM bindings after
> > > this. At least webkit_dom_document_get_elements_by_tag_name,
> > > webkit_dom_document_create_node_iterator, and
> > > webkit_dom_document_create_tree_walker (from our stable API) have
> > > disappeared. Probably more; those are just the functions we use in
> > > DOMNodeTest.cpp and DOMNodeFilterTest.cpp.
> > 
> > I fixed this in r188854, using getElementsByTagNameForObjC and
> > getElementsByTagNameNSForObjC. Maybe these should be renamed as ForBindings
> > or AsHTMLCollection or something like that.
> 
> Thanks for the fix.
> 
> 'ForBindings' wouldn't be correct as JS bindings don't use it (only ObjC
> bindings, and not I assume the GTK ones). AsHTMLCollection wouldn't either
> as it returns a NodeList.
> Maybe rename to getElementsByTagNameAsNodeList() would be a better name
> though, now that it is used by more than the ObjC bindings. Feel free to
> make this change.

Oops, sure, I actually meant AsNodeList().

[Chris Dumez]

*** Bug 78909 has been marked as a duplicate of this bug. ***