Bug 55750 - SVG <image> referenced by <use> is displayed incorrectly
Summary: SVG <image> referenced by <use> is displayed incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 33971 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-03-04 00:00 PST by Cosmin Truta
Modified: 2011-04-01 10:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.27 KB, patch)
2011-03-04 00:30 PST, Cosmin Truta
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2011-03-30 09:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2011-03-31 09:27 PDT, Rob Buis
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2011-03-04 00:00:09 PST
This is a Chromium-specific bug. The original test case runs well on Safari and on the other non-Chromium browsers.
Here is the original bug report:
http://code.google.com/p/chromium/issues/detail?id=42465

Underneath, however, all platforms need fixing.

The difference in behavior between Chromium and the other browsers is caused by the difference between KURL and KURLGoogle. The two URL implementation classes are slightly different in their handling of invalid URLs. However, in principle, this difference should not cause issues.

The KURL constructor takes the arguments (base, relative). When base is the empty string, and relative has no scheme (i.e. base="", relative="foo.svg"), the resulting KURL object is invalid. Both KURL and KURLGoogle agree on this.
The difference is that the KURL string (upon failure) is set to the relative part, while the KURLGoogle string (also upon failure) is set to the empty string:

base = ""
relative = "foo.svg"
----
KURL(base, relative).isValid() == false
KURL(base, relative).string() = "foo.svg"
----
KURLGooglePrivate(base, relative).isValid() == false
KURLGooglePrivate(base, relative).string() = ""

Neither of the two URL implementations is "right" or "wrong", it's just that KURL happens to work with this test case, while KURLGoogle, unluckily, doesn't.
The real problem here, which needs fixing, is that the resulting URL should not be invalid. The test case is perfectly legitimate.

The fix consists in setting the base part to the correct value, which is the URL of the document that contains the <image> element. This will lead to:
base = "scheme://path/to/base.svg"
relative = "foo.svg"
-----
KURL(base, relative).isValid() == KURLGooglePrivate(base, relative).isValid() == true
KURL(base, relative).string() == KURLGooglePrivate(base, relative).string() == "scheme://path/to/foo.svg"
Comment 1 Cosmin Truta 2011-03-04 00:29:56 PST
I opened this bug, although the bug 33971 already exists. I could not change the title of that bug, which, among other things, is littered with red herrings of various kinds.
Can someone with the applicable permissions please mark the other bug as a duplicate of this one?
Comment 2 Cosmin Truta 2011-03-04 00:30:39 PST
Created attachment 84704 [details]
Patch
Comment 3 Adam Barth 2011-03-04 13:28:53 PST
Comment on attachment 84704 [details]
Patch

looks right to me.  The call to stripLeadingAndTrailingHTMLSpaces probably isn't correct, but that's another issue.
Comment 4 Fady Samuel 2011-03-07 12:48:42 PST
*** Bug 33971 has been marked as a duplicate of this bug. ***
Comment 5 WebKit Commit Bot 2011-03-07 17:49:44 PST
The commit-queue encountered the following flaky tests while processing attachment 84704 [details]:

http/tests/media/video-load-twice.html bug 51138 (authors: eric.carlson@apple.com and jamesr@chromium.org)
The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2011-03-07 17:51:55 PST
Comment on attachment 84704 [details]
Patch

Clearing flags on attachment: 84704

Committed r80516: <http://trac.webkit.org/changeset/80516>
Comment 7 WebKit Commit Bot 2011-03-07 17:52:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2011-03-07 19:15:58 PST
http://trac.webkit.org/changeset/80517 might have broken GTK Linux 32-bit Release
Comment 9 Nikolas Zimmermann 2011-03-14 12:44:13 PDT
This broke LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg. I'm wondering why this hasn't been noticed by the Chromium folks? Is the test skipped on all platforms?
Comment 10 Nikolas Zimmermann 2011-03-25 01:52:28 PDT
Ping! struct-image-07-t pixel test remains broken, any news?
Comment 11 Rob Buis 2011-03-30 09:12:54 PDT
Created attachment 87546 [details]
Patch
Comment 12 Dirk Schulze 2011-03-30 10:37:58 PDT
Comment on attachment 87546 [details]
Patch

We just have one pixel test failing. Can you create DRT test for this?
Comment 13 Rob Buis 2011-03-31 09:27:54 PDT
Created attachment 87747 [details]
Patch
Comment 14 Nikolas Zimmermann 2011-04-01 08:10:29 PDT
Comment on attachment 87747 [details]
Patch

Looks great, r=me.
Comment 15 Rob Buis 2011-04-01 10:20:54 PDT
Landed in r82689.