Bug 189499 – [SVG] fragment-only url 'url(#fragment)' should be resolved against the current document with regardless to HTML <base> element

• Component: SVG
• Status: RESOLVED FIXED

[Ismael]

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.

[Radar WebKit Bug Importer]

<rdar://problem/44466206>

[Simon Fraser (smfr)]

We should merge https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df9167fad106%5E%21/

[Simon Fraser (smfr)]

Spec now says that CSS fragment identifiers don't resolve against the base URL: https://drafts.csswg.org/css-values/#local-urls

[Ismael]

Any updates to this issue?

[Said Abou-Hallawa]

*** Bug 196950 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

*** Bug 200028 has been marked as a duplicate of this bug. ***

[Fujii Hironori]

Created attachment 375149 [details]
wrong patch

[Fujii Hironori]

Created attachment 375150 [details]
WIP patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 375779 [details]
Patch

[Simon Fraser (smfr)]

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() ?

[Fujii Hironori]

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.

[Said Abou-Hallawa]

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?

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 377422 [details]
Patch

[Fujii Hironori]

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?

[Said Abou-Hallawa]

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

[Fujii Hironori]

Thank you. Will fix.

[Fujii Hironori]

Created attachment 377548 [details]
Patch

[Fujii Hironori]

Said, do you have a chance to review this patch again?

[Said Abou-Hallawa]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 377833 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 377833 [details]
Patch for landing

Clearing flags on attachment: 377833

Committed r249416: <https://trac.webkit.org/changeset/249416>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.