Bug 37187 – SVG <use href="foo"> is interpreted as <use href="#foo">

• Component: SVG
• Status: RESOLVED FIXED

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 52703 [details]
patch v0

[Eric Seidel (no email)]

We have an ancient bug about this.  bug 6007.

[Eric Seidel (no email)]

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

[Hajime Morrita]

Created attachment 53033 [details]
v1; add explanations to testcase, fix comment

[Hajime Morrita]

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.

[Nikolas Zimmermann]

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)

[Hajime Morrita]

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.

[Nikolas Zimmermann]

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.

[Hajime Morrita]

Committed r58187: <http://trac.webkit.org/changeset/58187>

[James Robinson]

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

[Hajime Morrita]

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

[Hajime Morrita]

Committed r58195: <http://trac.webkit.org/changeset/58195>