239976 - nsXULElement::GetElementsByAttributeNS (also for nsXULDocument)

Status: RESOLVED FIXED

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

Description

[Alex Vincent [:WeirdAl]]

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

Comment 1

[Alex Vincent [:WeirdAl]]

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

Err, I mean attributes, not elements.

Comment 2

[Boris Zbarsky [:bzbarsky]]

How about we fix GetElementsByAttribute to be live like nodelists are supposed
to be, first?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm... mind posting a testcase or two for getElementsByAttribute and
getElementsByAttributeNS?  I have an idea....

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: testcase with DOM manipulations available

Comment 5

[Alex Vincent [:WeirdAl]]

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.

Comment 6

[marja]

(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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I vote for correcting nodelist too.

What is there to correct in nodelist?

Comment 8

[marja]

> ------- Additional Comments From [e-mail]  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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[marja]

> 
> 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

[Thomas Blatter]

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

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=319421

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

[Thomas Blatter]

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).

Comment 14

[Boris Zbarsky [:bzbarsky]]

So at this point, this should be doable by just combining the GetElementsByTagNameNS code and the GetElementsByAttribute code; MatchAttribute should Just Work...

Comment 15

[Joerg Bornemann]

Attachment: patch #1

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

Comment 16

[Joerg Bornemann]

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;

Comment 17

[Boris Zbarsky [: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....

Comment 18

[Joerg Bornemann]

Attachment: patch #2

Changes:
indentation corrected,
interface UIDs changed,
unnecessary null-checks removed,
handling of namespace "*" in nsXULDocument::MatchAttribute.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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!

Comment 20

[Joerg Bornemann]

Attachment: patch #3

declaration of |list| moved in nsGenericElement::GetElementsByTagNameNS and nsDocument::GetElementsByTagNameNS

Comment 21

[Boris Zbarsky [:bzbarsky]]

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).

Comment 22

[Joerg Bornemann]

(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.

Comment 23

[Joerg Bornemann]

Attachment: 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 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Combined patch with comment grammar nit fixed.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the patch!