SVGSVGElement.getElementById not implemented

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: jwatt, Assigned: Robert Longson)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
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.
(Reporter)

Comment 2

13 years ago
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.
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 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.
Attachment #172854 - Flags: review-
Oh, and feel free to make those changes in nsXMLDocument too ;-)
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.
Does the spec actually define behavior when multiple elements have the same ID, btw?
no, it says it's undefined.
Could someone raise that with the WG?  Or should I?
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"
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.
I sent mail to the WG, btw.
(Reporter)

Comment 13

13 years ago
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
Status: NEW → ASSIGNED
(Reporter)

Comment 14

12 years ago
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
(Reporter)

Updated

12 years ago
Assignee: jonathan.watt → wattie
Status: ASSIGNED → NEW
QA Contact: ian → general
(Assignee)

Comment 15

7 years ago
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.
Assignee: wattie → longsonr
Attachment #456625 - Flags: review?(jwatt)
Is the behavior of that patch if there are multiple elements with the same id the one the spec calls for?
(Assignee)

Comment 17

7 years ago
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.
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...
(Reporter)

Comment 19

7 years ago
I was actually hoping to get this method removed from that interface.
(Assignee)

Comment 20

7 years ago
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?
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.
(Assignee)

Comment 22

7 years ago
Let's see how Jonathan gets on with that then.
(Assignee)

Updated

7 years ago
Attachment #456625 - Flags: review?(jwatt)
(Assignee)

Updated

7 years ago
Assignee: longsonr → nobody
The IE test center has tests for this now...
Blocks: 554013
Duplicate of this bug: 589645
(Assignee)

Comment 25

6 years ago
Created attachment 571354 [details] [diff] [review]
updated patch
Attachment #456625 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Created attachment 571372 [details] [diff] [review]
corrected mochitest
Attachment #571354 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #571372 - Flags: review?(jwatt)
Attachment #571372 - Flags: feedback?(bzbarsky)
Comment on attachment 571372 [details] [diff] [review]
corrected mochitest

Looks good to me.
Attachment #571372 - Flags: feedback?(bzbarsky) → feedback+
(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.
(Reporter)

Comment 29

6 years ago
Does element.querySelector('#id') work for elements not in the document?
> 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?
Well, apart from the obvious: you'd have to CSS-escape the id....
(Assignee)

Comment 32

6 years ago
Created attachment 571594 [details] [diff] [review]
address review comments
Assignee: nobody → longsonr
Attachment #172852 - Attachment is obsolete: true
Attachment #172854 - Attachment is obsolete: true
Attachment #571372 - Attachment is obsolete: true
Attachment #571594 - Flags: review?(jwatt)
Attachment #571594 - Flags: feedback?(bzbarsky)
Attachment #571372 - Flags: review?(jwatt)
(Assignee)

Comment 33

6 years ago
Created attachment 571595 [details] [diff] [review]
improve mochitest
Attachment #571594 - Attachment is obsolete: true
Attachment #571595 - Flags: review?(jwatt)
Attachment #571595 - Flags: feedback?(bzbarsky)
Attachment #571594 - Flags: review?(jwatt)
Attachment #571594 - Flags: feedback?(bzbarsky)
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!
Attachment #571595 - Flags: feedback?(bzbarsky) → feedback-
(Assignee)

Comment 35

6 years ago
Created attachment 571624 [details] [diff] [review]
address review comments
Attachment #571595 - Attachment is obsolete: true
Attachment #571624 - Flags: review?(bzbarsky)
Attachment #571595 - Flags: review?(jwatt)
(Assignee)

Comment 36

6 years ago
I had to convert elementId to an nsAutoString to pass into nsStyleUtil::AppendEscapedCSSIdent
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
Attachment #571624 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 38

6 years ago
Created attachment 571637 [details] [diff] [review]
nsPromiseFlatString
Attachment #571624 - Attachment is obsolete: true
Attachment #571637 - Flags: review?(jwatt)
Comment on attachment 571637 [details] [diff] [review]
nsPromiseFlatString

You don't needthe "ns" bit.  Just PromiseFlatString(elementId).
(Assignee)

Comment 40

6 years ago
Created attachment 571663 [details] [diff] [review]
PromiseFlatString
Attachment #571637 - Attachment is obsolete: true
Attachment #571663 - Flags: review?(jwatt)
Attachment #571637 - Flags: review?(jwatt)
(Reporter)

Comment 41

6 years ago
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.
Attachment #571663 - Flags: review?(jwatt) → review+
(Assignee)

Comment 42

6 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/052bf8bd491e
Flags: in-testsuite+

Updated

6 years ago
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/052bf8bd491e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: dev-doc-needed
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.
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?
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Duplicate of this bug: 732367
You need to log in before you can comment on or make changes to this bug.