Closed
Bug 1289011
Opened 9 years ago
Closed 8 years ago
Handle clip-path: fill-box | stroke-box | view-box correctly
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(10 files, 1 obsolete file)
1.10 KB,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
In current implementation, we did not handle fill-box and stroke-box correctly. In [1], they are treated as border-box.
[1] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsCSSClipPathInstance.cpp#81
Updated•9 years ago
|
Blocks: basic-shape-ship
fill-box: calls nsIFrame::GetVisualOverflowRectRelativeToSelf()?
stroke-box: https://drafts.fxtf.org/css-masking-1/#compute-stroke-bounding-box
Comment hidden (mozreview-request) |
Summary: Handle clip-path: fill-box | stroke-box correctly → Handle clip-path: fill-box | stroke-box | view-box correctly
CSS masking spec said:
For SVG elements without associated CSS layout box, the used value for content-box, padding-box, border-box and margin-box is fill-box.
For elements with associated CSS layout box, the used value for fill-box, stroke-box and view-box is border-box.
I will fix this TODO in this bug as well
http://searchfox.org/mozilla-central/source/layout/svg/nsCSSClipPathInstance.cpp#69
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8785234 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8786005 -
Flags: review?(cam)
Attachment #8786006 -
Flags: review?(cam)
Attachment #8786007 -
Flags: review?(cam)
Attachment #8786008 -
Flags: review?(cam)
Attachment #8786009 -
Flags: review?(cam)
Attachment #8786010 -
Flags: review?(cam)
Attachment #8786005 -
Flags: review?(cam)
Attachment #8786006 -
Flags: review?(cam)
Attachment #8786007 -
Flags: review?(cam)
Attachment #8786008 -
Flags: review?(cam)
Attachment #8786009 -
Flags: review?(cam)
Attachment #8786010 -
Flags: review?(cam)
Assignee | ||
Comment 13•8 years ago
|
||
The way of view-box computing is not correct. Repatching.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8786005 -
Flags: review?(cam)
Attachment #8786006 -
Flags: review?(cam)
Attachment #8786007 -
Flags: review?(cam)
Attachment #8786008 -
Flags: review?(cam)
Attachment #8786009 -
Flags: review?(cam)
Attachment #8786010 -
Flags: review?(cam)
Attachment #8786185 -
Flags: review?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8786007 [details]
Bug 1289011 - Part 3. Handle view-box.
https://reviewboard.mozilla.org/r/74996/#review73556
::: dom/svg/SVGSVGElement.h:291
(Diff revision 2)
> using nsINode::GetElementById; // This does what we want
> already_AddRefed<SVGAnimatedRect> ViewBox();
> already_AddRefed<DOMSVGAnimatedPreserveAspectRatio> PreserveAspectRatio();
> uint16_t ZoomAndPan();
> void SetZoomAndPan(uint16_t aZoomAndPan, ErrorResult& rv);
> + virtual nsSVGViewBox *GetViewBox() override;
Nit: While you're here, please associate \* with the type, i.e. nsSVGViewBox\* GetViewBox().
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8786005 [details]
Bug 1289011 - Part 1. Implement ComputeHTMLReferenceRect.
https://reviewboard.mozilla.org/r/74992/#review73888
Attachment #8786005 -
Flags: review?(cam) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8786005 [details]
Bug 1289011 - Part 1. Implement ComputeHTMLReferenceRect.
https://reviewboard.mozilla.org/r/74992/#review73894
::: layout/svg/nsCSSClipPathInstance.h:52
(Diff revision 1)
>
> already_AddRefed<Path> CreateClipPathInset(DrawTarget* aDrawTarget,
> const nsRect& aRefBox);
> +
> +
> + nsRect ComputeHTMLReferenceRect(DrawTarget* aDrawTarget);
Do we need this aDrawTarget argument? We don't seem to use it here, or in the later patches.
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8786006 [details]
Bug 1289011 - Part 2. Implement ComputeSVGReferenceRect.
https://reviewboard.mozilla.org/r/74994/#review73890
::: layout/svg/nsCSSClipPathInstance.cpp:82
(Diff revision 1)
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Content ||
> +
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Padding ||
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Margin ||
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Fill);
> + r = mTargetFrame->GetVisualOverflowRectRelativeToParent();
Nit: I would add a "break;" to the end of this case (and in ComputeHTMLReferenceRect too) so that we don't accidentally fall through if we add another case after it.
::: layout/svg/nsCSSClipPathInstance.cpp:105
(Diff revision 1)
> default: // Use the border box
> + MOZ_ASSERT(
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::NoBox ||
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Border ||
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Fill ||
> +
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::Stroke ||
> + mClipPathStyle.GetReferenceBox() == StyleClipPathGeometryBox::View);
> r = mTargetFrame->GetRectRelativeToSelf();
> }
You could move these value checks into cases, if you want this to be a bit more concise. Like:
default:
MOZ_ASSERT_UNREACHABLE("unknown StyleClipPathGeometryBox type");
// fall through
case StyleClipPathGeometryBox::NoBox:
case StyleClipPathGeometryBox::Border:
...
case StyleClipPathGeometryBox::View:
r = mTargetFrame->GetRectRelativeToSelf();
break;
Similarly for ComputeSVGReferenceRect.
::: layout/svg/nsCSSClipPathInstance.cpp:122
(Diff revision 1)
> - nsRect r = ComputeHTMLReferenceRect(aDrawTarget);
> + nsRect r = mTargetFrame->GetContent()->IsSVGElement()
> + ? ComputeSVGReferenceRect(aDrawTarget)
> + : ComputeHTMLReferenceRect(aDrawTarget);
I think using IsSVGElement() isn't exactly the right thing to test, because that will return true for an outer <svg> element, but such elements do have a an associated CSS layout box. (The nsSVGOuterSVGFrame participates in the CSS layout of its parent, and can have borders, margin and padding set on it. Even in SVG documents where this <svg> element is the root.)
So I think instead we can use:
mTargetFrame->IsFrameOfType(nsIFrame::eSVG) &&
!mTargetFrame->GetType() == nsGkAtoms::svgOuterSVGFrame
Some other interesting cases that are important to note:
* <foreignObject> doesn't have an associated CSS layout box, although it does have CSS layout descendants, so we are right to use ComputeSVGReferenceRect for it.
* text content children (<tspan> and <tref>), which get nsInlineFrames (which are not SVG frames), however clip-path is defined not to apply to text content children anyway, so we shouldn't get in here for them
Attachment #8786006 -
Flags: review?(cam) → review+
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8786007 [details]
Bug 1289011 - Part 3. Handle view-box.
https://reviewboard.mozilla.org/r/74996/#review73896
r=me with these comments addressed.
::: layout/svg/nsCSSClipPathInstance.cpp:73
(Diff revision 2)
> + case StyleClipPathGeometryBox::View:
> + {
Nit: the style guide says to put the brace for a case statement on the same line as the case, and for the final break to be within that brace.
::: layout/svg/nsCSSClipPathInstance.cpp:75
(Diff revision 2)
> + nsIContent* content = mTargetFrame->GetContent();
> + for (; content; content = content->GetParent()) {
> + if (content->IsSVGElement(nsGkAtoms::svg)) {
> + break;
> + }
> + }
For this to work with elements inside a <use> shadow tree, we probably want to use GetFlattenedTreeParent().
But maybe use nsSVGElement::GetCtx() instead, which will do this for you. You'll need to cast content to nsSVGElement*, which is safe only if content->IsSVGElement(), but you can assert that since we checked this when deciding to call ComputeSVGReferenceRect.
::: layout/svg/nsCSSClipPathInstance.cpp:90
(Diff revision 2)
> + nsSVGViewBox* viewBox = svgElement->GetViewBox();
> + const nsSVGViewBoxRect& value = viewBox->GetBaseValue();
We should use the animated viewBox="" value. Please ensure you have a test for an animated viewBox="" affecting clip-path.
::: layout/svg/nsCSSClipPathInstance.cpp:92
(Diff revision 2)
> + r = nsRect(nsPresContext::CSSPixelsToAppUnits(value.x),
> + nsPresContext::CSSPixelsToAppUnits(value.y),
> + nsPresContext::CSSPixelsToAppUnits(value.width),
> + nsPresContext::CSSPixelsToAppUnits(value.height));
Is it correct that we use the x and y of the viewBox here? I thought that the rectangle we need to generate here must be in the same coordinate system as mTargetFrame, and that (0, 0) in that coordinate system is the top-left corner of the viewBox.
Can you add a test with a viewBox that has a non-zero origin, to make sure?
::: layout/svg/nsCSSClipPathInstance.cpp:97
(Diff revision 2)
> + // No viewBox is specified, uses the nearest SVG viewport as reference
> + // box.
It looks like the spec doesn't say what to do when there is no viewBox="" attribute specified, but I guess this is the sensible thing to do. Can you file an issue on the spec about this?
Attachment #8786007 -
Flags: review?(cam) → review+
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8786008 [details]
Bug 1289011 - Part 4. Handle stroke-box.
https://reviewboard.mozilla.org/r/74998/#review73936
::: layout/svg/nsCSSClipPathInstance.cpp:74
(Diff revision 2)
> + // XXXCJ In css-masking spec
> + // https://drafts.fxtf.org/css-masking-1/#stroke-bounding-box
> + // There is no clear definition of how to compute stoke-box for
> + // 1. a graphics element without <use> or <image>
> + // 2. a container element
I think you should read the spec to mean:
* "graphics element without <use> or <image>" is handled the same way as "an <a> element with a text content element"
* "a container element" is handled the same way as "<use>"
i.e., just like "case" statements in a "switch".
Also, note that the wording there about "graphics element without <use> or <image>" means "any graphics element, except <use> or <image>".
I believe this section was added to the Masking spec before the SVG 2 spec got a definition for stroke bounding box. So for a more complete definition you can see:
https://svgwg.org/svg2-draft/coords.html#BoundingBoxes
The result of that is that you should do something with nsSVGUtils::GetBBox(nsSVGUtils::eBBoxIncludeFill | nsSVGUtils::eBBoxIncludeStroke). See SVGTransformableElement::GetBBox, which is the implementation of the DOM getBBox() method.
Attachment #8786008 -
Flags: review?(cam) → review-
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8786009 [details]
Bug 1289011 - Part 5. shape-box reftest for elements with associated CSS layout box.
https://reviewboard.mozilla.org/r/75000/#review73946
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-1a.html:14
(Diff revision 4)
> + position: absolute;
> + left: 10px;
> + top: 10px;
> + width: 100px;
> + height: 100px;
> + padding: 40px;
> + clip-path: circle(farthest-side) content-box;
I'm a bit confused. If I understand correctly, the <div> should have:
border/padding box: (10,10) -> (110,110)
content box: (50,50) -> (70,70)
and then the basic shape, "circle(farthest-side) content-box" should define a circle with radius 10 positioned at the centre of the content box, i.e. at (60,60).
But the reference file has a circle of radius 50 positioned at (100,100).
What am I missing?
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-1a.html:25
(Diff revision 4)
> + clip-path: circle(farthest-side) content-box;
> + }
> + </style>
> + </head>
> + <body>
> + <div/>
Self-closing tags like this are not supported in HTML syntax (except for SVG tags, I believe). So if you had another element before the </body> it would become a child of the <div>, not a sibling. So, write this as <div></div>.
Assignee | ||
Comment 29•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786007 [details]
Bug 1289011 - Part 3. Handle view-box.
https://reviewboard.mozilla.org/r/74996/#review73896
> We should use the animated viewBox="" value. Please ensure you have a test for an animated viewBox="" affecting clip-path.
Bug 1299760 filed.
> Is it correct that we use the x and y of the viewBox here? I thought that the rectangle we need to generate here must be in the same coordinate system as mTargetFrame, and that (0, 0) in that coordinate system is the top-left corner of the viewBox.
>
> Can you add a test with a viewBox that has a non-zero origin, to make sure?
Spec said:
The reference box is positioned at the origin of the coordinate system established by the ‘viewBox‘ attribute.
(value.x, value.y) is the offset of viewbox in mTargetFrame coordination, so this offset is required.
And yes, I do have a test case to verify it(clip-path-geometryBox-1i.html), I will update a version with non-zero origin later.
> It looks like the spec doesn't say what to do when there is no viewBox="" attribute specified, but I guess this is the sensible thing to do. Can you file an issue on the spec about this?
Spec says:
Uses the nearest SVG viewport as reference box.
Assignee | ||
Comment 30•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786008 [details]
Bug 1289011 - Part 4. Handle stroke-box.
https://reviewboard.mozilla.org/r/74998/#review73936
> I think you should read the spec to mean:
>
> * "graphics element without <use> or <image>" is handled the same way as "an <a> element with a text content element"
>
> * "a container element" is handled the same way as "<use>"
>
> i.e., just like "case" statements in a "switch".
>
> Also, note that the wording there about "graphics element without <use> or <image>" means "any graphics element, except <use> or <image>".
>
>
> I believe this section was added to the Masking spec before the SVG 2 spec got a definition for stroke bounding box. So for a more complete definition you can see:
>
> https://svgwg.org/svg2-draft/coords.html#BoundingBoxes
>
> The result of that is that you should do something with nsSVGUtils::GetBBox(nsSVGUtils::eBBoxIncludeFill | nsSVGUtils::eBBoxIncludeStroke). See SVGTransformableElement::GetBBox, which is the implementation of the DOM getBBox() method.
Arr, I see(like switch). Will update a new version.
Assignee | ||
Comment 31•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786009 [details]
Bug 1289011 - Part 5. shape-box reftest for elements with associated CSS layout box.
https://reviewboard.mozilla.org/r/75000/#review73946
> I'm a bit confused. If I understand correctly, the <div> should have:
>
> border/padding box: (10,10) -> (110,110)
> content box: (50,50) -> (70,70)
>
> and then the basic shape, "circle(farthest-side) content-box" should define a circle with radius 10 positioned at the centre of the content box, i.e. at (60,60).
>
> But the reference file has a circle of radius 50 positioned at (100,100).
>
> What am I missing?
border/padding box: (x=10,y=10) -> (w=180,h=180)
content box: (x=50,y=50) -> (w=100,h=100)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 40•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786009 [details]
Bug 1289011 - Part 5. shape-box reftest for elements with associated CSS layout box.
https://reviewboard.mozilla.org/r/75000/#review73946
> border/padding box: (x=10,y=10) -> (w=180,h=180)
> content box: (x=50,y=50) -> (w=100,h=100)
Ah, I overlooked the fact that the width and height properties were exclusive of the padding.
Assignee | ||
Comment 41•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786008 [details]
Bug 1289011 - Part 4. Handle stroke-box.
https://reviewboard.mozilla.org/r/74998/#review73936
> Arr, I see(like switch). Will update a new version.
I filed another bug(Bug 1299876) to keep tracing this issue. nsSVGUtils::GetBBox works fine except two special cases.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8787220 -
Flags: review?(cam)
Attachment #8787311 -
Flags: review?(cam)
Comment 50•8 years ago
|
||
mozreview-review |
Comment on attachment 8786008 [details]
Bug 1289011 - Part 4. Handle stroke-box.
https://reviewboard.mozilla.org/r/74998/#review74308
::: layout/svg/nsCSSClipPathInstance.cpp:76
(Diff revision 4)
> // For SVG elements without associated CSS layout box, the used value for
> // content-box, padding-box, border-box and margin-box is fill-box.
> switch (mClipPathStyle.GetReferenceBox()) {
> + case StyleClipPathGeometryBox::Stroke: {
> + // XXX Bug 1299876
> + // The size of stoke-box is not correct if this graphic element has
"stroke-box"
::: layout/svg/nsCSSClipPathInstance.cpp:78
(Diff revision 4)
> switch (mClipPathStyle.GetReferenceBox()) {
> + case StyleClipPathGeometryBox::Stroke: {
> + // XXX Bug 1299876
> + // The size of stoke-box is not correct if this graphic element has
> + // specific stroke-linejoin or stroke-linecap.
> + gfxRect bbox = nsSVGUtils::GetBBox(mTargetFrame ,
Nit: drop the space before the comma.
Attachment #8786008 -
Flags: review?(cam) → review+
Comment 51•8 years ago
|
||
mozreview-review |
Comment on attachment 8786009 [details]
Bug 1289011 - Part 5. shape-box reftest for elements with associated CSS layout box.
https://reviewboard.mozilla.org/r/75000/#review74314
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-2a.html:20
(Diff revision 6)
> + top: 0px;
> + background-color: blue;
> + width: 100px;
> + height: 100px;
> + padding: 50px;
> + clip-path: polygon(0% 50%, 50% 0%, 100% 50%, 100% 100%, 0% 100%) content-box;
Nit: remove one space before "content-box".
For manual verification that this matches the reference, it would be nice if the order of the points matched the reference, i.e.:
polygon(50% 0%, 100% 50%, 100% 100%, 0% 100%, 0% 50%)
(Same for the other polygon-based test files.)
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-2b.html:10
(Diff revision 6)
> + <title>CSS Masking: clip-path: clip path border-box</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-geometryBox-2-ref.html">
> + <meta name="assert" content="Test checks whether clip-path border-box works correctly or not.">
padding-box
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-2c.html:10
(Diff revision 6)
> + <title>CSS Masking: clip-path: clip path padding-box</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-geometryBox-2-ref.html">
> + <meta name="assert" content="Test checks whether clip-path padding-box works correctly or not.">
border-box
Attachment #8786009 -
Flags: review?(cam) → review+
Comment 52•8 years ago
|
||
mozreview-review |
Comment on attachment 8786010 [details]
Bug 1289011 - Part 6. shape-box reftest for SVG elements without associated CSS layout box.
https://reviewboard.mozilla.org/r/75002/#review74316
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-1d.html:10
(Diff revision 6)
> + <title>CSS Masking: clip-path: clip path content-box</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-geometryBox-1-ref.html">
> + <meta name="assert" content="Test checks whether clip-path content-box works correctly or not.">
Mention in the <meta name=assert> that this is for clip-path applied to an SVG element. (And maybe mention in the previous tests that they are testing clip-path applied to an HTML element.)
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-1d.html:14
(Diff revision 6)
> + <rect x="50" y="50" width="100" height="100" fill="blue"
> + clip-path="circle(50%) content-box">
Kind of inverse of a previous comment: no SVG elements are "empty" tags, where you don't need to provide a closing tag (like HTML's <img>). So you should write either <rect ... /> or <rect></rect> here. Same for the other files added in this patch.
I wonder if you might run into problems with anti-aliasing at the edges here? Since the edges of the blue rectangle coincide with the edge of the circle. If so, consider making the rectangle bigger and the circle() radius smaller.
Also, let's add a stroke to the rectangle. That way the test will fail if the browser incorrectly treats content-box like stroke-box. Same for 1e, 1f and 1g.
Attachment #8786010 -
Flags: review?(cam) → review+
Comment 53•8 years ago
|
||
mozreview-review |
Comment on attachment 8786185 [details]
Bug 1289011 - Part 7. view-box reftest.
https://reviewboard.mozilla.org/r/75148/#review74318
Same comments apply about <rect .../>.
r=me if you add a test using a |clip-path: ... view-box| applied to an HTML element.
::: layout/reftests/w3c-css/submitted/masking/clip-path-geometryBox-1i.html:13
(Diff revision 4)
> + <link rel="help" href="https://www.w3.org/TR/css-masking-1/#the-clip-path">
> + <link rel="match" href="clip-path-geometryBox-1-ref.html">
> + <meta name="assert" content="Test checks whether clip-path view-box with viewbox works correctly or not.">
> + </head>
> + <body>
> + <svg width="200" height="200" viewbox="50 50 100 100" preserveAspectRatio="none" style="position: absolute; left: 10px; top: 10px;">
Nit: The canonical spelling of the attribute is viewBox, not viewbox. (But we're parsing inline SVG in HTML so it will still work.)
Attachment #8786185 -
Flags: review?(cam) → review+
Comment 54•8 years ago
|
||
mozreview-review |
Comment on attachment 8787220 [details]
Bug 1289011 - Part 8. shape-box reftest for SVG outter element.
https://reviewboard.mozilla.org/r/76016/#review74320
Please add some tests for fill-box, stroke-box and view-box when used on an outer SVG element to check that they are treated like border-box.
Attachment #8787220 -
Flags: review?(cam) → review+
Comment 55•8 years ago
|
||
mozreview-review |
Comment on attachment 8787311 [details]
Bug 1289011 - Part 9. stroke-box reftest.
https://reviewboard.mozilla.org/r/76118/#review74322
Attachment #8787311 -
Flags: review?(cam) → review+
Assignee | ||
Comment 56•8 years ago
|
||
Assignee | ||
Comment 57•8 years ago
|
||
Assignee | ||
Comment 58•8 years ago
|
||
Assignee | ||
Comment 59•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 69•8 years ago
|
||
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/900800f54055
Part 1. Implement ComputeHTMLReferenceRect. r=heycam
https://hg.mozilla.org/integration/autoland/rev/34e7dd6acb4c
Part 2. Implement ComputeSVGReferenceRect. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a623cab8e4a7
Part 3. Handle view-box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/43d1ec007a49
Part 4. Handle stroke-box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aa59aca85397
Part 5. shape-box reftest for elements with associated CSS layout box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d1b4d0b38cdb
Part 6. shape-box reftest for SVG elements without associated CSS layout box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4b8d3be7bdf8
Part 7. view-box reftest. r=heycam
https://hg.mozilla.org/integration/autoland/rev/03572962add4
Part 8. shape-box reftest for SVG outter element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/dea9d3f8bfe0
Part 9. stroke-box reftest. r=heycam
Comment 70•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/1bee4f0f5239 for a bunch of clip-path reftest failures on Win8, https://treeherder.mozilla.org/logviewer.html#?job_id=3006711&repo=autoland
Assignee | ||
Comment 71•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 81•8 years ago
|
||
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/632cf28211de
Part 1. Implement ComputeHTMLReferenceRect. r=heycam
https://hg.mozilla.org/integration/autoland/rev/43daa18916ce
Part 2. Implement ComputeSVGReferenceRect. r=heycam
https://hg.mozilla.org/integration/autoland/rev/062cae507676
Part 3. Handle view-box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8e3fc6d5237f
Part 4. Handle stroke-box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fd404df10eed
Part 5. shape-box reftest for elements with associated CSS layout box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b3da5e487c2c
Part 6. shape-box reftest for SVG elements without associated CSS layout box. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3f1f0bc8c878
Part 7. view-box reftest. r=heycam
https://hg.mozilla.org/integration/autoland/rev/869997798534
Part 8. shape-box reftest for SVG outter element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/296f8c0b77e1
Part 9. stroke-box reftest. r=heycam
Comment 82•8 years ago
|
||
Well, at least now you might be within 1 pixel of success.
Backed out in https://hg.mozilla.org/integration/autoland/rev/d61bbdd0b155 for Android failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3025722&repo=autoland
Assignee | ||
Comment 83•8 years ago
|
||
failures on Win8, https://treeherder.mozilla.org/logviewer.html#?job_id=3006711&repo=autoland
Android failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3025722&repo=autoland
What I change between this two check-in is add fuzz prefix in reftest.list. Android test case works correctly in the first check-in... BTW, I will figure it out.
Assignee | ||
Comment 84•8 years ago
|
||
Assignee | ||
Comment 85•8 years ago
|
||
Assignee | ||
Comment 86•8 years ago
|
||
Assignee | ||
Comment 87•8 years ago
|
||
Assignee | ||
Comment 88•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/664a9cffbf1a07d1382c305aaa4c7dc3c339f0d9
Bug 1289011 - Part 1. Implement ComputeHTMLReferenceRect. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/11871df86ff8f9972770e46d91b6115ed6338a13
Bug 1289011 - Part 2. Implement ComputeSVGReferenceRect. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eacb8a7c6cfd89b18c82bde40b5f00db19bb9ef
Bug 1289011 - Part 3. Handle view-box. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b803d96ab1a52fc23dc3c150fed268803ebb5cc0
Bug 1289011 - Part 4. Handle stroke-box. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/72136b9b75329a08c5b209b412ffc5b5779e902f
Bug 1289011 - Part 5. refetst for reference box. r=heycam
Comment 89•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/664a9cffbf1a
https://hg.mozilla.org/mozilla-central/rev/11871df86ff8
https://hg.mozilla.org/mozilla-central/rev/8eacb8a7c6cf
https://hg.mozilla.org/mozilla-central/rev/b803d96ab1a5
https://hg.mozilla.org/mozilla-central/rev/72136b9b7532
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 90•8 years ago
|
||
Updated https://developer.mozilla.org/en-US/docs/Web/CSS/clip-path
and
https://developer.mozilla.org/en-US/Firefox/Releases/51
Keywords: dev-doc-needed → dev-doc-complete
Blocks: basic-shape
You need to log in
before you can comment on or make changes to this bug.
Description
•