nsXULElement::GetElementsByAttributeNS (also for nsXULDocument)

RESOLVED FIXED

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: WeirdAl, Assigned: Joerg Bornemann)

Tracking

({helpwanted, testcase})

Trunk
helpwanted, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
XUL documents and elements have a getElementsByAttribute() method.  It'd really
be nice to support this for elements with non-null namespaces as well.
(Reporter)

Comment 1

13 years ago
(In reply to comment #0)
>support this for elements with non-null namespaces as well.

Err, I mean attributes, not elements.

How about we fix GetElementsByAttribute to be live like nodelists are supposed
to be, first?
Hmm... mind posting a testcase or two for getElementsByAttribute and
getElementsByAttributeNS?  I have an idea....
(Reporter)

Comment 4

13 years ago
Created attachment 145833 [details]
testcase with DOM manipulations available
(Reporter)

Comment 5

13 years ago
Comment on attachment 145833 [details]
testcase with DOM manipulations available

The last green row in any set has an attribute foo2:foo instead of foo:foo. 
This is to test the namespace-aware method's requirement to not look at the
complete nodeName.  However, I didn't style it that way, so clicking on the
last row of a set will cause an adjustment in the NS-aware function's return,
but not in the NS-blind one.
(Reporter)

Updated

13 years ago
Keywords: testcase
Depends on: 240186

Comment 6

13 years ago
(In reply to comment #2)
> How about we fix GetElementsByAttribute to be live like nodelists are 
supposed
> to be, first?

I vote for correcting nodelist too. I had a lot of problems until I understood 
nodelist with getElementsByAttribute if different from nodelist from 
getElementsByTagName and need totally different code.

I also suggest adding into documentation how to write nodelist code to be 
backwards compatible between different versions (start from end and not from 
the beginning.
> I vote for correcting nodelist too.

What is there to correct in nodelist?

Comment 8

13 years ago
> ------- Additional Comments From bzbarsky@mit.edu  2005-02-27 18:23 PST -------
>  
>
>> I vote for correcting nodelist too.
>>   
>
>
> What is there to correct in nodelist?
>  
>
I thought that was your comment also. Maybe I used wrong wording...

Anyway, the nodelist returned by getElementsByAttribute() is not live so
removing node item(0) several times always removes the same node. When you do
the same with getElementsByTagName() the returned nodelist is live and removing
node item(0) removes the nodes one after another and the length of the nodelist
changes automatically as well.

If you remove starting from the last node it does not make a big difference, but
who would know that you need to do that without documentation?

Marja
The point is, getElementsByAttribute doesn't actually return a nodelist (since
the object it returns does not properly implement the nodelist api).  That's
what needs to be corrected.

As for documentation, getElementsByTagName() is certainly documented, in the DOM
spec.  getElementsByAttribute does need documentation.
Keywords: helpwanted

Comment 10

13 years ago
> 
> As for documentation, getElementsByTagName() is certainly documented, in the DOM
> spec.  getElementsByAttribute does need documentation.

The xulplanet document says it returns a nodelist and W3C DOM says nodelist is
live and even if not the same returned nodelist does a different thing when
returned from getElementsByTagName and getElementsByAttribute
http://www.xulplanet.com/references/xpcomref/ifaces/nsIDOMXULDocument.html#method_getElementsByAttribute

But maybe there is better documentation somewhere in mozilla.org and I should
not believe xulplanet?

Marja

Comment 11

12 years ago
As by Firefox 1.5 in the testcase not even .getElementsByAttribute works any more.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=319421
It works fine when it's supposed to work -- for attributes in the per-element namespace partition.  This bug is about having a way to work with other attributes.  Note comment 0 of this bug.

Comment 13

12 years ago
In pre-1.5 it worked (i used it occasionally, that is why i noticed the change).

I understood .doWhatever (opposed to .doWhateverNS) as DOM L 1 non-namespace-aware methods, where "pref:localname" is a legal and normal xml name which can be found as such in the qualified form (DOM L 2 Core: "DOM Level 1 methods solely identify attribute nodes by their nodeName.", where nodeName is the qualified name).
So at this point, this should be doable by just combining the GetElementsByTagNameNS code and the GetElementsByAttribute code; MatchAttribute should Just Work...
(Assignee)

Comment 15

11 years ago
Created attachment 236130 [details] [diff] [review]
patch #1

Implementation of GetElementsByAttributeNS in nsXULDocument and nsXULElement. See comment #14.
(Assignee)

Comment 16

11 years ago
The testcase is only for nsXULElement::GetElementsByAttributeNS. If you want to test nsXULDocument::GetElementsByAttributeNS then replace in the testcase in window.addEventListener:

master = document.getElementById("master");

with

master = document;
(Assignee)

Updated

11 years ago
Attachment #236130 - Flags: review?(bzbarsky)
Comment on attachment 236130 [details] [diff] [review]
patch #1

>Index: content/xul/content/src/nsXULElement.cpp
>+    if (!aNamespaceURI.EqualsLiteral("*")) {
>+	    nsresult rv =
>+		    nsContentUtils::NameSpaceManager()->RegisterNameSpace(aNamespaceURI,
>+															                                nameSpaceId);

Some weird indentation here... tabs or something?

>Index: content/xul/document/src/nsXULDocument.cpp

Same in this file.

>Index: dom/public/idl/xul/nsIDOMXULDocument.idl
>Index: dom/public/idl/xul/nsIDOMXULElement.idl

Change both IIDs?

Btw, the nsDocument and nsGenericElement versions of GetElementsByTagNameNS have some left-over null-checks of |list| that are not needed anymore. Want to remove them while you're looking at this code?

Most importantly, I don't think this code handles "*" for the namespace correctly... Back when I wrote comment 14 I didn't realize we needed kNameSpaceID_Wildcard.  That value needs special handling in nsXULDocument::MatchAttribute, I think....
Attachment #236130 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 18

11 years ago
Created attachment 237172 [details] [diff] [review]
patch #2

Changes:
indentation corrected,
interface UIDs changed,
unnecessary null-checks removed,
handling of namespace "*" in nsXULDocument::MatchAttribute.
Attachment #236130 - Attachment is obsolete: true
Comment on attachment 237172 [details] [diff] [review]
patch #2

>Index: content/base/src/nsDocument.cpp
>+  list = NS_GetContentList(this, nameAtom, nameSpaceId).get();

Move the declaration of |list| here too?  Same for nsGenericElement.

r+sr=bzbarsky with that; this looks great!
(Assignee)

Comment 20

11 years ago
Created attachment 237314 [details] [diff] [review]
patch #3

declaration of |list| moved in nsGenericElement::GetElementsByTagNameNS and nsDocument::GetElementsByTagNameNS
Attachment #237172 - Attachment is obsolete: true
Comment on attachment 237314 [details] [diff] [review]
patch #3

r+sr=bzbarsky.  I'll get this checked in Sunday morning.

By the way, did you run the existing getElementsByAttribute tests?  In a build with --enable-tests, go into content/test and run "make check".

Speaking of which, we should add tests for getElementsByAttributeNS to content/test/unit/test_nodelist.js.  I can do it if you'd rather not, I guess.  You could use http://lxr.mozilla.org/seamonkey/source/content/test/unit/test_nodelist.js#169 as a model (and change the XUL file as needed if it's not set up to test this new code well).
Attachment #237314 - Flags: superreview+
Attachment #237314 - Flags: review+
(Assignee)

Comment 22

11 years ago
(In reply to comment #21)

> By the way, did you run the existing getElementsByAttribute tests?  In a build
> with --enable-tests, go into content/test and run "make check".

No, sorry I just used test cases like the one in the 1st attachment.
I'll have a try adding a getElementsByAttributeNS test.
(Assignee)

Comment 23

11 years ago
Created attachment 238191 [details] [diff] [review]
unit test for XULElement::GetElementsByAttributeNS(.)

OK here is a unit test for XULElement::GetElementsByAttributeNS.
I think it covers all interesting situations (or did I forget something?). And it passes with patch #3.
Comment on attachment 238191 [details] [diff] [review]
unit test for XULElement::GetElementsByAttributeNS(.)

This is great!

>Index: test_nodelist.js
>+  // Test null namespace. This should be the same like getElementsByAttribute.

"same as".  I'll fix it before checking in.
Assignee: general → jobor
Created attachment 238362 [details] [diff] [review]
Combined patch with comment grammar nit fixed.
Attachment #237314 - Attachment is obsolete: true
Attachment #238191 - Attachment is obsolete: true
Checked in.  Thanks for the patch!
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.