Last Comment Bug 280391 - SVGSVGElement.getElementById not implemented
: SVGSVGElement.getElementById not implemented
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla11
Assigned To: Robert Longson
:
Mentors:
http://www.w3.org/TR/SVG/struct.html#...
: 589645 732367 (view as bug list)
Depends on:
Blocks: ietestcenter
  Show dependency treegraph
 
Reported: 2005-01-29 20:41 PST by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2014-04-25 15:13 PDT (History)
19 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch defering to nsXMLDocument::GetElementById (2.44 KB, patch)
2005-01-29 20:44 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch doing brute force search (4.74 KB, patch)
2005-01-29 20:45 PST, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
jonas: review-
Details | Diff | Review
patch (3.70 KB, patch)
2010-07-09 14:23 PDT, Robert Longson
no flags Details | Diff | Review
updated patch (3.57 KB, patch)
2011-11-02 09:24 PDT, Robert Longson
no flags Details | Diff | Review
corrected mochitest (3.53 KB, patch)
2011-11-02 10:48 PDT, Robert Longson
bzbarsky: feedback+
Details | Diff | Review
address review comments (3.66 KB, patch)
2011-11-03 03:56 PDT, Robert Longson
no flags Details | Diff | Review
improve mochitest (3.66 KB, patch)
2011-11-03 03:59 PDT, Robert Longson
bzbarsky: feedback-
Details | Diff | Review
address review comments (4.71 KB, patch)
2011-11-03 06:34 PDT, Robert Longson
bzbarsky: review+
Details | Diff | Review
nsPromiseFlatString (4.72 KB, patch)
2011-11-03 07:31 PDT, Robert Longson
no flags Details | Diff | Review
PromiseFlatString (4.72 KB, patch)
2011-11-03 09:01 PDT, Robert Longson
jwatt: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2005-01-29 20:41:55 PST
I'm trying to get rid of some of the "in progress" stuff I've been working on.
This isn't all that high a priority, but it gives me somewhere to put these patches.
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2005-01-29 20:44:08 PST
Created attachment 172852 [details] [diff] [review]
patch defering to nsXMLDocument::GetElementById

I don't think this is the way we should do this given bug 75435, but it's an
option.
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2005-01-29 20:45:15 PST
Created attachment 172854 [details] [diff] [review]
patch doing brute force search

I think this is the way we should do this given the comment in the patch.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-29 21:15:50 PST
Since the spec requires this function to work on fragments that aren't in the
document I guess the second patch is the way to go.

Though once we actually do keep a hash in the document it might be faster to
check if the element is in the document and if so ask the document. However you
would also have to check that the returned element is actually a descendent of
the svgelement. Anyway, that would be for later.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-29 21:22:09 PST
Comment on attachment 172854 [details] [diff] [review]
patch doing brute force search

I know you just copied this code, but the code is way inefficient.

First off, there is no reason to QI to nsIXMLContent. GetID lives on
nsIStyledContent.

Second, there's no need for a special codepath for html. HTML elements
implement nsIStyledContent. Especially given that the html codepath is actually
slower then the generic one :).

This also means that you can get rid of the aId argument.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-29 21:24:13 PST
Oh, and feel free to make those changes in nsXMLDocument too ;-)
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-29 21:27:46 PST
Actually, if you really want to speed things up you can move GetID to nsIContent
and get rid of the QI alltogether. There's nothing 'style' specific about IDs,
there's no reason for that function not to live on nsIContent.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-01-29 23:06:12 PST
Does the spec actually define behavior when multiple elements have the same ID, btw?
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-30 08:42:36 PST
no, it says it's undefined.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-01-30 09:11:52 PST
Could someone raise that with the WG?  Or should I?
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2005-01-30 10:32:06 PST
I don't think there is anything to rise. The spec explicitly says "Behavior is
not defined if more than one element has this id"
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-01-30 11:42:21 PST
http://w3.org/TR/SVG11/struct.html#SVGElement is not clear about whether
behavior is only undefined if two elements in the document fragment involved
have the same id, or whether it's undefined if two elements in the document have
the same ID.

So as things stand, people could argue that the behavior is well-defined if
there is a unique element with that ID that has the SVGElement as ancestor. 
Which would mean we can't do what you suggested in comment 3...

Hence my suggestion that we raise this in the WG.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-01-30 11:45:41 PST
I sent mail to the WG, btw.
Comment 13 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2005-01-31 05:02:12 PST
Thanks for the comments. I'll get back to this after a few other SVG bugs that
are higher on my priority list.

Thanks for raising the issue with the WG boris. No doubt we'll look back and
wonder what happened, and just so we can be absolutely sure they didn't respond
;-) the thread is here:
http://lists.w3.org/Archives/Public/www-svg/2005Jan/thread.html#120
Comment 14 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2005-11-10 18:50:16 PST
Ah well, there's always the exception that proves the rule. ;-) Continued here:
http://lists.w3.org/Archives/Public/www-svg/2005Feb/thread.html#15
Comment 15 Robert Longson 2010-07-09 14:23:03 PDT
Created attachment 456625 [details] [diff] [review]
patch

