Bug 41655 – Implement SVGSVGElement.getElementById

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Need to implement and expose it in the js to fix the referenced url.

[Rob Buis]

Created attachment 60591 [details]
Patch

[Eric Seidel (no email)]

Attachment 60591 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3376315

[WebKit Review Bot]

Attachment 60591 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3372362

[WebKit Review Bot]

Attachment 60591 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3347568

[Eric Seidel (no email)]

Comment on attachment 60591 [details]
Patch

Can you please link to the relevant part of the spec in your implementation.  A comment with the URL would be ideal.

r- due to build breaks (and missing spec justification).

[Early Warning System Bot]

Attachment 60591 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3415279

[Darin Adler]

Comment on attachment 60591 [details]
Patch

There's an extra open brace in the patch, which is why it's not compiling.

Should be "Node*" and it would be better to use a word than a letter for the local variable "n".

[Eric Seidel (no email)]

You should just always use getElemenetById(), and if you're not the root SVG element, check that the returned element is under the <Svg> before returning it.  If it's not, you could do a manual crawl for the duplicate entry.

There might even be a getElementsById() function for getting all fo them.

Make sense?

[Rob Buis]

Created attachment 60748 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 60748 [details]
Patch

What about duplicate ids?

<div id="foo">
<svg>
<rect id="foo">
</svg>
<div id="foo>

I think would cause your code to wrongly fail.  I don't remember which id wins for "getElementById", but you want to check all of them to see if any are under <svg> (if thee is a gitElementByIds) or you need to fall back to the walking in teh case where you find an id but it isn't an ancestor.

[Eric Seidel (no email)]

Your test cases are also really hard to read.  Can you make them more minimal?

[Rob Buis]

Hi Eric,

(In reply to comment #11)
> Your test cases are also really hard to read.  Can you make them more minimal?

I tried to keep it close to the IE test. If keeping that connection is not so important I can
certainly strip it if needed. Maybe even the variant that uses the js framework with
shouldBe tests etc.?
Cheers,

Rob.

[Rob Buis]

Hi Eric,

(In reply to comment #10)
> (From update of attachment 60748 [details])
> What about duplicate ids?
> 
> <div id="foo">
> <svg>
> <rect id="foo">
> </svg>
> <div id="foo>
> 
> I think would cause your code to wrongly fail.  I don't remember which id wins for "getElementById", but you want to check all of them to see if any are under <svg> (if thee is a gitElementByIds) or you need to fall back to the walking in teh case where you find an id but it isn't an ancestor.

Officially duplicate ids are undefined IIRC. But I guess it makes sense to match Document.getElementById, so I'll look into fixing that. Thanks for the review!
Cheers,

Rob.

[Eric Seidel (no email)]

The point of my test was duplicates *outside* of the <svg>, which should not be undefined for your case.

But you're right.  You also need to test duplicates *inside* the <svg>.

1. <div id="foo"><svg><rect id="foo"></svg><div id="foo">

As well as:

2. <div id="foo"><svg><rect id="foo"><rect id="foo"></svg><div id="foo">

Ideally that would behave the same as:

3. <svg><rect id="foo"><rect id="foo"></svg>

But if it's truly undefined, then the correlation between the last two tests is not required.

1. Will use the fallback walk-the-tree behavior, but will only find the one element.
2.  Will use fallack behavior, but which element will it find?  (probably the first)
3.  Will use document.getElementById() default behavior, but which element will it find?  Hopefully the same as #2 would.

I don't think it's important to use the IE test cases.  I think we can make our own which are less gnarly.

[Rob Buis]

Created attachment 60923 [details]
Patch

Testcase is now more minimal and contains Eric's suggested tests. Fallback to subtree search is included.

[Eric Seidel (no email)]

Comment on attachment 60923 [details]
Patch

WebCore/svg/SVGSVGElement.cpp:600
 +      if (element && element->isSVGElement() && element->isDescendantOf(this))
Why isSVGElement()?  Please test that case by adding some foreignObject content (or simply non-SVG content) under the SVG tree.

WebCore/svg/SVGSVGElement.cpp:606
 +          if (node->isElementNode()) {
This should use the "early return" style, with an early continue here.

LayoutTests/svg/custom/svg-getelementid.xhtml:3
 +      <div id="foo">
This does not test "foo" both before and after, like suggested in my comment.

LayoutTests/svg/custom/svg-getelementid-expected.txt:1
 +  PASS
If you're going to go to all the trouble to re-write the test, you might as well use the script-test js files (even if you don't make this a script test).  Teh output is much much much nicer to read.

LayoutTests/svg/custom/svg-getelementid.xhtml:4
 +        <div id="bar">
Why put all the content inside these divs instead of having them before or after the SVG content?

[Rob Buis]

Created attachment 60942 [details]
Patch

Improved testcase and code improvements

[Eric Seidel (no email)]

Comment on attachment 60942 [details]
Patch

This is a fantastic change.  Thank you!

[Nikolas Zimmermann]

Rob, did you forget to close this bug?

[Rob Buis]

Niko, I didnt forget, but always wait a bit for buildbot errors :) Can now safely close I think though.