1460389 - Emoticon bubble on Github disappears for a short moment

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Darkspirit]

Attachment: 2018-05-09_20-35-12.mp4

Debian Testing, KDE, Radeon RX480
Win10 1803, Nvidia GTX1060
2560x1440 (Dell U2515H)

mozregression --good 2018-05-07 --bad 9294f67b3f3bd4a3dd898961148cecd8bfc1ce9c --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://github.com/carllerche/h2/pull/273'
> 9:39.38 INFO: Last good revision: 3b84be9ee9e6a19d8e49aa86b71c9d60b5eaec8b
> 9:39.38 INFO: First bad revision: 1a51d479e4404608fea41fc4a8e3d5b21b1850a7
> 9:39.38 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3b84be9ee9e6a19d8e49aa86b71c9d60b5eaec8b&tochange=1a51d479e4404608fea41fc4a8e3d5b21b1850a7

> 1a51d479e440	Hiroyuki Ikezoe — Bug 1456679 - Enable tests in test_animations_omta.html on WebRender. r=kats
> 217999de6c88	Hiroyuki Ikezoe — Bug 1456679 - Don't set non-animated values as AnimatedValue in delay phase. r=kats
> a7074da0c89c	Hiroyuki Ikezoe — Bug 1456679 - Update the previous timestamp with the current timestamp even if there are only delayed phase animations. r=kats
> 8e9fc4d911cf	Hiroyuki Ikezoe — Bug 1456679 - Make SampleAnimations return boolean to tell there is any animations even if the animation in delay phase. r=kats

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It seems animation on pseudo element.  We may use a parent nsIFrame somewhere.  Anyway I need to debug it, I'd want devtools to stop this kind of hover triggered animation. :)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: A simplified test case

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Ok, now I think I understand what's going on there.

From WebRenderBridgeParent::CompositeToTarget <https://hg.mozilla.org/mozilla-central/file/96b37ba12225/gfx/layers/wr/WebRenderBridgeParent.cpp#l1328>;

  // We do this even if the arrays are empty, because it will clear out any
  // previous properties store on the WR side, which is desirable.
  txn.UpdateDynamicProperties(opacityArray, transformArray);

I've read this comment before, but missed somehow.   Though I don't have any ideas to solve this, assigning to myself for now.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

A solution I can think of is not to override static opacity/transform value if the array is empty, but I am not sure we can do it, not I am reading webrender code.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Here is a try with an attempt at very early stage.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3beb632516940cca34384006cb710cccdc22eb

This try fixes the original case, but doesn't fix transform animation case yet.  And also I noticed that animations with positive end delay case doesn't originally work either (i.e. before bug 1456679).  So we have to figure out a way to fix these bunch of the issues.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I've filed for the end delay issue in another bug (bug 1460804) since the issue also happens on non-WebRender.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a32a8be31e39c631ce0904b75dff7a1feea1a95b

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Here is another try that makes webrender changes as separate commits.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=849d2cc1cd9189f37d324615fb5b6978731848fe

I will create a PR to webrender repo.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1460389 - Remove TransactionBuilder::AppendTransformProperties.

This function is no longer used since we introduced TransactionWrapper class in
bug 1451469.

Review commit: https://reviewboard.mozilla.org/r/243294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243294/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1460389 - Constify arguments for StackingContextHelper ctor.

Review commit: https://reviewboard.mozilla.org/r/243296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243296/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1460389 - Use non-animated (static) value if the animation is in not in-effect.

Review commit: https://reviewboard.mozilla.org/r/243298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243298/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1460389 - Reftests for opacity and transform animations that are sent to the compositor in their delay phase.

Both tests fail on WebRender without the fix in this patch series.

In the delay phase, those animations should be painted using non-animated
value (i.e. opacity:0 or translateX(100px)), but on WebRender they were painted
using opacity: 1 or identity transform.

