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.

I don't think this is the way we should do this given bug 75435, but it's an option.

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.

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.

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

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

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.

Is the behavior of that patch if there are multiple elements with the same id the one the spec calls for?

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

I was actually hoping to get this method removed from that interface.

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.

Let's see how Jonathan gets on with that then.

The IE test center has tests for this now...

Comment on attachment 571372 [details] [diff] [review] corrected mochitest Looks good to me.

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

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

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!

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

Comment on attachment 571637 [details] [diff] [review] nsPromiseFlatString You don't needthe "ns" bit. Just PromiseFlatString(elementId).

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.

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/052bf8bd491e

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?