Bug 37187 - SVG <use href="foo"> is interpreted as <use href="#foo">
Summary: SVG <use href="foo"> is interpreted as <use href="#foo">
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-06 21:00 PDT by Hajime Morrita
Modified: 2010-05-05 22:20 PDT (History)
3 users (show)

See Also:


Attachments
reproduce - an expected behaviour is not show anything. (463 bytes, image/svg+xml)
2010-04-06 21:00 PDT, Hajime Morrita
no flags Details
patch v0 (12.17 KB, patch)
2010-04-06 22:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
v1; add explanations to testcase, fix comment (10.61 KB, patch)
2010-04-09 23:19 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>