Closed Bug 1275694 Opened 3 years ago Closed 3 years ago

Animating an element into view, inside an overflow:hidden container causes graphical glitches

Categories

(Core :: Layout, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: gordonrankin82, Assigned: botond)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached file example.html (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Steps to reproduce:

Animate an element, that is inside an 'overflow:hidden' parent into view using translate with css-transitions.

The animating element must start translated 'out of view'.


Actual results:

The animating element is not visible during it's entry.  Sometimes, other visual glitches occur instead.


Expected results:

The element should smoothly transition into view.

I have attached a testcase to better explain the problem, which occurs only on recent Nightlies.

In the testcase the parent overflow:hidden element is a grey rectangle and the inner element is a green rectangle.  The inner element starts translated -100% on its x axis so that it is outside the parent element.  

If you then click the 'TOGGLE' button, the element should animate in.  Clicking the toggle button again will then return the inner element to its starting position.
Version: 48 Branch → 49 Branch
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
This is broken because of this code: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#807

We intentionally use the 'client' side visible rect to get around checkboarding problems. Unfortunately this layer is entirely invisible on the client side (before the animation moves it) and so we never paint it, regardless of where the animation moves it to.

The client side visible region is what is visible in the current display port, and doesn't take async animation/apz into account. The shadow visible region is what is visible in the current viewport, and does take async animation/apz into account.

I think what we really want to check here is the visibility within the displayport, but taking async animation/apz into account. This isn't something we compute, and it would be somewhat of a pain to do so.

One option would be to only return here if both the visible regions are empty, another would be to skip the check if the layer is one that we'd want to paint checkerboard for.

Any thoughts Botond?
Flags: needinfo?(matt.woodrow) → needinfo?(botond)
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> I think what we really want to check here is the visibility within the
> displayport, but taking async animation/apz into account. This isn't
> something we compute, and it would be somewhat of a pain to do so.

Looping :seth in here. He's done a bunch of work recently related to computing visibility, including within the displayport. Perhaps leveraging some of his work, it's not as much of a pain to compute?
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> One option would be to only return here if both the visible regions are
> empty, another would be to skip the check if the layer is one that we'd want
> to paint checkerboard for.

Either of these seems like a reasonable workaround to me, with perhaps the latter seeming more robust.

We can check "if the layer is one that we'd want to paint checkerboard for" using the NeedToDrawCheckerboardingForLayer() function in ContainerLayerComposite.cpp (which we could move to a header).
Flags: needinfo?(botond)
Finally, it's worth noting that Markus has a plan to revamp the way checkerboarding backgrounds are drawn that will avoid this issue altogether.
Attached patch bug1275694 (obsolete) — Splinter Review
Assignee: nobody → matt.woodrow
Attachment #8760505 - Flags: review?(botond)
Attachment #8760505 - Flags: review?(botond) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8e6e22a983
Use the presence of checkerboarding to decide when to override the clip rect rather than always using the client side visible rect. r=botond
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29860978&repo=mozilla-inbound
Flags: needinfo?(matt.woodrow)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da40184c044a
Backed out changeset 1b8e6e22a983 for test failures in checkerboard-3.html
Hmm, checkerboard-3.html is the same test that failed when we originally tried just using GetLocalVisibleRegion().

I wonder if perhaps the layer in question is a ContainerLayer, and the scroll metadata is on a child PaintedLayer, in which case NeedToDrawCheckerboarding() would only return true for the child.
Marking 50 as affected, Matt maybe you can take this on before 49 goes to beta. Botond, do you think the issue is in the test, or in the patch?
Flags: needinfo?(botond)
I believe the problem is in the patch. If my theory from comment 10 is right, we should be able to fix the patch by adjusting the added check to call NeedToDrawCheckerboarding() on the entire layer subtree, not just the one layer.

I can have a look at this on Monday if Matt doesn't get to it by then.
Flags: needinfo?(botond)
I looked into this. My theory from comment 10 was wrong, the patch actually had a different problem: NeedToDrawCheckerboarding() was only implemented for container layers, but the layer in question was a color layer.
Er, that commit message is quite misleading. Let me try again.
Comment on attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62168/diff/1-2/
Attachment #8767762 - Attachment description: Bug 1275694 - Avoid culling a layer in CalculateScissorRect() if we need to draw a checkerboarding background for it. → Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().
Assignee: matt.woodrow → botond
Flags: needinfo?(matt.woodrow)
Comment on attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

https://reviewboard.mozilla.org/r/62168/#review59068

::: gfx/layers/Layers.h:1561
(Diff revision 2)
>      return !GetLocalVisibleRegion().IsEmpty() || Extend3DContext();
>    }
>  
>    /**
> +   * Return true if a checkerboarding background color needs to be drawn
> +   * for this layer. This can only ever be true for LayerComposites.

I'd prefer to not even have this on the Layer class, and instead have the callsites get the LayerComposite directly. I see two callsites of this function in the patch, one of which already has the LayerComposite in a local var, so it shouldn't be a huge burden to do.
Comment on attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62168/diff/2-3/
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6678b3791f1
Use the local visible region to decide when to cull a layer in CalculateScissorRect(). r=kats
Addressed review comment and autolanded.
https://hg.mozilla.org/mozilla-central/rev/f6678b3791f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attachment #8760505 - Attachment is obsolete: true
Could someone who can reproduce the original problem, verify that the patch fixes it? Thanks!
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gordonrankin82)
Flags: needinfo?(alice0775)
Reproduced the problem on Nightly49.0a1[1].
And verified to fix on Nightly50.0a1[2].

[1]https://hg.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160520030251

[2]https://hg.mozilla.org/mozilla-central/rev/45682df2d2d45e5a8385fd842579e661a4b60bc5
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 ID:20160708030216
Flags: needinfo?(alice0775)
Great, thank you Alice!
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gordonrankin82)
Comment on attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

As per comment 11, we'd like to uplift this to 49.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1141468

[User impact if declined]:
  Sometimes, when CSS transitions are used to animate an element
  that start off-screen, onto the screen, the element can appear
  too late.

[Describe test coverage new/current, TreeHerder]:
  Verified fix by manual testing, and lack of regressions via
  automatic tests.
  
[Risks and why]: 
  Low to medium. The fix is small and localized, but it does
  touch the compositor rendering code.

[String/UUID change made/needed]:
  Small.
Attachment #8767762 - Flags: approval-mozilla-aurora?
Comment on attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

Fix for regression in 49, verified in m-c. Let's uplift to aurora so that we never ship this regression
Attachment #8767762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is still broken in the latest Nightly.

Although the original testcase I created is indeed now fixed, if I modify the testcase to include any content inside the animating element (Even if it is simply some text), then the problem begins to occur again.

I will create a new testcase and attach it.
Status: RESOLVED → REOPENED
Flags: needinfo?(bugmail)
Flags: needinfo?(botond)
Flags: needinfo?(alice0775)
Resolution: FIXED → ---
This is a new testcase, showing the the original bug is still present when there is content inside the animating element.  In this case, simply adding the text 'TESTING' inside the element, caused the bug to re-occur.
Attachment #8756498 - Attachment is obsolete: true
Matt is probably the best person to investigate this.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugmail)
Flags: needinfo?(botond)
Flags: needinfo?(alice0775)
Also worth noting, is that every time we animate the transform, the whole element is repainted (presumably at the end).  My understanding is that there should be no need for this as animating "transform:translate" should not cause a repaint at all.

In our app, we currently have a huge amount of animations of this sort, with several elements that are being animated, via translate/scale (transforms) in to, or out of view.  This causes huge paint times of 100-200ms each time we animate, despite the fact that simply translating an element should not cause a repaint of that element at all.  Perhaps I should create a better test case showing this and file a separate bug?
(In reply to gordonrankin82 from comment #30)
> Created attachment 8772956 [details]
> Example 2 - With content inside animating element
> 
> This is a new testcase, showing the the original bug is still present when
> there is content inside the animating element.  In this case, simply adding
> the text 'TESTING' inside the element, caused the bug to re-occur.


Wow! i think this specific issue is whats causing animation's not appearing in outlook.com 
https://webcompat.com/issues/1751 - video to show the animation problem.   
https://bugzilla.mozilla.org/show_bug.cgi?id=1275000 - actual bug report
(In reply to gordonrankin82 from comment #32)
> Also worth noting, is that every time we animate the transform, the whole
> element is repainted (presumably at the end).  My understanding is that
> there should be no need for this as animating "transform:translate" should
> not cause a repaint at all.
> 
> In our app, we currently have a huge amount of animations of this sort, with
> several elements that are being animated, via translate/scale (transforms)
> in to, or out of view.  This causes huge paint times of 100-200ms each time
> we animate, despite the fact that simply translating an element should not
> cause a repaint of that element at all.  Perhaps I should create a better
> test case showing this and file a separate bug?

You should be filed a new separate bug.
Flags: needinfo?(gordonrankin82)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
> You should be filed a new separate bug.

Thanks Alice, I've just done so: bug 1288216
Alice, you marked this as fixed, the original bug is still occurring.  Please see comment 29 and comment 30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(gordonrankin82)
(In reply to gordonrankin82 from comment #36)
> Alice, you marked this as fixed, the original bug is still occurring. 
> Please see comment 29 and comment 30

> Although the original testcase I created is indeed now fixed.

Do not reopen this.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED
Depends on: 1288216
What Alice is saying is: There was a bug fixed here, even if it's not the complete bug as filed originally. In these cases, we recommend filing a new bug for fixing the rest of the original problem. This makes tracking fixes much easier, because there will be only one landed fix per bug.
Blocks: 1288244
(In reply to Markus Stange [:mstange] from comment #38)
> What Alice is saying is: There was a bug fixed here, even if it's not the
> complete bug as filed originally. In these cases, we recommend filing a new
> bug for fixing the rest of the original problem. This makes tracking fixes
> much easier, because there will be only one landed fix per bug.


Anyway, I have filed a new bug 1288244 for new test case attachment 8772956 [details].
You need to log in before you can comment on or make changes to this bug.