This only works with elements in documents but I think that's pretty reasonable. I can't see anything in the 2nd edition that says otherwise.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-07-09 14:27:50 PDT
Is the behavior of that patch if there are multiple elements with the same id the one the spec calls for?
Comment 17 Robert Longson 2010-07-09 14:32:03 PDT
The spec does not define what happens. Comment 14 suggests that may at some point be defined but it currently isn't so I'd argue that if it is defined we make sure that the definition is compatible with the behaviour when getElementById is called on a document.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-07-09 14:38:10 PDT
When called on a document, the first element in the document with that id is returned.

With your code, your getElementById will return null if the first element with that id in the document is not your descendant, and will return the element if it is...
Comment 19 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2010-07-09 14:44:07 PDT
I was actually hoping to get this method removed from that interface.
Comment 20 Robert Longson 2010-07-09 14:47:28 PDT
The testsuite test is here: http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_05.6.svg

Regarding comment 18. Do you think dealing with that in a more sensible fashion which would probably be to return the element that is your descendent is worth the code required to write it when the specification says it's undefined?
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-07-09 14:48:23 PDT
Given that I consider such a claim in the spec to be a spec bug...  dunno.  I'd much rather remove the method than have something whose behavior is undefined.
Comment 22 Robert Longson 2010-07-09 15:58:05 PDT
Let's see how Jonathan gets on with that then.
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-08-22 23:52:00 PDT
The IE test center has tests for this now...
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-08-22 23:52:10 PDT
*** Bug 589645 has been marked as a duplicate of this bug. ***
Comment 25 Robert Longson 2011-11-02 09:24:22 PDT
Created attachment 571354 [details] [diff] [review]
updated patch
Comment 26 Robert Longson 2011-11-02 10:48:24 PDT
Created attachment 571372 [details] [diff] [review]
corrected mochitest
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-02 10:57:00 PDT
Comment on attachment 571372 [details] [diff] [review]
corrected mochitest

Looks good to me.
Comment 28 :Ms2ger 2011-11-02 12:03:23 PDT
(In reply to Robert Longson from comment #15)
> Created attachment 456625 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> This only works with elements in documents but I think that's pretty
> reasonable.

I don't actually think that's reasonable.
Comment 29 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-02 12:06:32 PDT
Does element.querySelector('#id') work for elements not in the document?
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-02 12:11:30 PDT
> Does element.querySelector('#id') work for elements not in the document?

Yes.

I'd actually missed the not-in-document issue above.

Is there a reason this code can't just call querySelector?
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-02 12:12:01 PDT
Well, apart from the obvious: you'd have to CSS-escape the id....
Comment 32 Robert Longson 2011-11-03 03:56:25 PDT
Created attachment 571594 [details] [diff] [review]
address review comments
Comment 33 Robert Longson 2011-11-03 03:59:38 PDT
Created attachment 571595 [details] [diff] [review]
improve mochitest
Comment 34 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-03 06:05:08 PDT
Comment on attachment 571595 [details] [diff] [review]
improve mochitest

>+  selector.Append(elementId);

You want nsStyleUtil::AppendEscapedCSSIdent(elementId, selector).

Otherwise getElementById("foo bar") won't work right, and similar for "foo > bar", "foo~bar", "foo+bar", etc, etc.  Please add tests!
Comment 35 Robert Longson 2011-11-03 06:34:12 PDT
Created attachment 571624 [details] [diff] [review]
address review comments
Comment 36 Robert Longson 2011-11-03 06:38:19 PDT
I had to convert elementId to an nsAutoString to pass into nsStyleUtil::AppendEscapedCSSIdent
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-03 06:55:52 PDT
Comment on attachment 571624 [details] [diff] [review]
address review comments

You should be able to use PromiseFlatString() there instead of nsAutoString.

r=me with that change
Comment 38 Robert Longson 2011-11-03 07:31:24 PDT
Created attachment 571637 [details] [diff] [review]
nsPromiseFlatString
Comment 39 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-03 07:35:56 PDT
Comment on attachment 571637 [details] [diff] [review]
nsPromiseFlatString

You don't needthe "ns" bit.  Just PromiseFlatString(elementId).
Comment 40 Robert Longson 2011-11-03 09:01:01 PDT
Created attachment 571663 [details] [diff] [review]
PromiseFlatString
Comment 41 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-11-08 03:10:52 PST
Comment on attachment 571663 [details] [diff] [review]
PromiseFlatString

I've been persuaded by longsonr and others that we should fix this bug for now. The IE guys I've been talking to want to look at removing it in SVG2, even though I explained why that won't happen if we land this fix. Anyways, I guess there are more important battles to fight.
Comment 43 Marco Bonardo [::mak] 2011-11-09 05:18:45 PST
https://hg.mozilla.org/mozilla-central/rev/052bf8bd491e
Comment 44 :Ms2ger 2011-11-10 03:18:51 PST
For documentation, note that we always return the first element in tree order with the given ID within the subtree rooted at the SVGSVGElement the method was called on.
Comment 45 Jeremie Patonnier :Jeremie 2012-02-11 14:11:44 PST
Doc complete :

https://developer.mozilla.org/en/Firefox_11_for_developers
https://developer.mozilla.org/en/DOM/SVGSVGElement

(In reply to Ms2ger from comment #44)
> For documentation, note that we always return the first element in tree
> order with the given ID within the subtree rooted at the SVGSVGElement the
> method was called on.

I think the doc is already clear on that. Do you thing something else is required?
Comment 46 :Ms2ger 2012-03-02 03:50:03 PST
*** Bug 732367 has been marked as a duplicate of this bug. ***

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