Closed
Bug 1302779
Opened 9 years ago
Closed 9 years ago
CSS clip-path referencing an inline SVG does not work if document url is not identical to base url
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
| Tracking | Status | |
|---|---|---|
| firefox52 | --- | fixed |
People
(Reporter: duanyao.ustc, Assigned: u459114)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160912030421
Steps to reproduce:
Try to reference an inline SVG clipPath in CSS clip-path, and the document url is not identical to the base url. Code:
<!DOCTYPE html>
<base href="http://example.com" >
<div style="width:300px; height:300px; background-color:yellow;
font-size: 40px;
clip-path: url(#c1); -webkit-clip-path: url(#c1)">
An div to be clipped An div to be clipped An div to be clipped An div to be clipped
</div>
<svg height="0" width="0">
<defs>
<clipPath id="c1">
<circle cy="110" cx="137" r="90" />
</clipPath>
</defs>
</svg>
You can load the attached html file to reproduce this issue.
Actual results:
The clipPath is not applied.
Expected results:
The clipPath is applied.
Chrome 53 works as expected, however it seems due to a bug: Chrome ignores everything before the fragment part of the url in CSS clip-path.
I guess Firefox always resolves urls in clip-path against the base url. This is fine if the url is pointing at an external SVG, but doesn't work if the url is a pure fragment, and the document url and base url are different.
Maybe a related spec issue:
"Need a way to specify base url of pure fragment urls" https://github.com/whatwg/html/issues/1781
I just found that the latest CSS draft has already special cased fragment-only URLs:
https://drafts.csswg.org/css-values-3/#local-urls
>When matching a url() with the local url flag set, ignore everything but the URL’s fragment, and resolve that fragment against the current document that relative URLs are resolved against. This reference must always be treated as same-document (rather than cross-document).
So Firefox should be fixed.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
Attachment #8792781 -
Flags: review?(cam)
Attachment #8792782 -
Flags: review?(cam)
Thanks I see now correctly the css of my web https://bulldog.pro/
|
||
Comment 7•9 years ago
|
||
| mozreview-review | ||
Comment on attachment 8792781 [details]
Bug 1302779 - Part 1. Resolve a local fragment against the current document that relative URLs are resolved against.
https://reviewboard.mozilla.org/r/79696/#review78692
I think there is a problem with the <use> shadow tree handling here. Let me know if I'm wrong (maybe content->GetBaseURI() is returning the right thing for us to use here).
::: layout/svg/nsSVGEffects.cpp:912
(Diff revision 1)
> // content is in a shadow tree.
> // Depending on where this url comes from, choose either the baseURI of the
> // original document of content or the root document of the shadow tree
> // to resolve URI.
Can we expand this comment? Reading it again, now, I think it should be clearer about what we're doing. Maybe something like this?
// content is in a shadow tree. If this URL was specified in the subtree
// referenced by the <use> element, and that subtree came from a separate
// resource document, then we want the fragment-only URL to resolve to
// an element from the resource document. Otherwise, the URL was specified
// somewhere in the document with the <use> element, and we want the
// fragment-only URL to resolve to an element in that document.
::: layout/svg/nsSVGEffects.cpp:916
(Diff revision 1)
> - if (!aFragmentOrURL->EqualsExceptRef(baseURI))
> - baseURI = content->OwnerDoc()->GetBaseURI();
> + nsCOMPtr<nsIURI> originalURI = content->GetBaseURI();
> + if (aFragmentOrURL->EqualsExceptRef(originalURI)) {
> + baseURI = content->GetBaseURI();
> + }
Is using the base URI for the "did this URL come from the resource document" check correct?
It seems that we could "trick" this check by putting an xml:base="..." attribute on the element referenced by the <use>. For example consider this:
-- http://example.com/a/main.svg --
<linearGradient id="g">
<stop stop-color="red"/>
</linearGradient>
<use href="/b/resource.svg#r"/>
-- http://example.com/b/resource.svg --
<linearGradient id="g">
<stop stop-color="green"/>
</linearGradient>
<style>
rect { fill: url(#g); }
</style>
<rect id="r" xml:base="http://somewhere-else.com/" width="100" height="100"/>
aFragmentOrURL->mURI would be http://example.com/b/resource.svg#g and originalURI would be http://somewhere-else.com/. Since they don't match, we'd resolve the FragmentOrURL to the #g element in main.svg, but it should be the one in resource.svg.
We know that resource documents cannot load external style sheets, so we know that any url() value in the resource document must have been resolved against the resource document's base URL. (Maybe that can be affected by an <html:base> in the document, I'm not sure.) So I think we should be setting originalURI to the base URI of the document that content was cloned from.
I think we can get that original content's document's base URI by finding the SVGUseElement that referenced |content| (it's the parent of the root of the shadow tree I think) and then get its mOriginal->OwnerDoc()->GetBaseURI().
For the actual assignment to baseURI if the check succeeds, I think we want to use the original content's document's URI.
Attachment #8792781 -
Flags: review?(cam) → review-
|
|
||
Comment 8•9 years ago
|
||
| mozreview-review | ||
Comment on attachment 8792782 [details]
Bug 1302779 - Part 2. Test case.
https://reviewboard.mozilla.org/r/79698/#review78698
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1-ref.html:6
(Diff revision 1)
> +<!DOCTYPE HTML>
> +
> +<html>
> + <head>
> + <meta charset="utf-8">
> + <title>Reference for clipp-path linked to local-ref URL</title>
s/clipp-path/clip-path/
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:2
(Diff revision 1)
> +<!DOCTYPE HTML>
> +
Nit: drop this blank line. (And in the reference file.)
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:6
(Diff revision 1)
> +<!DOCTYPE HTML>
> +
> +<html>
> + <head>
> + <head>
> + <base href="http://example.com" >
Nite: drop the space before the ">".
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:13
(Diff revision 1)
> + <title>Testcase for clip-path linked to local-ref URL</title>
> + <link rel="author" title="CJ Ku" href="mailto:cku@mozilla.com">
> + <link rel="author" title="Mozilla" href="https://www.mozilla.org">
> + <link rel="help" href="https://www.w3.org/TR/css-masking-1/#the-clip-path">
> + <link rel="match" href="clip-path-localRef-1-ref.html">
> + <meta name="assert" content="Test checks after changing base URL, whether fragement URLs works correctly or not.">
s/fragement/fragment/
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:21
(Diff revision 1)
> + <clipPath id="c1">
> + <circle cy="110" cx="137" r="90" />
> + </clipPath>
> + </defs>
> + </svg>
> + <style type="text/css">
Nit: type="text/css" is the default (and no other type is supported) so please drop it.
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:23
(Diff revision 1)
> + width:300px;
> + height:300px;
Nit: put spaces after the ":"s to keep the formatting consistent with the other declarations. (And in the reference file.)
::: layout/reftests/w3c-css/submitted/masking/clip-path-localRef-1.html:26
(Diff revision 1)
> + <style type="text/css">
> + div {
> + width:300px;
> + height:300px;
> + background-color: blue;
> + font-size: 40px;
Does this font-size declaration do anything in this test? If not, please drop it.
Attachment #8792782 -
Flags: review?(cam) → review+
|
Assignee | |
Comment 10•9 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8792781 [details]
Bug 1302779 - Part 1. Resolve a local fragment against the current document that relative URLs are resolved against.
https://reviewboard.mozilla.org/r/79696/#review78692
> Is using the base URI for the "did this URL come from the resource document" check correct?
>
> It seems that we could "trick" this check by putting an xml:base="..." attribute on the element referenced by the <use>. For example consider this:
>
> -- http://example.com/a/main.svg --
> <linearGradient id="g">
> <stop stop-color="red"/>
> </linearGradient>
> <use href="/b/resource.svg#r"/>
>
> -- http://example.com/b/resource.svg --
> <linearGradient id="g">
> <stop stop-color="green"/>
> </linearGradient>
> <style>
> rect { fill: url(#g); }
> </style>
> <rect id="r" xml:base="http://somewhere-else.com/" width="100" height="100"/>
>
> aFragmentOrURL->mURI would be http://example.com/b/resource.svg#g and originalURI would be http://somewhere-else.com/. Since they don't match, we'd resolve the FragmentOrURL to the #g element in main.svg, but it should be the one in resource.svg.
>
> We know that resource documents cannot load external style sheets, so we know that any url() value in the resource document must have been resolved against the resource document's base URL. (Maybe that can be affected by an <html:base> in the document, I'm not sure.) So I think we should be setting originalURI to the base URI of the document that content was cloned from.
>
> I think we can get that original content's document's base URI by finding the SVGUseElement that referenced |content| (it's the parent of the root of the shadow tree I think) and then get its mOriginal->OwnerDoc()->GetBaseURI().
>
> For the actual assignment to baseURI if the check succeeds, I think we want to use the original content's document's URI.
Unfortunately, SVGUseElement::mOriginal is not the "original" element in the referenced external document.
Except SVG use, we still have to take care of XBL binding element.
But I understand what's your concern, and I will rework this patch
|
|
Assignee | |
Comment 11•9 years ago
|
||
|
|
Assignee | |
Comment 12•9 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 15•9 years ago
|
||
| mozreview-review | ||
Comment on attachment 8792781 [details]
Bug 1302779 - Part 1. Resolve a local fragment against the current document that relative URLs are resolved against.
https://reviewboard.mozilla.org/r/79696/#review80894
Can you also add a test that ensures that fragment-only URLs with XBL bindings work correctly?
::: dom/svg/SVGUseElement.h:79
(Diff revision 2)
> already_AddRefed<SVGAnimatedLength> X();
> already_AddRefed<SVGAnimatedLength> Y();
> already_AddRefed<SVGAnimatedLength> Width();
> already_AddRefed<SVGAnimatedLength> Height();
>
> + already_AddRefed<nsIURI> GetSourceDocURI();
FWIW I think it would be fine to just return |nsIURI*| here, just like nsIDocument::GetDocumentURI does.
::: dom/xbl/nsXBLBinding.cpp:437
(Diff revision 2)
> + nsIContent* targetContent =
> + mPrototypeBinding->GetImmediateChild(nsGkAtoms::content);
I guess we probably won't end up calling nsXBLBinding::GetSourceDocURI() if the binding didn't have a <content> element, but I think we should make this handle not having one, and return null if so.
::: layout/svg/nsSVGEffects.cpp:927
(Diff revision 2)
> + MOZ_ASSERT(binding,
> + "An anonymous tree which is not under a use or "
> + "-moz-binding element?");
We can have content in an anonymous subtree that is not (a) an SVG <use> element subtree, or (b) anonymous content from an XBL binding. Native anonymous content for HTML form elements, for example. But that content isn't considered to come from a separate document (and that content probably wouldn't be defining resource elements to reference with IDs anyway). So in that case leaving baseURI as it is should be fine.
I think we should adjust the assertion:
if (binding) {
originalURI = ...;
} else {
MOZ_ASSERT(content->IsInNativeAnonymousSubtree(),
"an non-native anonymous tree which is not from "
"an XBL binding?");
}
Attachment #8792781 -
Flags: review?(cam) → review-
|
|
||
Comment 16•9 years ago
|
||
| mozreview-review | ||
Comment on attachment 8792781 [details]
Bug 1302779 - Part 1. Resolve a local fragment against the current document that relative URLs are resolved against.
https://reviewboard.mozilla.org/r/79696/#review80900
Attachment #8792781 -
Flags: review- → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 19•9 years ago
|
||
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6856e71c0ee
Part 1. Resolve a local fragment against the current document that relative URLs are resolved against. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1ec196a69c0d
Part 2. Test case. r=heycam
|
||
Comment 20•9 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b6856e71c0ee
https://hg.mozilla.org/mozilla-central/rev/1ec196a69c0d
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•