Closed Bug 1299715 Opened 8 years ago Closed 8 years ago

Replace ContainerItemType::eSVGEffects with eMask and eFilter.

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

test case
(deleted), text/html
Details
Bug 1299715 - Part 1. Replace ContainerItemType::eSVGEffects by ContainerItemType::eFilter.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 9. Test cases of clip-path and mask over an out-of-flow element.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 4. Correct clip region for clip-path mask surface.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 2. Correct the value of nsDisplayMask::mVisibleRect.
(deleted), text/x-review-board-request
mstange
: review+
Details
test opacity
(deleted), text/html
Details
Bug 1299715 - Part 3. Clip the target context by visible region of nsDisplayMask.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 5. Correct clip region for opacity surface.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 7. Correct clip region for mask surface.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 8. Fix typo in comment of GetVisualOverflowRect.
(deleted), text/x-review-board-request
mstange
: review+
Details
Bug 1299715 - Part 6. Move ComputeMaskGeometry from nsSVGIntegrationUtils to nsDisplayMask.
(deleted), 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>
Attached file test case (deleted) — 

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8798135 -
        Flags: review?(mstange)

        Attachment #8798136 -
        Flags: review?(mstange)

    
    

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

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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?

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8798371 -
        Flags: review?(mstange)

        Attachment #8798372 -
        Flags: review?(mstange)

    
    

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

    
    

    
  
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

    
    

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

    
    

    
  
(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

    
    

    
  

      
      
      
      

        Attached file
          
        test opacity (deleted)
        — 
      

    


  

    
    

    
  
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.

    
  
Blocks: mask-ship, mask-image
No longer blocks: mask-perf

    
    

    
  
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

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8798765 -
        Flags: review?(mstange)

        Attachment #8798766 -
        Flags: review?(mstange)

        Attachment #8798767 -
        Flags: review?(mstange)

        Attachment #8798768 -
        Flags: review?(mstange)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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 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 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+

    
    

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

    
    

    
  
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

    
    

    
  
I mean, why are image masks "bounded inside the applied element"?

    
    

    
  
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.

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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 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 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 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 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 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 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+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
  
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)

    
    

    
  
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

    
  
Flags: needinfo?(cku)
Depends on: 1356179

    
  
Blocks: 1343147

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: