Support document.getElementsByAttribute[NS] in chrome HTML documents

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
8 months ago
10 days ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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)
(Assignee)

Updated

8 months ago
Blocks: 1473165
(Assignee)

Updated

8 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

8 months ago
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.
(Assignee)

Comment 5

8 months ago
(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).
(Assignee)

Updated

8 months ago
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)
(Assignee)

Comment 7

8 months ago
Moved the question and ni? to https://bugzilla.mozilla.org/show_bug.cgi?id=1477765#c2.
Flags: needinfo?(jimb)

Comment 8

8 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

8 months ago
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*.

Comment 13

8 months ago
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

Comment 14

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96087548d901
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.