Closed
Bug 1275694
Opened 9 years ago
Closed 9 years ago
Animating an element into view, inside an overflow:hidden container causes graphical glitches
Categories
(Core :: Layout, defect)
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)
Bug 1275694 - Use the local visible region to decide when to cull a layer in CalculateScissorRect().
58 bytes,
text/x-review-board-request
|
kats
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
1.26 KB,
text/html
|
Details |
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•9 years ago
|
Version: 48 Branch → 49 Branch
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression,
regressionwindow-wanted
Product: Firefox → Core
![]() |
||
Comment 1•9 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
Comment 2•9 years ago
|
||
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•9 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•9 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•9 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.
Comment 6•9 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8760505 -
Flags: review?(botond)
Assignee | ||
Updated•9 years ago
|
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
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 10•9 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.
Comment 11•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
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•9 years ago
|
||
Er, that commit message is quite misleading. Let me try again.
Assignee | ||
Comment 16•9 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•9 years ago
|
Assignee: matt.woodrow → botond
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 17•9 years ago
|
||
Updated•9 years ago
|
Attachment #8767762 -
Flags: review?(bugmail) → review+
Comment 18•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Addressed review comment and autolanded.
Comment 22•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Updated•9 years ago
|
Attachment #8760505 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 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•9 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•9 years ago
|
||
Great, thank you Alice!
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gordonrankin82)
Assignee | ||
Comment 26•9 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 27•9 years ago
|
||
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•9 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 29•9 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•9 years ago
|
||
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•9 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•9 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•9 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•9 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•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 35•9 years ago
|
||
> You should be filed a new separate bug.
Thanks Alice, I've just done so: bug 1288216
Reporter | ||
Comment 36•9 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•9 years ago
|
Flags: needinfo?(gordonrankin82)
![]() |
||
Comment 37•9 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
Closed: 9 years ago → 9 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED
Comment 38•9 years ago
|
||
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.
![]() |
||
Comment 39•9 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.
Description
•