|Summary:||SVG <use href="foo"> is interpreted as <use href="#foo">|
|Product:||WebKit||Reporter:||Hajime Morrita <morrita>|
|Severity:||Normal||CC:||eric, jamesr, zimmermann|
|Version:||528+ (Nightly build)|
Description Hajime Morrita 2010-04-06 21:00:57 PDT
Created attachment 52699 [details] reproduce - an expected behaviour is not show anything. in SVG, <use xlink:href="foo"> refers <defs id="foo">. But it shoudn't. It should be referenced as <use xlink:href="#foo">, but not href="foo". See attachment to reproduce. Glancing code, I suspect that almost every internal reference ("fragment identifier") handling in svg/ would be wrong. Because SVGURIReference::getTarget() looks broken, which is widely used in SVG.
Comment 2 Eric Seidel (no email) 2010-04-09 13:27:36 PDT
We have an ancient bug about this. bug 6007.
Comment 3 Eric Seidel (no email) 2010-04-09 13:29:24 PDT
Comment on attachment 52703 [details] patch v0 drive-by nits: The test case is difficult to read. Can we make it simpler? Also, this is not really english: 63 } else // The url doesn't have no target internal reference
Comment 4 Hajime Morrita 2010-04-09 23:19:18 PDT
Created attachment 53033 [details] v1; add explanations to testcase, fix comment
Comment 5 Hajime Morrita 2010-04-09 23:30:47 PDT
Eric, thank you for reviewing! (In reply to comment #3) > (From update of attachment 52703 [details]) > drive-by nits: > The test case is difficult to read. Can we make it simpler? I hope we could use script-test but SVG DOM doesn't provide to the way to get linked node. I add an explanation to the testcase to help understanding what the case does. > Also, this is not really english: > 63 } else // The url doesn't have no target internal reference Oops. (try to) fix it... BTW, Bug 6007 looks already be handled inside code because referring invalid node cause errors. I agree we need tests for them, although.
Comment 6 Nikolas Zimmermann 2010-04-22 09:49:07 PDT
This probably needs to be updated if bug 37986 lands. Fix looks good though, as Eric reviewed the first round, I'd like him to give final r+ though. Are you sure this doesn't affect any other pixel tests btw? (Run run-webkit-tests --tolerance 0 -p svg, with 0% tolerance)
Comment 7 Hajime Morrita 2010-04-22 14:17:44 PDT
Nikolas, thank you for reviewing! (In reply to comment #6) > This probably needs to be updated if bug 37986 lands. Wow. A huge change is coming... I hope this patch would land before that. > Are you sure this doesn't affect any other pixel tests btw? (Run > run-webkit-tests --tolerance 0 -p svg, with 0% tolerance) Yes. The results don't change even after the patch is applied.
Comment 8 Nikolas Zimmermann 2010-04-23 01:20:56 PDT
Comment on attachment 53033 [details] v1; add explanations to testcase, fix comment Okay the patch is simple enough, I will just give r+, so you have a chance to land before the big refactoring sets in.
Comment 9 Hajime Morrita 2010-04-23 13:40:49 PDT
Committed r58187: <http://trac.webkit.org/changeset/58187>
Comment 10 James Robinson 2010-04-23 15:08:22 PDT
The ChangeLog entry mentions these files: * platform/mac/svg/custom/broken-internal-references-expected.checksum: Added. * platform/mac/svg/custom/broken-internal-references-expected.png: Added. but it looks like you didn't actually check them in? The pixel tests will fail without these results
Comment 11 Hajime Morrita 2010-04-23 15:23:18 PDT
> The ChangeLog entry mentions these files: > > * platform/mac/svg/custom/broken-internal-references-expected.checksum: > Added. > * platform/mac/svg/custom/broken-internal-references-expected.png: Added. > > but it looks like you didn't actually check them in? The pixel tests will fail > without these results I'm sorry. The checkin missed them. Now I'm preparing them.