Last Comment Bug 239976 - nsXULElement::GetElementsByAttributeNS (also for nsXULDocument)
: nsXULElement::GetElementsByAttributeNS (also for nsXULDocument)
Status: RESOLVED FIXED
: helpwanted, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: ---
Assigned To: Joerg Bornemann
: Hixie (not reading bugmail)
Mentors:
Depends on: 240186
Blocks:
  Show dependency treegraph
 
Reported: 2004-04-07 21:47 PDT by Alex Vincent [:WeirdAl]
Modified: 2013-04-04 13:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase with DOM manipulations available (3.84 KB, application/vnd.mozilla.xul+xml)
2004-04-10 15:44 PDT, Alex Vincent [:WeirdAl]
no flags Details
patch #1 (6.47 KB, patch)
2006-08-30 13:43 PDT, Joerg Bornemann
bzbarsky: review-
Details | Diff | Review
patch #2 (11.67 KB, patch)
2006-09-07 12:55 PDT, Joerg Bornemann
no flags Details | Diff | Review
patch #3 (12.29 KB, patch)
2006-09-08 03:30 PDT, Joerg Bornemann
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
unit test for XULElement::GetElementsByAttributeNS(.) (5.61 KB, patch)
2006-09-13 03:56 PDT, Joerg Bornemann
no flags Details | Diff | Review
Combined patch with comment grammar nit fixed. (17.95 KB, patch)
2006-09-13 21:22 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review

Description Alex Vincent [:WeirdAl] 2004-04-07 21:47:05 PDT
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] 2004-04-07 21:48:09 PDT
(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 [:bz] 2004-04-09 22:36:15 PDT
How about we fix GetElementsByAttribute to be live like nodelists are supposed
to be, first?
Comment 3 Boris Zbarsky [:bz] 2004-04-10 14:27:28 PDT
Hmm... mind posting a testcase or two for getElementsByAttribute and
getElementsByAttributeNS?  I have an idea....
Comment 4 Alex Vincent [:WeirdAl] 2004-04-10 15:44:03 PDT
Created attachment 145833 [details]
testcase with DOM manipulations available
Comment 5 Alex Vincent [:WeirdAl] 2004-04-10 15:47:20 PDT
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 2005-02-27 16:36:12 PST
(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 [:bz] 2005-02-27 18:23:38 PST
> I vote for correcting nodelist too.

What is there to correct in nodelist?
Comment 8 marja 2005-02-27 21:53:52 PST
> ------- 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
Comment 9 Boris Zbarsky [:bz] 2005-02-27 22:14:25 PST
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 2005-02-28 07:47:39 PST
> 
> 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 2005-12-12 07:59:46 PST
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 [:bz] 2005-12-12 08:19:47 PST
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 2005-12-12 08:54:41 PST
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 [:bz] 2006-06-23 15:18:47 PDT
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 2006-08-30 13:43:58 PDT
Created attachment 236130 [details] [diff] [review]
patch #1

Implementation of GetElementsByAttributeNS in nsXULDocument and nsXULElement. See comment #14.
Comment 16 Joerg Bornemann 2006-08-30 13:47:39 PDT
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 [:bz] 2006-09-07 00:17:19 PDT
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 2006-09-07 12:55:35 PDT
Created attachment 237172 [details] [diff] [review]
patch #2

Changes:
indentation corrected,
interface UIDs changed,
unnecessary null-checks removed,
handling of namespace "*" in nsXULDocument::MatchAttribute.
Comment 19 Boris Zbarsky [:bz] 2006-09-07 13:08:57 PDT
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 2006-09-08 03:30:17 PDT
Created attachment 237314 [details] [diff] [review]
patch #3

declaration of |list| moved in nsGenericElement::GetElementsByTagNameNS and nsDocument::GetElementsByTagNameNS
Comment 21 Boris Zbarsky [:bz] 2006-09-08 23:26:19 PDT
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 2006-09-11 01:13:08 PDT
(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 2006-09-13 03:56:23 PDT
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 24 Boris Zbarsky [:bz] 2006-09-13 21:17:02 PDT
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 [:bz] 2006-09-13 21:22:53 PDT
Created attachment 238362 [details] [diff] [review]
Combined patch with comment grammar nit fixed.
Comment 26 Boris Zbarsky [:bz] 2006-09-13 21:36:35 PDT
Checked in.  Thanks for the patch!

Note You need to log in before you can comment on or make changes to this bug.