Bug 41655

Summary: Implement SVGSVGElement.getElementById
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gns, webkit-ews, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: http://samples.msdn.microsoft.com/ietestcenter/svg112/svg/svg/chapter_05.4.svg
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch eric: review+

Description Rob Buis 2010-07-05 23:22:38 PDT
Need to implement and expose it in the js to fix the referenced url.
Comment 1 Rob Buis 2010-07-05 23:31:31 PDT
Created attachment 60591 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-07-05 23:40:15 PDT
Attachment 60591 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3376315
Comment 3 WebKit Review Bot 2010-07-05 23:45:32 PDT
Attachment 60591 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3372362
Comment 4 WebKit Review Bot 2010-07-05 23:50:14 PDT
Attachment 60591 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3347568
Comment 5 Eric Seidel (no email) 2010-07-05 23:54:20 PDT
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).
Comment 6 Early Warning System Bot 2010-07-05 23:58:26 PDT
Attachment 60591 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3415279
Comment 7 Darin Adler 2010-07-06 08:12:33 PDT
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".
Comment 8 Eric Seidel (no email) 2010-07-07 04:06:43 PDT
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?
Comment 9 Rob Buis 2010-07-07 10:01:33 PDT
Created attachment 60748 [details]
Patch
Comment 10 Eric Seidel (no email) 2010-07-07 13:42:13 PDT
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.
Comment 11 Eric Seidel (no email) 2010-07-07 13:42:31 PDT
Your test cases are also really hard to read.  Can you make them more minimal?
Comment 12 Rob Buis 2010-07-07 14:09:52 PDT
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.
Comment 13 Rob Buis 2010-07-07 14:11:37 PDT
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.
Comment 14 Eric Seidel (no email) 2010-07-07 14:17:40 PDT
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.
Comment 15 Rob Buis 2010-07-08 11:59:10 PDT
Created attachment 60923 [details]
Patch

Testcase is now more minimal and contains Eric's suggested tests. Fallback to subtree search is included.
Comment 16 Eric Seidel (no email) 2010-07-08 12:13:21 PDT
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?
Comment 17 Rob Buis 2010-07-08 13:27:34 PDT
Created attachment 60942 [details]
Patch

Improved testcase and code improvements
Comment 18 Eric Seidel (no email) 2010-07-08 13:34:06 PDT
Comment on attachment 60942 [details]
Patch

This is a fantastic change.  Thank you!
Comment 19 Nikolas Zimmermann 2010-07-09 07:29:57 PDT
Rob, did you forget to close this bug?
Comment 20 Rob Buis 2010-07-09 07:36:50 PDT
Niko, I didnt forget, but always wait a bit for buildbot errors :) Can now safely close I think though.