Closed Bug 1217803 Opened 4 years ago Closed 4 years ago

Animating elements don't always respect border-radius mask

Categories

(Core :: Graphics: Layers, defect)

Unspecified
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: wilsonpage, Assigned: mattwoodrow)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(2 files, 2 obsolete files)

A combination of `border-radius` and `overflow: hidden` doesn't *always* mask animating child at corners.

EXAMPLE: http://jsbin.com/qofima/edit?html,css,output
This reproduces 100% for me on Linux with apz and async-animations enabled.

Milan, is there anyone on gfx that can look at this?
Flags: needinfo?(milan)
With async animations, I imagine Brian could help.  For me, the "not masking" is frequent, but temporary - at least in the above example, in that I see it unmasked, but then it gets masked on the next refresh.
I'm not convinced this is a big deal, especially since it doesn't work at all in Safari, and has exactly the same behaviour in Chrome (to be fair, IE works.)
Flags: needinfo?(milan) → needinfo?(bbirtles)
OS: Unspecified → All
By 100%, I meant while animating - it indeed settles into the right rendering once the animation finishes. Also interesting that this happens in Chrome, rather unexpected! (and kudos IE :))
Total guess: I'd suggest first looking into the code that should be building the mask layer. I'd imagine we avoid building the mask layer during OMTA if the initial position doesn't require it without considering if any off the possible OMTA value require it.

If you set:
span {
  transform: scaleX(0.9);
}

making both the initial and final position require the mask then it works properly.
(In reply to Benoit Girard (:BenWa) from comment #4)
> Total guess: I'd suggest first looking into the code that should be building
> the mask layer. I'd imagine we avoid building the mask layer during OMTA if
> the initial position doesn't require it without considering if any off the
> possible OMTA value require it.

Looks like it:

  https://dxr.mozilla.org/mozilla-central/rev/28068d907290d1f5138a0b9e59ae2233a1c1b7a3/layout/base/FrameLayerBuilder.cpp#4133

I guess we need to do something like nsLayoutUtils::ComputeSuitableScaleForAnimation where we work out the maximum animated scale and then use that to work out the maximum content bounds before deciding if we need the mask layer or not.
Flags: needinfo?(bbirtles)
Whiteboard: gfx-noted
Assignee: nobody → motoryo1
Motozawa-san, will you have time to work on this, or is it ok to re-assign to someone else?
Flags: needinfo?(motoryo1)
Assignee: motoryo1 → mantaroh
Flags: needinfo?(motoryo1)
Attachment #8770763 - Attachment is obsolete: true
Couldn't we also have a similar problem with transforms other than scales?

Like if we translated the content so that it intersected with the corner when it didn't previously?

What about other async changes, like scrolling with apz?

We might need to detect if the content can at all be affected by APZ, and treat the region as infinite in that case.
Or we could always treat the region as infinite. (And actually just replace the condition with layerClip.GetRoundedRectCount() > 0).
We enter this code for active layers, and usually they're active because their contents are in some way animated.
That seems like a pretty reasonable simplification to me.
Duplicate of this bug: 1284720
Assignee: mantaroh → matt.woodrow
Attachment #8770765 - Attachment is obsolete: true
Attachment #8770791 - Flags: review?(mstange)
Comment on attachment 8770791 [details] [diff] [review]
always-build-mask-layer

Review of attachment 8770791 [details] [diff] [review]:
-----------------------------------------------------------------

This will also fix bug 1286622 which I coincidentally filed today.

Please add a test.
Attachment #8770791 - Flags: review?(mstange) → review+
Blocks: 1286622
I'm not aware of an easy way to write tests for async animations compositing behaviour.
Ok, I'm not sure how to do those offhand either, but you could turn the testcase in bug 1286622 into an async scrolling test.
Attached patch ReftestSplinter Review
Struggling with this a bit, this test still passes.
Depends on: 1287654
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6d8283458c
Always build a mask layer if we have rounded corners, regardless of the visible region since this might change on the compositor. r=mstange
https://hg.mozilla.org/mozilla-central/rev/2e6d8283458c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1289321
You need to log in before you can comment on or make changes to this bug.