Bug 189499 - [SVG] fragment-only url 'url(#fragment)' should be resolved against the current document with regardless to HTML <base> element
Summary: [SVG] fragment-only url 'url(#fragment)' should be resolved against the curre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 11
Hardware: All All
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
: 196950 200028 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-11 07:00 PDT by Ismael
Modified: 2019-09-02 18:56 PDT (History)
8 users (show)

See Also:


Attachments
Sample SVG with Gradient and Base Href (Gradient Renders on Chrome/Firefox, Not on Safari) (387 bytes, text/html)
2018-09-11 07:00 PDT, Ismael
no flags Details
wrong patch (9.39 KB, patch)
2019-07-30 01:06 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (11.98 KB, patch)
2019-07-30 01:07 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.21 MB, application/zip)
2019-07-30 02:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.68 MB, application/zip)
2019-07-30 02:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (2.99 MB, application/zip)
2019-07-30 02:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews215 for win-future (13.47 MB, application/zip)
2019-07-30 03:17 PDT, Build Bot
no flags Details
Patch (22.40 KB, patch)
2019-08-07 19:35 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (27.03 KB, patch)
2019-08-27 21:01 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (27.04 KB, patch)
2019-08-28 23:19 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (26.86 KB, patch)
2019-09-01 19:49 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ismael 2018-09-11 07:00:14 PDT
Created attachment 349394 [details]
Sample SVG with Gradient and Base Href  (Gradient Renders on Chrome/Firefox, Not on Safari)

When a <base href="/"> tag is used, it seems that WebKit/Safari explicitly require a path relative to the current page for a url fill rather than just requiring an id. Other browsers (Chrome and Firefox),
do not explicitly require the path for url fills. 

In practice, this means that SVGs with fill gradients are not displayed with gradients in Safari while they are displayed properly in Chrome/Firefox.
Comment 1 Radar WebKit Bug Importer 2018-09-14 12:54:58 PDT
<rdar://problem/44466206>
Comment 3 Simon Fraser (smfr) 2018-11-10 12:38:25 PST
Spec now says that CSS fragment identifiers don't resolve against the base URL: https://drafts.csswg.org/css-values/#local-urls
Comment 4 Ismael 2019-04-02 08:57:33 PDT
Any updates to this issue?
Comment 5 Said Abou-Hallawa 2019-05-06 15:26:02 PDT
*** Bug 196950 has been marked as a duplicate of this bug. ***
Comment 6 Said Abou-Hallawa 2019-07-23 09:51:20 PDT
*** Bug 200028 has been marked as a duplicate of this bug. ***
Comment 7 Fujii Hironori 2019-07-30 01:06:12 PDT
Created attachment 375149 [details]
wrong patch
Comment 8 Fujii Hironori 2019-07-30 01:07:19 PDT
Created attachment 375150 [details]
WIP patch
Comment 9 Build Bot 2019-07-30 02:15:19 PDT
Comment on attachment 375150 [details]
WIP patch

Attachment 375150 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12834825

New failing tests:
svg/custom/local-url-references.html
css3/filters/effect-reference-local-url-with-base.html
Comment 10 Build Bot 2019-07-30 02:15:20 PDT
Created attachment 375151 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 Build Bot 2019-07-30 02:23:10 PDT
Comment on attachment 375150 [details]
WIP patch

Attachment 375150 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12834831

New failing tests:
svg/custom/local-url-references.html
css3/filters/effect-reference-local-url-with-base.html
Comment 12 Build Bot 2019-07-30 02:23:12 PDT
Created attachment 375152 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 13 Build Bot 2019-07-30 02:59:55 PDT
Comment on attachment 375150 [details]
WIP patch

Attachment 375150 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12834857

New failing tests:
svg/custom/local-url-references.html
css3/filters/effect-reference-local-url-with-base.html
Comment 14 Build Bot 2019-07-30 02:59:57 PDT
Created attachment 375153 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 15 Build Bot 2019-07-30 03:17:18 PDT
Comment on attachment 375150 [details]
WIP patch

Attachment 375150 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12834908

New failing tests:
svg/custom/local-url-references.html
css3/filters/effect-reference-local-url-with-base.html
Comment 16 Build Bot 2019-07-30 03:17:27 PDT
Created attachment 375154 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 17 Fujii Hironori 2019-08-07 04:21:06 PDT
I added the test cases from the Blink commit.

