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

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gordonrankin82, Assigned: botond)

Tracking

(Depends on: 1 bug, {regression})

49 Branch
mozilla50
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 fixed, firefox50 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8756498 [details]
example.html

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

Updated

2 years ago
Version: 48 Branch → 49 Branch
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
Product: Firefox → Core

Comment 1

2 years ago
regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=204bdc8b929c88d164e165fd8f28e4c1cd002bef&tochange=7fb6f9a2d7fee76746c73fa5ba1a5c091073e199

Regressed by: Bug 1141468
Blocks: 1141468
status-firefox48: --- → unaffected
status-firefox49: --- → affected
Flags: needinfo?(matt.woodrow)
Keywords: regressionwindow-wanted
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)
(Assignee)

Comment 3

2 years ago
(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?
(Assignee)

Comment 4

2 years ago
(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)
(Assignee)

Comment 5

2 years ago
Finally, it's worth noting that Markus has a plan to revamp the way checkerboarding backgrounds are drawn that will avoid this issue altogether.
Created attachment 8760505 [details] [diff] [review]
bug1275694
Assignee: nobody → matt.woodrow
Attachment #8760505 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Attachment #8760505 - Flags: review?(botond) → review+

Comment 7

2 years ago
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)

Comment 9

2 years ago
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
(Assignee)

Comment 10

2 years ago
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?
status-firefox50: --- → affected
Flags: needinfo?(botond)
(Assignee)

Comment 12

2 years ago
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)
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

2 years ago
Created attachment 8767762 [details]
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().

Review commit: https://reviewboard.mozilla.org/r/62168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62168/
Attachment #8767762 - Flags: review?(bugmail)
(Assignee)

Comment 15

2 years ago
Er, that commit message is quite misleading. Let me try again.
(Assignee)

Comment 16

2 years ago
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)

Updated

2 years ago
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.
(Assignee)

Comment 19

2 years ago
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/

Comment 20

2 years ago
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
(Assignee)

Comment 21

2 years ago
Addressed review comment and autolanded.

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6678b3791f1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

2 years ago
Attachment #8760505 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
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)

Comment 24

2 years ago
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)
(Assignee)

Comment 25

2 years ago
Great, thank you Alice!
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gordonrankin82)
(Assignee)

Comment 26

2 years ago
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+

Comment 28

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1269ccfe4c1
status-firefox49: affected → fixed
(Reporter)

Comment 29

2 years ago
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 → ---
(Reporter)

Comment 30

2 years ago
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.
Attachment #8756498 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Matt is probably the best person to investigate this.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugmail)
Flags: needinfo?(botond)
Flags: needinfo?(alice0775)
(Reporter)

Comment 32

2 years ago
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?

Comment 33

2 years ago
(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

Comment 34

2 years ago
(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)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Reporter)

Comment 35

2 years ago
> You should be filed a new separate bug.

Thanks Alice, I've just done so: bug 1288216
(Reporter)

Comment 36

2 years ago
Alice, you marked this as fixed, the original bug is still occurring.  Please see comment 29 and comment 30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

2 years ago
Flags: needinfo?(gordonrankin82)

Comment 37

2 years ago
(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
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED

Updated

2 years ago
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.

Updated

2 years ago
Blocks: 1288244

Comment 39

2 years ago
(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.