Review commit: https://reviewboard.mozilla.org/r/243300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243300/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: The PR to webrender

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8974916 [details]
Bug 1460389 - Remove TransactionBuilder::AppendTransformProperties.

https://reviewboard.mozilla.org/r/243294/#review249148

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8974917 [details]
Bug 1460389 - Constify arguments for StackingContextHelper ctor.

https://reviewboard.mozilla.org/r/243296/#review249150

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8974918 [details]
Bug 1460389 - Use non-animated (static) value if the animation is in not in-effect.

https://reviewboard.mozilla.org/r/243298/#review249156

LGTM. Because of the way our WR update process works I might end up landing this patch as part of bug 1459935 or whatever bug ends up landing the PR you made.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8974918 [details]
Bug 1460389 - Use non-animated (static) value if the animation is in not in-effect.

https://reviewboard.mozilla.org/r/243298/#review249486

::: commit-message-ada5f:1
(Diff revision 1)
> +Bug 1460389 - Use non-animated (static) value if the animation is in delay phase. r?kats,birtles

Nit: I think this is not strictly about the delay phase but about any case where the animation is not in-effect.

::: gfx/webrender_bindings/src/bindings.rs:1626
(Diff revision 1)
> +                                                                        // the value for the case where the animation is
> +                                                                        // in delay phase.

s/is in delay phase/is in not in-effect (e.g. in the delay phase with no corresponding fill mode)/

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8974919 [details]
Bug 1460389 - Reftests for opacity and transform animations that are sent to the compositor in their delay phase.

https://reviewboard.mozilla.org/r/243300/#review249488

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974916 [details]
Bug 1460389 - Remove TransactionBuilder::AppendTransformProperties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243294/diff/1-2/

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974917 [details]
Bug 1460389 - Constify arguments for StackingContextHelper ctor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243296/diff/1-2/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974918 [details]
Bug 1460389 - Use non-animated (static) value if the animation is in not in-effect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243298/diff/1-2/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974919 [details]
Bug 1460389 - Reftests for opacity and transform animations that are sent to the compositor in their delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243300/diff/1-2/

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Bug 1460861 is on autoland and looks like it will stick. So part 3 of this patchset will need to be rebased on top of that (should be pretty trivial - just keep the new version of the code in your patch) and then this should be good to land.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974916 [details]
Bug 1460389 - Remove TransactionBuilder::AppendTransformProperties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243294/diff/2-3/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974917 [details]
Bug 1460389 - Constify arguments for StackingContextHelper ctor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243296/diff/2-3/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974918 [details]
Bug 1460389 - Use non-animated (static) value if the animation is in not in-effect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243298/diff/2-3/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8974919 [details]
Bug 1460389 - Reftests for opacity and transform animations that are sent to the compositor in their delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243300/diff/2-3/

Comment 30

[Darkspirit]

checkin-needed?

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Pushed a final try now,  I will land them later.  (I didn't know that we can land patches for WebRender separately)

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/137e5960bb28
Remove TransactionBuilder::AppendTransformProperties. r=kats
https://hg.mozilla.org/integration/autoland/rev/8e9820ac53ce
Constify arguments for StackingContextHelper ctor. r=kats
https://hg.mozilla.org/integration/autoland/rev/bfc8fa6e75a2
Use non-animated (static) value if the animation is in not in-effect. r=birtles,kats
https://hg.mozilla.org/integration/autoland/rev/63988e7c891f
Reftests for opacity and transform animations that are sent to the compositor in their delay phase. r=birtles

Comment 33

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/137e5960bb28
https://hg.mozilla.org/mozilla-central/rev/8e9820ac53ce
https://hg.mozilla.org/mozilla-central/rev/bfc8fa6e75a2
https://hg.mozilla.org/mozilla-central/rev/63988e7c891f

Comment 34

[Darkspirit]

Verified fixed in Nightly 62 x64 20180516100125 de_DE @ Debian Testing (Radeon RX480). Thanks!
Emoticon bubble on Github and Gmail's loading animation are fixed.