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.
<rdar://problem/44466206>
We should merge https://chromium.googlesource.com/chromium/src/+/e7d7225c33aa7fc42ee390125b01df9167fad106%5E%21/
Spec now says that CSS fragment identifiers don't resolve against the base URL: https://drafts.csswg.org/css-values/#local-urls
Any updates to this issue?
*** Bug 196950 has been marked as a duplicate of this bug. ***
*** Bug 200028 has been marked as a duplicate of this bug. ***
Created attachment 375149 [details] wrong patch
Created attachment 375150 [details] WIP patch
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
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 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
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 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
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 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
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
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
Created attachment 375779 [details] Patch
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 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 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 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 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).
Created attachment 377422 [details] Patch
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 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
Thank you. Will fix.
Created attachment 377548 [details] Patch
Said, do you have a chance to review this patch again?
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 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.
Created attachment 377833 [details] Patch for landing
Comment on attachment 377833 [details] Patch for landing Clearing flags on attachment: 377833 Committed r249416: <https://trac.webkit.org/changeset/249416>
All reviewed patches have been landed. Closing bug.