Bug 37187

Summary: SVG <use href="foo"> is interpreted as <use href="#foo">
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jamesr, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
reproduce - an expected behaviour is not show anything.
none
patch v0
none
v1; add explanations to testcase, fix comment none

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 1 Hajime Morrita 2010-04-06 22:50:30 PDT
Created attachment 52703 [details]
patch v0
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.
Comment 12 Hajime Morrita 2010-04-23 15:32:45 PDT
Committed r58195: <http://trac.webkit.org/changeset/58195>