(In reply to Build Bot from comment #15)
> New failing tests:
> svg/custom/local-url-references.html
> css3/filters/effect-reference-local-url-with-base.html

Both failures are other existing WebKit issues.

  Bug 200501 – [SVG] coordinates system issue of feImage referring an SVG element
  Bug 144012 – SVG filters are not applied correctly to HTML elements through css 'filter' property
Comment 18 Fujii Hironori 2019-08-07 19:35:19 PDT
Created attachment 375779 [details]
Patch
Comment 19 Simon Fraser (smfr) 2019-08-07 19:38:41 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

> Source/WebCore/ChangeLog:21
> +        (WebCore::SVGURIReference::fragmentIdentifierFromIRIString): Return the fragemnt if the URL starts with '#'.

fragemnt

> Source/WebCore/svg/SVGURIReference.cpp:61
> +    if (!start)
> +        return url.substring(1);

Should this use url.fragmentIdentifier() ?
Comment 20 Fujii Hironori 2019-08-07 19:44:54 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>> Source/WebCore/svg/SVGURIReference.cpp:61
>> +        return url.substring(1);
> 
> Should this use url.fragmentIdentifier() ?

With regardless to the variable name 'url', it is not a URL, but a String. I can't do so.
Comment 21 Said Abou-Hallawa 2019-08-08 11:21:27 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

> Source/WebCore/ChangeLog:8
> +

Can you please add a comment explaining what you are fixing in this patch?
Can you also mention that this patch is based on the Blink change https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df9167fad106%5E%21/?
Is there a reason for not porting the change in StyleBuilderConverter::convertClipPath()?

> LayoutTests/svg/animations/local-url-target-reference.html:18
> +  <set attributeName="fill" xlink:href="#rect" to="green" dur="indefinite"/>

You do not have to include the xlink namespace in the href attribute.

> LayoutTests/svg/custom/linking-base-external-reference.xhtml:-18
> -</html>

What is the reason for removing this test?

> LayoutTests/svg/custom/local-url-references-webkit.html:53
> +</svg>

Why "webkit" is included in the name of the file? Does it test specific WebKit functionalities? I think we should not do that.

> LayoutTests/svg/custom/local-url-references.html:53
> +</svg>

What is the difference between this test and local-url-references-webkit.html? Also this test and the other one are too long to read. They both test almost all the SVG resources. Can you split them into smaller ones?
Comment 22 Fujii Hironori 2019-08-09 00:26:47 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>> Source/WebCore/ChangeLog:8
>> +
> 
> Can you please add a comment explaining what you are fixing in this patch?
> Can you also mention that this patch is based on the Blink change https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df9167fad106%5E%21/?
> Is there a reason for not porting the change in StyleBuilderConverter::convertClipPath()?

Will fix.
StyleBuilderConverter::convertClipPath has a issue it doesn't work with external SVG references.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/StyleBuilderConverter.h?rev=248463#L594
However I will check it again.

>> Source/WebCore/ChangeLog:21
>> +        (WebCore::SVGURIReference::fragmentIdentifierFromIRIString): Return the fragemnt if the URL starts with '#'.
> 
> fragemnt

Will fix.

>> LayoutTests/svg/animations/local-url-target-reference.html:18
>> +  <set attributeName="fill" xlink:href="#rect" to="green" dur="indefinite"/>
> 
> You do not have to include the xlink namespace in the href attribute.

Will fix.

>> LayoutTests/svg/custom/linking-base-external-reference.xhtml:-18
>> -</html>
> 
> What is the reason for removing this test?

This is a test case for the old spec.

>> LayoutTests/svg/custom/local-url-references-webkit.html:53
>> +</svg>
> 
> Why "webkit" is included in the name of the file? Does it test specific WebKit functionalities? I think we should not do that.

I added local-url-references-webkit.html for WebKit issue (Comment 17). But, I will remove this.

>> LayoutTests/svg/custom/local-url-references.html:53
>> +</svg>
> 
> What is the difference between this test and local-url-references-webkit.html? Also this test and the other one are too long to read. They both test almost all the SVG resources. Can you split them into smaller ones?

I replaced <rect> with <polygon> to avoid Bug 200501.
Got it. I will split.
Comment 23 Fujii Hironori 2019-08-22 19:11:27 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>>> Source/WebCore/ChangeLog:8
>>> +
>> 
>> Can you please add a comment explaining what you are fixing in this patch?
>> Can you also mention that this patch is based on the Blink change https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df9167fad106%5E%21/?
>> Is there a reason for not porting the change in StyleBuilderConverter::convertClipPath()?
> 
> Will fix.
> StyleBuilderConverter::convertClipPath has a issue it doesn't work with external SVG references.
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/StyleBuilderConverter.h?rev=248463#L594
> However I will check it again.

I changed StyleBuilderConverter::convertClipPath to use SVGURIReference::fragmentIdentifierFromIRIString in r249040 (Bug 201030).
Comment 24 Fujii Hironori 2019-08-27 21:01:20 PDT
Created attachment 377422 [details]
Patch
Comment 25 Fujii Hironori 2019-08-27 21:05:51 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>>> LayoutTests/svg/animations/local-url-target-reference.html:18
>>> +  <set attributeName="fill" xlink:href="#rect" to="green" dur="indefinite"/>
>> 
>> You do not have to include the xlink namespace in the href attribute.
> 
> Will fix.

Umm, 'xlink:' of <set> element can't be removed. Other tests are also using 'xlink:href'. Why?
Comment 26 Said Abou-Hallawa 2019-08-28 11:01:47 PDT
Comment on attachment 375779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375779&action=review

>>>> LayoutTests/svg/animations/local-url-target-reference.html:18
>>>> +  <set attributeName="fill" xlink:href="#rect" to="green" dur="indefinite"/>
>>> 
>>> You do not have to include the xlink namespace in the href attribute.
>> 
>> Will fix.
> 
> Umm, 'xlink:' of <set> element can't be removed. Other tests are also using 'xlink:href'. Why?

This happens because of b201227.

We kept the existing tests with the xlink qualifier to make sure we are still backward compatible in this regard. But we should not use it in new tests to ensure that we get enough testing in different areas. And the href of the SVG animate elements was one case that I did not test in b153854
Comment 27 Fujii Hironori 2019-08-28 23:16:22 PDT
Thank you. Will fix.
Comment 28 Fujii Hironori 2019-08-28 23:19:57 PDT
Created attachment 377548 [details]
Patch
Comment 29 Fujii Hironori 2019-08-29 22:09:04 PDT
Said, do you have a chance to review this patch again?
Comment 30 Said Abou-Hallawa 2019-08-30 10:58:32 PDT
Comment on attachment 377548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377548&action=review

> LayoutTests/css3/filters/effect-reference-local-url-with-base.html:16
> +</style>
> +<svg>
> +  <filter id="filter" x="0" y="0" width="1" height="1" color-interpolation-filters="sRGB">
> +    <feFlood flood-color="green"/>
> +  </filter>
> +</svg>
> +<div id="target"></div>

If you swap the order of the <svg> and the <div> elements, the expected file will not have to have the mysterious  "<svg></svg>".

> LayoutTests/svg/custom/local-url-reference-clip-path.html:3
> +<svg width="400" height="300">

No need to set a size for the svg. The default size (300x15) should be sufficient for the drawing.

> LayoutTests/svg/custom/local-url-reference-fill-expected.html:4
> +<svg width="400" height="300">
> +    <rect width="100" height="100" fill="green"/>
> +</svg>

Like the other expected files, this can be replaced by:

<div style="width: 100px; height: 100px; background-color: green"></div>

> LayoutTests/svg/custom/local-url-reference-fill.html:3
> +<svg width="400" height="300">

Ditto.

> LayoutTests/svg/custom/local-url-reference-fill.html:7
> +    <linearGradient id="inheritedPaint" href="#paint"/>

inheritedPaint is defined but not used. Did you mean to add something like this:

    <g transform="translate(100)">
      <rect width="100" height="100" fill="url(#inheritedPaint) red"/>
    </g>

Or maybe you wanted to use it instead of #paint.

> LayoutTests/svg/custom/local-url-reference-filter-expected.html:4
> +<svg width="400" height="300">
> +    <rect width="100" height="100" fill="green"/>
> +</svg>

Ditto.

> LayoutTests/svg/custom/local-url-reference-stroke.html:7
> +    <linearGradient id="inheritedPaint" href="#paint"/>

inheritedPaint is defined but it is not used.
Comment 31 Fujii Hironori 2019-09-01 19:47:10 PDT
Comment on attachment 377548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377548&action=review

>> LayoutTests/css3/filters/effect-reference-local-url-with-base.html:16
>> +<div id="target"></div>
> 
> If you swap the order of the <svg> and the <div> elements, the expected file will not have to have the mysterious  "<svg></svg>".

It is needed to work around Bug 144012.

>> LayoutTests/svg/custom/local-url-reference-clip-path.html:3
>> +<svg width="400" height="300">
> 
> No need to set a size for the svg. The default size (300x15) should be sufficient for the drawing.

Will fix.

>> LayoutTests/svg/custom/local-url-reference-fill-expected.html:4
>> +</svg>
> 
> Like the other expected files, this can be replaced by:
> 
> <div style="width: 100px; height: 100px; background-color: green"></div>

Will fix.

>> LayoutTests/svg/custom/local-url-reference-fill.html:7
>> +    <linearGradient id="inheritedPaint" href="#paint"/>
> 
> inheritedPaint is defined but not used. Did you mean to add something like this:
> 
>     <g transform="translate(100)">
>       <rect width="100" height="100" fill="url(#inheritedPaint) red"/>
>     </g>
> 
> Or maybe you wanted to use it instead of #paint.

Right. I need one more <rect> element.
Comment 32 Fujii Hironori 2019-09-01 19:49:25 PDT
Created attachment 377833 [details]
Patch for landing
Comment 33 Fujii Hironori 2019-09-02 18:55:59 PDT
Comment on attachment 377833 [details]
Patch for landing

Clearing flags on attachment: 377833

Committed r249416: <https://trac.webkit.org/changeset/249416>
Comment 34 Fujii Hironori 2019-09-02 18:56:03 PDT
All reviewed patches have been landed.  Closing bug.