Need to implement and expose it in the js to fix the referenced url.
Created attachment 60591 [details] Patch
Attachment 60591 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3376315
Attachment 60591 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3372362
Attachment 60591 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3347568
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).
Attachment 60591 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3415279
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".
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?
Created attachment 60748 [details] Patch
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.
Your test cases are also really hard to read. Can you make them more minimal?
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.
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.
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.
Created attachment 60923 [details] Patch Testcase is now more minimal and contains Eric's suggested tests. Fallback to subtree search is included.
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?
Created attachment 60942 [details] Patch Improved testcase and code improvements
Comment on attachment 60942 [details] Patch This is a fantastic change. Thank you!
Rob, did you forget to close this bug?
Niko, I didnt forget, but always wait a bit for buildbot errors :) Can now safely close I think though.