Closed
Bug 1299715
Opened 8 years ago
Closed 8 years ago
Replace ContainerItemType::eSVGEffects with eMask and eFilter.
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
(Blocks 1 open bug)
Details
Attachments
(11 files)
462 bytes,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
856 bytes,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
According to bug 1295094 comment 29:
1. you should also change the ContainerItemType enum and replace eSVGEffects with eMask and eFilter.
2. With this split, we can even make clip capturing more correct than it was before: only filters should capture clips, masks / clip paths should not. They should instead just pass clips through (the way opacity does it)
3. Create test cases for this new behavior.
here's what the result of that change will be:
<div style="width:100px; height:100px; overflow:hidden">
<div style="clip-path: url(#circleWithWidthAndHeight200)">
<div style="position:absolute;top:0;left:0;width:400px;height:400px;">
<!-- This should only be clipped by the circle clip-path, not by the 100x100 overflow:hidden clip. At the moment it's clipped by both. -->
</div>
</div>
</div>
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8798135 -
Flags: review?(mstange)
Attachment #8798136 -
Flags: review?(mstange)
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8798135 [details]
Bug 1299715 - Part 1. Replace ContainerItemType::eSVGEffects by ContainerItemType::eFilter.
https://reviewboard.mozilla.org/r/83680/#review82258
::: layout/base/nsDisplayList.cpp:6976
(Diff revision 1)
> #endif
>
> nsDisplayFilter::nsDisplayFilter(nsDisplayListBuilder* aBuilder,
> nsIFrame* aFrame, nsDisplayList* aList,
> bool aHandleOpacity)
> - : nsDisplaySVGEffects(aBuilder, aFrame, aList, aHandleOpacity)
> + : nsDisplaySVGEffects(aBuilder, aFrame, aList, aHandleOpacity, nullptr)
Instead of nullptr, this needs to be aBuilder->ClipState().GetCurrentInnermostScrollClip(). Or, alternatively, you could add a four-argument constructor to nsDisplaySVGEffects which calls the three-argument constructor of nsDisplayWrapList, which already does this correctly.
Sorry about the confusing constructors.
::: layout/generic/nsFrame.cpp:2231
(Diff revision 1)
> - eSVGEffects,
> - eBlendContainer
> + eBlendContainer,
> + eFilter
Please reverse the order here - blend container should go last, as a documentation that it's always going to be the outermost container item.
Attachment #8798135 -
Flags: review?(mstange) → review+
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8798136 [details]
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element.
https://reviewboard.mozilla.org/r/83682/#review82260
::: layout/reftests/svg/reftest.list:448
(Diff revision 3)
> == use-localRef-stroke-01.svg use-localRef-stroke-01-ref.svg
> == use-localRef-mask-01.svg use-localRef-mask-01-ref.svg
>
> fuzzy(1,5000) == mask-opacity-01.svg mask-opacity-01-ref.svg
> +
> +fuzzy(66,729) == clipPath-and-fixedPosition-01a.html clipPath-and-fixedPosition-01-ref.html
66 is extremely high. Could you use a pixel-aligned shape instead of a circle? For example a union of two or three rectangles.
Attachment #8798136 -
Flags: review?(mstange)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8798136 [details]
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element.
https://reviewboard.mozilla.org/r/83682/#review82262
::: layout/reftests/svg/clipPath-and-fixedPosition-01a.html:29
(Diff revision 3)
> +</style>
> +
> +<body>
> + <div id="outer">
> + <div id="clipped">
> + <div id="fixedPosition">
maybe call these absolutePosition because they're position:absolute
Assignee | ||
Comment 11•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8798135 [details]
Bug 1299715 - Part 1. Replace ContainerItemType::eSVGEffects by ContainerItemType::eFilter.
https://reviewboard.mozilla.org/r/83680/#review82258
> Please reverse the order here - blend container should go last, as a documentation that it's always going to be the outermost container item.
Sure.
Do you know why we define the following enum value but never use them?
eOwnLayerIfNeeded
eBlendMode
eSeparatorTransforms
eOpacity
eBlendContainer
Should we just remove them?
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8798372 [details]
Bug 1299715 - Part 2. Correct the value of nsDisplayMask::mVisibleRect.
https://reviewboard.mozilla.org/r/83890/#review82432
::: layout/base/nsDisplayList.cpp:6921
(Diff revision 1)
> + if (style->HasClipPath()) {
> + // Clip path is not bounded by mFrame rectangle.
> + // All descendants, outflow elements(postion is absolute, for example)
> + // included, of mFrame should be clipped by clip-path.
> + mVisibleRect = mList.GetBounds(aBuilder);
> + }
This patch is to fix the early return of a nsDisplayMask item with empty size
http://searchfox.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#5655
Without the fix, clipPath-on-outflowElement-01b.html(in Part 4) will draw nothing, since the size of <div id="clipped"> is empty
Assignee | ||
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8798371 [details]
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface.
https://reviewboard.mozilla.org/r/83888/#review82434
::: layout/svg/nsSVGIntegrationUtils.cpp:861
(Diff revision 1)
> RefPtr<SourceSurface> clipMaskSurface =
> clipPathFrame->GetClipMask(context, frame, cssPxToDevPxMatrix,
> &clippedMaskTransform, maskSurface,
> maskTransform, &result);
> - context.PopClip();
>
Skip clipping by visual-rect of mFrame, otherwise, you will see only two rectangles in clipPath-outflowElement-01a.html.
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8798371 -
Flags: review?(mstange)
Attachment #8798372 -
Flags: review?(mstange)
Comment 30•8 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #14)
> Do you know why we define the following enum value but never use them?
> eOwnLayerIfNeeded
> eBlendMode
> eSeparatorTransforms
> eOpacity
> eBlendContainer
For documentation. It's a list of all the possible container item types we can create in that function. I suppose we could turn the unneeded lines into comments, but to me it seems nicer the way it is.
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8798371 [details]
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface.
https://reviewboard.mozilla.org/r/83888/#review82444
Is the visual overflow rect on the frame wrong? Or is this how things are supposed to work? (I don't actually know.) Does the same happen if you use e.g. opacity instead of clip-path?
Attachment #8798371 -
Flags: review?(mstange)
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8798136 [details]
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element.
https://reviewboard.mozilla.org/r/83682/#review82446
Could you add a test for mask as well? And maybe even one with mask-image (which is only run when mask-image is enabled).
These tests still use position:absolute on an element with the ID fixedPosition.
Attachment #8798136 -
Flags: review?(mstange)
Assignee | ||
Comment 33•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8798371 [details]
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface.
https://reviewboard.mozilla.org/r/83888/#review82444
Take clipPath-outflowElement-01a.html(in Part 4) as an exameple.
There are three rectangle in <clipPath id="myClip">
<clipPath id="myClip">
<rect x="0" y="0" width="100" height="100"/> /* The first rect */
<rect x="100" y="100" width="100" height="100"/> /* The second rect */
<rect x="200" y="200" width="100" height="100"/> /* The third rect */
</clipPath>
And the size of <div id="clipped"> is set to (200 X 200)
#clipped {
clip-path: url(#myClip);
width:200px;
height:200px;
}
If we don't skip that clip in SetupContextMatrix(clipped by a 200X200 visual-rect), nsSVGClipPathFrame::GetClipMask will generate an (200X200) A8 suface at [1], which means the third rect(at x="200" y="200") in <clipPath id="myClip"> will be clipped out.
PS: Please add -webkit-clip-path: url(#myClip); in #clipped style if you want to see how chrome handle it.
Since nsDisplayOpacity does not need to generate a temp surface, it does not clip context at all.\
Without Part 1, you will see only the first rect in clipPath
Without Part 2, you will see only the first and the second rect in clipPath
[1] http://searchfox.org/mozilla-central/source/layout/svg/nsSVGClipPathFrame.cpp#131
Comment 34•8 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #33)
> If we don't skip that clip in SetupContextMatrix(clipped by a 200X200
> visual-rect)
Right, so we clip to the frame's visual overflow rect here. I'm wondering why the frame's visual overflow rect does not include the out-of-flow descendant, and whether that's normal or something special to masks / clip paths.
> Since nsDisplayOpacity does not need to generate a temp surface, it does not
> clip context at all.
It does not clip, but frames with opacity still have a visual overflow rect. I'm asking whether that visual overflow rect includes the out-of-flow descendant.
Assignee | ||
Comment 35•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #34)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #33)
> > If we don't skip that clip in SetupContextMatrix(clipped by a 200X200
> > visual-rect)
>
> Right, so we clip to the frame's visual overflow rect here. I'm wondering
> why the frame's visual overflow rect does not include the out-of-flow
> descendant, and whether that's normal or something special to masks / clip
> paths.
>
> > Since nsDisplayOpacity does not need to generate a temp surface, it does not
> > clip context at all.
>
> It does not clip, but frames with opacity still have a visual overflow rect.
> I'm asking whether that visual overflow rect includes the out-of-flow
> descendant.
Ahh...I see. Let me check
Assignee | ||
Comment 36•8 years ago
|
||
Assignee | ||
Comment 37•8 years ago
|
||
Hi mstange,
I used attachment 8798524 [details] to verify the opacity visual overflow rect you asked.
1. The visual overflow size of <div id="clipped">'s primary frame is
width = 6000, height = 0 (app unit)
Replace "opacity: 0.5;" by "clip-path: url(#myClip)" get the same result.
It tells us that the region of out-of-flow element, <div id="fixedPosition">, is not counted in visual overflow region of <div id="clipped">.
(By look into code, I don't think visual-overflow rect contains out-of-flow or inflow descendants.)
2. If I don't clip clip-path A8 surface by visual overflow rect, I should not clip mask surface by it too.
3. test cases for mask is needed. I will update a new version which include mask test cases.
Assignee | ||
Comment 38•8 years ago
|
||
Assignee | ||
Comment 39•8 years ago
|
||
Assignee | ||
Comment 40•8 years ago
|
||
Thing we need to deal with here is more complicated then I thought.
From Patch part 4 to part 6, each patch fix one single problem
-----------------------------------------
Part 4 - Fix wrong clipped region of a clip-path on an element contains a out-of-flow element
<div id="outer" style="width:100px; height:100px; overflow:hidden">
<div id="inner"style="clip-path: url(#circleWithRadius200)">
<div id="out-of-flow"
style="position:absolute;top:0;left:0;width:400px;height:400px;">
<!-- This should only be clipped by the circle clip-path, not by the 100x100 overflow:hidden clip. At the moment it's clipped by both. -->
</div>
</div>
</div>
We want to generate a (400X400) mask surface to contains a circle with radius 200.
Here is the comment of GetVisualOverflowRect() in nsIFrame.h:
* Returns a rect that encompasses everything that might be painted by
* this frame. This includes this frame, all its descendent frames, this
* frame's outline, and descentant frames' outline, but does not include
* areas clipped out by the CSS "overflow" and "clip" properties.
It says:
"does not include areas clipped out by the CSS "overflow" and "clip" properties"
Which tell us that the return size of GetVisualOverflowRect() of #inner should be (100X100), in theory. In practice, I found the return size of this function is (100X0). We either have a bug here or the comment is not match current behavior. No matter (100X100) or (100X0), we want none of them.
(PS: We always get (100X0) with or without mask / clip-path)
To create a (400X400) mask surface, we need to skip clipping context by GetVisualOverflowRect.
Without that clipping, we are going to generate a huge A8 surface, which is not good. Fortunately, in Part 3, I already clip the context by nsDisplayMask::mVisibleRect. So the size of A8 surface for is bounded by the dirty rectangle, which is bounded by rectangle of mFrame and _all_ its descendants(include out-of-flow element)
In Part 7, there are two test cases to verify this fix
clipPath-on-outflowElement-01a.html
clipPath-on-outflowElement-01b.html
-----------------------------------------
Part 5 - Fix wrong clipped region of an opacity surface on an element contains a out-of-flow element
<div id="outer" style="width:100px; height:100px; overflow:hidden">
<div id="inner"style="opacity:0.5; clip-path: circle(200 at 200 200)">
<div id="out-of-flow"
style="position:absolute;top:0;left:0;width:400px;height:400px;">
<!-- This should only be clipped by the circle clip-path, not by the 100x100 overflow:hidden clip. At the moment it's clipped by both. -->
</div>
</div>
</div>
Under this circumstance, we will temporary surface at
http://searchfox.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#887
The size of that surface depends on how we clip the source context. Like what I explain in Part 4, we have to replace clipping by visual overflow rectangle by nsDisplayMask::mVisibleRect. The change in this patch is similar to previous one
In Part 7, there are two test cases to verify this fix
clipPath-on-outflowElement-02a.html
clipPath-on-outflowElement-02b.html
-----------------------------------------
Part 6 - Fix wrong clipped region of a mask surface on an element contains a out-of-flow element
<div id="outer" style="width:100px; height:100px; overflow:hidden">
<div id="inner"style="mask: url(#circleWithRadius200)">
<div id="out-of-flow"
style="position:absolute;top:0;left:0;width:400px;height:400px;">
<!-- This should only be clipped by the circle clip-path, not by the 100x100 overflow:hidden clip. At the moment it's clipped by both. -->
</div>
</div>
</div>
This one is even more complicated, since we have two kinds of mask, image-mask and svg-mask.
image-mask should always be clipped by the rectangle of the applied element, we should keep it work as before. In another hand, SVG-mask with maskUnits="userSpaceOnUse" need to be treated differently.
For a svg-mask, we actually does not need to clip context at all, since nsSVGMaskFrame::GetMaskArea returns the size bounded by frame rectangle(if maskUnit is objectBoundary) and determine the size of generated A8 surface.
In Part 7, there are 4 test cases to verify this fix
mask-on-outflowElement-01a.html
mask-on-outflowElement-01b.html
mask-on-outflowElement-01c.html
mask-on-outflowElement-01d.html
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 #8798765 -
Flags: review?(mstange)
Attachment #8798766 -
Flags: review?(mstange)
Attachment #8798767 -
Flags: review?(mstange)
Attachment #8798768 -
Flags: review?(mstange)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 51•8 years ago
|
||
mozreview-review |
Comment on attachment 8798765 [details]
Bug 1299715 - Part 3. Clip the target context by visible region of nsDisplayMask.
https://reviewboard.mozilla.org/r/84176/#review82898
Is this for limiting the size of the intermediate surface (which is based on the context's clip bounds)?
Attachment #8798765 -
Flags: review?(mstange) → review+
Comment 52•8 years ago
|
||
mozreview-review |
Comment on attachment 8798767 [details]
Bug 1299715 - Part 7. Correct clip region for mask surface.
https://reviewboard.mozilla.org/r/84180/#review82900
If we can't do this for image masks, I wonder whether it's even worth doing it for SVG masks...
::: layout/svg/nsSVGIntegrationUtils.cpp:433
(Diff revision 1)
> return mozilla::gfx::Factory::CheckSurfaceSize(result.Size()) ? result
> : IntRect();
> }
>
> +static bool
> +HasNoneSVGMask(const nsTArray<nsSVGMaskFrame *>& aMaskFrames)
Let's call this HasNonSVGMask without the "e", and add a comment that says "Returns true if any of the masks is an image mask (and not an SVG mask)."
::: layout/svg/nsSVGIntegrationUtils.cpp:843
(Diff revision 1)
> + // image mask is bounded inside the applied element, while SVG mask is
> + // not.
Oh? Why? Is that what other browsers do?
Attachment #8798767 -
Flags: review?(mstange)
Comment 53•8 years ago
|
||
mozreview-review |
Comment on attachment 8798768 [details]
Bug 1299715 - Part 8. Fix typo in comment of GetVisualOverflowRect.
https://reviewboard.mozilla.org/r/84182/#review82902
::: layout/generic/nsIFrame.h:2510
(Diff revision 2)
> uint32_t aFlags = 0);
>
> /**
> * Returns a rect that encompasses everything that might be painted by
> - * this frame. This includes this frame, all its descendent frames, this
> - * frame's outline, and descentant frames' outline, but does not include
> + * this frame. This includes this frame, all its descendant frames, this
> + * frame's outline, and descendants frames' outline, but does not include
I think it should be "and descendant frames' outline", without the s.
Attachment #8798768 -
Flags: review?(mstange) → review+
Comment 54•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #51)
> Comment on attachment 8798765 [details]
> Bug 1299715 - Part 3. Clip the target context by visible region of
> nsDisplayMask.
>
> https://reviewboard.mozilla.org/r/84176/#review82898
>
> Is this for limiting the size of the intermediate surface (which is based on
> the context's clip bounds)?
Oh, you answered this question in comment 40. It would be nice to add a comment to the code.
Assignee | ||
Comment 55•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8798767 [details]
Bug 1299715 - Part 7. Correct clip region for mask surface.
https://reviewboard.mozilla.org/r/84180/#review82900
> Oh? Why? Is that what other browsers do?
Only FF suports svg-mask, webkit base browser support image-mask only
Comment 56•8 years ago
|
||
I mean, why are image masks "bounded inside the applied element"?
Assignee | ||
Comment 57•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8798767 [details]
Bug 1299715 - Part 7. Correct clip region for mask surface.
https://reviewboard.mozilla.org/r/84180/#review82900
For a image-mask, it need to respect mask-clip/ mask-position etc, so it has to be clip by the associate frame; SVG mask does not has this limitation.
With this patch, we can let the behavior of svg-mask and clip-path be the same.
Assignee | ||
Comment 58•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #56)
> I mean, why are image masks "bounded inside the applied element"?
Oops... we have a new mask-clip value: no-clip (bug 1303623)
https://drafts.fxtf.org/css-masking-1/#propdef-mask-clip
For a image-mask, this statement("bounded inside the applied element") should be true if mask-clip is not "no-clip"
And according to the spec, I think we did something wrong(not relative to the patches here):
Determines the mask painting area, which determines the area that is affected by the mask. The painted content of an element must be restricted to this area.
when we set "mask-clip: content", the drawing content on the border should not be effected by this mask. Currently, we will just clip out everything outside of mask-clip defined box.
Assignee | ||
Comment 59•8 years ago
|
||
Copy the discussion with mstange on IRC
CJKu> mstange: yes, I think the behavior of image-mask with "mask-clip:no-clip" should be the same with svg-mask.
03:55
<mstange> CJKu: ok
03:55 CJKu: would something go wrong if we used that behavior all the time?
03:56 CJKu: even if the mask surface will be bigger than necessary, mask-clip will make sure that we only fill the right parts of it, I think
03:56 so maybe we should just never clip to the frame's visual overflow rect
03:57 we'll always have the clip from the nsDisplayMask's mVisibleRect as a fallback, and in most cases that will be reasonable
03:59
<CJKu> mstange: even if the mask surface will be bigger than necessary << that's the concern
03:59
<mstange> ok, but that's only a perf concern, not a correctness concern, right?
03:59
<CJKu> mstange: yes
03:59
<mstange> ok, good
04:00 glad we agree so far :-)
04:00 we could make nsDisplayMask detect its image masks, and constrain its mBounds by the mask clips
04:01 Then, during painting, its mVisibleRect will reflect those constraints, and the mask surface will be small
04:01 CJKu: what do you think?
04:04
<CJKu> mstange: ohh, so should I move function like ComputeMaskGeometry into nsDisplayMask?
04:04
<mstange> CJKu: that might be worth doing, yeah
04:04 CJKu: since the display item knows its true bounds
Some rework is needed in Part 7.
Assignee | ||
Comment 60•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8798767 [details]
Bug 1299715 - Part 7. Correct clip region for mask surface.
https://reviewboard.mozilla.org/r/84180/#review82900
> Only FF suports svg-mask, webkit base browser support image-mask only
image mask respects mask-clip; svg mask does not.
Before we support mask-clip:no-clip, mask image is bounded inside css box(margin/border/padding/content box, and margin-box is not supported yet).
Base on our discussion on IRC, this statement is not needed. We clip gfxContext by nsDisplayMask::mVisibleRect, which is an union of all descendants frame rect(out-of-flow descendants included) and intersect with dirty region.
Assignee | ||
Comment 61•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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 74•8 years ago
|
||
mozreview-review |
Comment on attachment 8798372 [details]
Bug 1299715 - Part 2. Correct the value of nsDisplayMask::mVisibleRect.
https://reviewboard.mozilla.org/r/83890/#review83574
Attachment #8798372 -
Flags: review?(mstange) → review+
Comment 75•8 years ago
|
||
mozreview-review |
Comment on attachment 8798765 [details]
Bug 1299715 - Part 3. Clip the target context by visible region of nsDisplayMask.
https://reviewboard.mozilla.org/r/84176/#review83576
::: layout/base/nsDisplayList.cpp:6939
(Diff revision 2)
> + mFrame->PresContext()->AppUnitsPerDevPixel(),
> + *aCtx->GetDrawTarget()));
The indent here looks slightly wrong in mozreview. Not sure if it's actually wrong.
Comment 76•8 years ago
|
||
mozreview-review |
Comment on attachment 8798371 [details]
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface.
https://reviewboard.mozilla.org/r/83888/#review83578
Attachment #8798371 -
Flags: review?(mstange) → review+
Comment 77•8 years ago
|
||
mozreview-review |
Comment on attachment 8798766 [details]
Bug 1299715 - Part 5. Correct clip region for opacity surface.
https://reviewboard.mozilla.org/r/84178/#review83580
Attachment #8798766 -
Flags: review?(mstange) → review+
Comment 78•8 years ago
|
||
mozreview-review |
Comment on attachment 8799674 [details]
Bug 1299715 - Part 6. Move ComputeMaskGeometry from nsSVGIntegrationUtils to nsDisplayMask.
https://reviewboard.mozilla.org/r/84814/#review83584
::: layout/base/nsDisplayList.cpp:6878
(Diff revision 2)
> +}
> +
> +static void
> +ComputeMaskGeometry(PaintFramesParams& aParams)
> +{
> + // Properties are added lazily and may have been removed by a restyle, so
Wrong indent
Attachment #8799674 -
Flags: review?(mstange) → review+
Comment 79•8 years ago
|
||
mozreview-review |
Comment on attachment 8798767 [details]
Bug 1299715 - Part 7. Correct clip region for mask surface.
https://reviewboard.mozilla.org/r/84180/#review83592
Attachment #8798767 -
Flags: review?(mstange) → review+
Comment 80•8 years ago
|
||
mozreview-review |
Comment on attachment 8798136 [details]
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element.
https://reviewboard.mozilla.org/r/83682/#review83594
Awesome. I haven't looked at every single test in detail, but what I've seen looks great.
Attachment #8798136 -
Flags: review?(mstange) → review+
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) |
Assignee | ||
Comment 90•8 years ago
|
||
Assignee | ||
Comment 91•8 years ago
|
||
Assignee | ||
Comment 92•8 years ago
|
||
Assignee | ||
Comment 93•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/221d9d15395f79c573d081e76df7e93555846b80
Bug 1299715 - Part 1. Replace ContainerItemType::eSVGEffects by ContainerItemType::eFilter. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad315d1eb8945a9a655b3ce3c74d4b6b8c1e562
Bug 1299715 - Part 2. Correct the value of nsDisplayMask::mVisibleRect. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2428d586015dfe9d323b2da64dac37ef41ab43
Bug 1299715 - Part 3. Clip the target context by visible region of nsDisplayMask. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa72646efc0a8869ecb70f3875353183d48a41f2
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/c530869073563f3ae21442c1c8f40b70621c7099
Bug 1299715 - Part 5. Correct clip region for opacity surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/b91db6687d19cd68e39774058f69699013ece4c3
Bug 1299715 - Part 6. Move ComputeMaskGeometry from nsSVGIntegrationUtils to nsDisplayMask. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0b13e4444ac37904541be5195428385e65d134
Bug 1299715 - Part 7. Correct clip region for mask surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c910a1c399668b785dbb4fd2eefb26af19b66b
Bug 1299715 - Part 8. Fix typo in comment of GetVisualOverflowRect. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5077078dca3400dc69c6146dfce61ded547812
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element. r=mstange
Comment 94•8 years ago
|
||
Backed out for failing reftest clipPath-and-mask-on-outflowElement-01a.html on Windows 8 x64:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eac6c1e00f621de91e0316858d7f7e6592dc4186
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca77908e8e4288503e5bd17f147c0d98a5711e16
https://hg.mozilla.org/integration/mozilla-inbound/rev/7170d142d215bbdca8500585a50bc81a24454252
https://hg.mozilla.org/integration/mozilla-inbound/rev/5380881bab1f7549cc54d60df5b758164dc69292
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca00c1a74649dde3a691cc0179328dde099452f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6325d00c5f8fb701ee3771a2a3f7ec6779ce4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa7de99dc3778ae081077c59a1451e7b318538c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce837cd36349cd2c43d29b75e83d8bc1f3b711f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f1c51250c9a48ca92a783e09055f3332e0c6aa
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7d5077078dca3400dc69c6146dfce61ded547812
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37456299&repo=mozilla-inbound
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/clipPath-and-mask-on-outflowElement-01a.html == file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/clipPath-on-outflowElement-01-ref.html | image comparison, max difference: 255, number of differing pixels: 10000
Flags: needinfo?(cku)
Assignee | ||
Comment 95•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a514de15ff57c619b48045af7817680e9b8427fc
Bug 1299715 - Part 1. Replace ContainerItemType::eSVGEffects by ContainerItemType::eFilter. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8483cd4155cf767b78ffcd2795b6a92e974d31
Bug 1299715 - Part 2. Correct the value of nsDisplayMask::mVisibleRect. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6749fffc38b632df487e3aaa9d3b7b5b02d7fc7
Bug 1299715 - Part 3. Clip the target context by visible region of nsDisplayMask. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac90ef439ca4af9653115fad242a74670b28af0
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/63aa1ed1a4c0892500d2f84f3288ad3b6d1318f6
Bug 1299715 - Part 5. Correct clip region for opacity surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/be6c76f9b4e33c382824f291e5dea6e5af739c00
Bug 1299715 - Part 6. Move ComputeMaskGeometry from nsSVGIntegrationUtils to nsDisplayMask. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb327d0777818d0507a702f52a64ae5df7d8123
Bug 1299715 - Part 7. Correct clip region for mask surface. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5d3cfd6767045ba218418bcb000083add6d11a
Bug 1299715 - Part 8. Fix typo in comment of GetVisualOverflowRect. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2908794b6c9859048d81c2cf598bfca89a8ea2
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element. r=mstange
Comment 96•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a514de15ff57
https://hg.mozilla.org/mozilla-central/rev/7d8483cd4155
https://hg.mozilla.org/mozilla-central/rev/e6749fffc38b
https://hg.mozilla.org/mozilla-central/rev/4ac90ef439ca
https://hg.mozilla.org/mozilla-central/rev/63aa1ed1a4c0
https://hg.mozilla.org/mozilla-central/rev/be6c76f9b4e3
https://hg.mozilla.org/mozilla-central/rev/5bb327d07778
https://hg.mozilla.org/mozilla-central/rev/3d5d3cfd6767
https://hg.mozilla.org/mozilla-central/rev/4e2908794b6c
Status: NEW → RESOLVED
Closed: 8 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
•