Closed Bug 1475342 Opened 3 years ago Closed 3 years ago

Support document.getElementsByAttribute[NS] in chrome HTML documents

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Bug 1475342 - Move document.getElementsByAttribute[NS] to ParentNode so it'll work for HTML document and elements;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
There's not a ton of code that relies on this in the frontend and I suspect we could ultimately migrate it to querySelector. But the path of least resistance is probably either: Implement it on Document or put it on ParentNode and implement on nsINode.

Note that we don't necessarily need this on HTMLElements, this is needed just for porting existing code into an HTML document.
I don't know how hard this would be to do but I bet Boris does.
Flags: needinfo?(bzbarsky)
Priority: -- → P3
It's easy to do.
Flags: needinfo?(bzbarsky)
Blocks: 1473165
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
This seems to work, but triggers a devtools bug by moving the `documentReadyForIdle` getter into the previewed properties on XULDocument prototype (since it removes the two methods in front of it alphabetically): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fadcd470c9ecd167d58d91c691b14ec96c988cfa&selectedJob=189365223. I'll work on a fix for that next.
(In reply to Brian Grinstead [:bgrins] from comment #4)
> This seems to work, but triggers a devtools bug by moving the
> `documentReadyForIdle` getter into the previewed properties on XULDocument
> prototype (since it removes the two methods in front of it alphabetically):
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fadcd470c9ecd167d58d91c691b14ec96c988cfa&selectedJob=1
> 89365223. I'll work on a fix for that next.

Indeed, if I evaluate `document` in the Browser Console and then expand `<prototype>: XULDocumentPrototype` I see the same error. I'm not sure what the fix is. I guess we could try/catch getter evaluation, although it's not clear to me if we should be evaluating getters on the prototype object in the first place (and if so, if they should be evaluated with the actual object or the prototype object).
Depends on: 1477765
So... The code in _findSafeGetterValues expects this bit:

        let result = getter.call(this.obj);

to return a completion record (so a thing that can have "throw" as a property), not an actual value, and then skips over completion records that threw.  But the error in that try push seems to suggest we're actually getting an exception here somehow????
Flags: needinfo?(jimb)
Moved the question and ni? to https://bugzilla.mozilla.org/show_bug.cgi?id=1477765#c2.
Flags: needinfo?(jimb)
Comment on attachment 8993979 [details]
Bug 1475342 - Move document.getElementsByAttribute[NS] to ParentNode so it'll work for HTML document and elements;

https://reviewboard.mozilla.org/r/258582/#review265946

::: dom/base/nsINode.h:1857
(Diff revision 1)
>    // ParentNode methods
>    mozilla::dom::Element* GetFirstElementChild() const;
>    mozilla::dom::Element* GetLastElementChild() const;
>  
> +  static bool
> +  MatchAttribute(mozilla::dom::Element* aContent,

This doesn't need to be a class static, much less a public one.  It used to be because XULElement and XULDocument both used it, but we're not doing that here.  Just make it a static in the .cpp file.

::: dom/base/nsINode.cpp:1666
(Diff revision 1)
>  
> +already_AddRefed<nsINodeList>
> +nsINode::GetElementsByAttribute(const nsAString& aAttribute,
> +                                const nsAString& aValue)
> +{
> +    RefPtr<nsAtom> attrAtom(NS_Atomize(aAttribute));

This file is 2-space indented.  Please fix the indent on all the code being added.

::: dom/base/nsINode.cpp:1711
(Diff revision 1)
> +    return list.forget();
> +}
> +
> +/* static */
> +bool
> +nsINode::MatchAttribute(mozilla::dom::Element* aElement,

No need for the "mozilla::dom::" prefix in the .cpp file.

::: dom/webidl/ParentNode.webidl:22
(Diff revision 1)
>    readonly attribute Element? lastElementChild;
>    [Pure]
>    readonly attribute unsigned long childElementCount;
>  
> +  [Func="IsChromeOrXBL"]
> +  NodeList getElementsByAttribute(DOMString name,

This should probably claim to return HTMLCollection, since that's what it ends up returning...

::: dom/webidl/ParentNode.webidl:25
(Diff revision 1)
>  
> +  [Func="IsChromeOrXBL"]
> +  NodeList getElementsByAttribute(DOMString name,
> +                                  [TreatNullAs=EmptyString] DOMString value);
> +  [Throws, Func="IsChromeOrXBL"]
> +  NodeList getElementsByAttributeNS(DOMString? namespaceURI, DOMString name,

HTMLCollection.
Attachment #8993979 - Flags: review?(bzbarsky) → review+
Just want to confirm this is correct - after I switched to HTMLCollection in webidl I also changed nsINodeList to nsIHTMLCollection in the C++: https://reviewboard.mozilla.org/r/258580/diff/1-2/.

I see some cases (like DocumentOrShadowRoot::GetElementsByTagName) where HTMLCollection from webidl is returned as nsContentList in C++, but others like Element::GetElementsByTagName where it's returned as nsIHTMLCollection.
> I also changed nsINodeList to nsIHTMLCollection in the C++:

That seems fine, yes.  The important thing is that the return type be a subclass of nsIHTMLCollection, since the  bindings will try to assign it to an nsIHTMLCollection*.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96087548d901
Move document.getElementsByAttribute[NS] to ParentNode so it'll work for HTML document and elements;r=bz
https://hg.mozilla.org/mozilla-central/rev/96087548d901
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.