Closed Bug 239976 Opened 20 years ago Closed 18 years ago

nsXULElement::GetElementsByAttributeNS (also for nsXULDocument)

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: WeirdAl, Assigned: joerg.bornemann)

References

Details

(Keywords: helpwanted, testcase)

Attachments

(2 files, 4 obsolete files)

XUL documents and elements have a getElementsByAttribute() method.  It'd really
be nice to support this for elements with non-null namespaces as well.
(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....
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.
Keywords: testcase
Depends on: 240186
(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?
> ------- 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
> 
> 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
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.
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...
Attached patch patch #1 (obsolete) — Splinter Review
Implementation of GetElementsByAttributeNS in nsXULDocument and nsXULElement. See comment #14.
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;
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-
Attached patch patch #2 (obsolete) — Splinter Review
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!
Attached patch patch #3 (obsolete) — Splinter Review
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+
(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.
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
Attachment #237314 - Attachment is obsolete: true
Attachment #238191 - Attachment is obsolete: true
Checked in.  Thanks for the patch!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.