Ian Moody [:Kwan] (UTC+1) Reporter
	Description • 7 years ago

Testcase at http://kwan.perix.co.uk/mozilla/blinkTest.html

Only the first line blinks if layers.offmainthreadcomposition.async-animations is true (the default).  False makes the whole thing blink.

Setting gfx.webrender.enabled to true instead also fixes it.

Only tested on linux so far, both fx 56 and 58.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 1 • 7 years ago

(In reply to Ian Moody [:Kwan] from comment #0)
> Setting gfx.webrender.enabled to true instead also fixes it.

Correction: both gfx.webrender.enabled & layers.acceleration.force-enabled to true

Also of note: continuously moving the mouse pointer makes it work.  Only when the mouse is still do the later lines not blink.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 2 • 7 years ago

Reproduced on Win 7 with a trunk build.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 3 • 7 years ago

All the lines blink if you keep moving the mouse.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 4 • 7 years ago

More things of note:

Setting both gfx.webrender.enabled & layers.acceleration.force-enabled to true fixing it only seems to be true of trunk, not 56.

When OMTC.async-animations is false and it's working correctly, if you enable devtools's paint flashing the first line is shown coloured separately to the rest, which I guess means it's being painted separately?
(I tried using gfx.webrender.highlight-painted-layers while the above two prefs were true but it didn't show anything in the page, only UI)

I tried to write a reftest for this (using a single animation run rather than a looping one), and while I can see the bug myself if I wait a bit before removing the .reftest-wait from documentElement, reftest's screenshot (or whatever it does) seems to trigger a repaint or something so the opacity applies to the whole element, and the test can't see the bug.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 5 • 7 years ago

(In reply to Ian Moody [:Kwan] from comment #4)
> Setting both gfx.webrender.enabled & layers.acceleration.force-enabled to
> true fixing it only seems to be true of trunk, not 56.

mozregression traced this to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=17a1159f7f4d38777269f00b595096dce6f63d26&tochange=273f8af59697c9a025043bd050b62ae2f1ba04ec so this was:
 
43963e011193	Kartikaya Gupta — Bug 1389000 - Turn on webrender layers-free mode by default. r=jrmuizel

Adding gfx.webrender.layers-free to the other two doesn't fix it in 56, but re-bisecting with it true as well gets a lot of bugs, and then to https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=85b26c06735de0c68a2157696327ff0e58e4b981&tochange=e3782e1354af50aca5947a374a96a7819768be0e of which two seem related, the latter more so:

e3782e1354af	Morris Tseng — Bug 1378606 - Support mix-blend-mode in layers-free mode.
d4d95cd1fdff	Morris Tseng — Bug 1384000 - Call TriggerPendingAnimations in layers-free mode. r=ethlin,kats

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 6 • 7 years ago

All of these were meant to be WebRender specific changes, Morris, Kats, did we introduce a non-WR regression with some of these patches?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 7 • 7 years ago

Hmm, I can get this not working in 2015.  Having trouble with mozregression, but I see it working in 2014-11-27 and not working in 2015-11-13.  Brian, was that around the time we introduced OMTA?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 8 • 7 years ago

This apepars to have been around since Firefox 40 - https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=883e17fc475f&tochange=078128c2600a

David, possibly bug 1149848?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 9 • 7 years ago

(Or I guess the OMTA in the first place, bug 980770)

	David Baron :dbaron:
	Comment 10 • 7 years ago

Yeah, this is a pretty basic bug -- not sure how it didn't get noticed until now.

At this point, though, the OMTA code has changed enough since I last worked on it that I'm not particularly knowledgeable about it, so I think this is a question for birtles & team.

We somehow need to apply the style to all continuations of the frame even though we generally compute it only for the first and then copy it to the rest.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 11 • 7 years ago

(In reply to Ian Moody [:Kwan] from comment #4)
> I tried to write a reftest for this (using a single animation run rather
> than a looping one), and while I can see the bug myself if I wait a bit
> before removing the .reftest-wait from documentElement, reftest's screenshot
> (or whatever it does) seems to trigger a repaint or something so the opacity
> applies to the whole element, and the test can't see the bug.
So this was just because I needed to read more documentation (https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt) and discover reftest-no-flush.  With that I have a reftest that fails with OMTA on and passes with it off. Though for some reason the fail screenshot shows the first line visible as well as the rest, so it's still not the same as observed behaviour.
Here is said reftest if it's useful.

	Brian Birtles (:birtles)
	Comment 12 • 7 years ago

Sorry for the delay (long weekend here). I'll find someone to look into this.

Having a reliable reftest is very helpful but Ian did you perhaps attach the wrong patch?

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 13 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #12)
> Sorry for the delay (long weekend here). I'll find someone to look into this.
> 
> Having a reliable reftest is very helpful but Ian did you perhaps attach the
> wrong patch?

Yes, yes I did.  Apparently one of its sibling files.  Thanks for the heads up.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 14 • 7 years ago

This is due to a check whether the frame is the primary or not in nsIFrame::HasOpacityInternal().  If the start value is less than 0.99 (e.g. 0.98) in the test case, all continuation frames are animated correctly since StyleEffects()->mOpacity gets less than |aThreshold| (it's 0.99 for HasVisualOpacity).

Taking this, but I don't know the way how to check the frame is primary or one of the continuation frames yet.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 15 • 7 years ago

I am worrying that checking continuation frames wastes optimizations in nsIFrame::HasOpacityInternal, e.g. a recent optimization is bug 1381157.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 16 • 7 years ago

The opacity animation in the reftest by Ian needs to run reversely.  This is the reason why the first line was shown up in the test image.

Anyway thanks for the test case, Ian!

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 17 • 7 years ago

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

	David Baron :dbaron:
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8918073 [details]
Bug 1406211 - Check whether the frame is one of continuation or ib-split-sinbling frames for animating element in nsIFrame::HasOpacityInternal().

https://reviewboard.mozilla.org/r/188918/#review194278

r=dbaron with the changes below

::: commit-message-94053:1
(Diff revision 1)
> +Bug 1406211 - Check whether the frame is one of continuation frames for animating element in nsIFrame::HasOpacityInternal(). r?dbaron

adjust the commit message to say "continuation or ib-split-sibling" or something like that, to reflect the change below

::: commit-message-94053:3
(Diff revision 1)
> +Without this, we don't generate nsDisplayOpacity for the continuation frames
> +other than the first continuation and thus opacity animation on the
> +continuation frames don't run on the compositor.  I.e., the opacity animation

It's clearer to write this without negatives, i.e., by saying:

We need this change to generate nsDisplayOpacity for the continuation frames other than the first continuation so the opacity animation on the continuation frames runs on the compositor.

::: commit-message-94053:5
(Diff revision 1)
> +Bug 1406211 - Check whether the frame is one of continuation frames for animating element in nsIFrame::HasOpacityInternal(). r?dbaron
> +
> +Without this, we don't generate nsDisplayOpacity for the continuation frames
> +other than the first continuation and thus opacity animation on the
> +continuation frames don't run on the compositor.  I.e., the opacity animation

don't -> doesn't

::: commit-message-94053:5
(Diff revision 1)
> +continuation frames don't run on the compositor.  I.e., the opacity animation
> +only for the first continuation frame runs on the compositor and rest of frames
> +never update on the main thread since the animation is throttled.

I'd rephrase this to say:

When we generate an nsDisplayOpacity, the animation runs on the compositor, but when we don't, the animation never updates since the animation is throttled on the main thread.

::: layout/generic/nsFrame.cpp:1372
(Diff revision 1)
> -  return (IsPrimaryFrame() &&
> +  return ((IsPrimaryFrame() ||
> +           FirstContinuation() == mContent->GetPrimaryFrame()) &&

Two things here:
 - you could use nsIFrame::IsPrimaryFrame() to speed things up
 - you need to use nsLayoutUtils::FirstContinuationOrIBSplitSibling() to get things right for block-in-inline splits

Thus, I'd replace these two lines with:

nsLayoutUtils::FirstContinuationOrIBSplitSibling(this)->IsPrimaryFrame()

	David Baron :dbaron:
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8918072 [details]
Bug 1406211 - Early return from nsIFrame::HasOpacityInternal if there is no EffectSet for the frame.

https://reviewboard.mozilla.org/r/188916/#review194280

	David Baron :dbaron:
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8918074 [details]
Bug 1406211 - Add a reftest for animating opacity on a multiline inline element.

https://reviewboard.mozilla.org/r/188920/#review194282

Did you check that this test failed without the patch?

(It feels like it might be a more reliable test if it used step-start and waited for the animation to change state?  But maybe it works even without that.)


Also, please add a test for a block-within-inline split, e.g.:

  <span id="animating">A <span style="display:block">B</span> C</span>

testing that all of A, B, and C animate.  This should only pass once you address my review comments on the middle patch.

r=dbaron with those changes

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 24 • 7 years ago

Thanks for lots of corrections.  I will add the test case for the block-within-inline case.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 25 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #23)
> Also, please add a test for a block-within-inline split, e.g.:
> 
>   <span id="animating">A <span style="display:block">B</span> C</span>
> 
> testing that all of A, B, and C animate.  This should only pass once you
> address my review comments on the middle patch.

I did test it. Without the modification what you suggested (i.e. using FirstContinuationOrIBSplitSibling), only A animates.  With the modification unfortunately B does not still animate. So there is still something I am missing.

	Ian Moody [:Kwan] (UTC+1) Reporter
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8918074 [details]
Bug 1406211 - Add a reftest for animating opacity on a multiline inline element.

https://reviewboard.mozilla.org/r/188920/#review194406

::: layout/reftests/css-animations/continuation-opacity-ref.html:15
(Diff revision 1)
> +  max-width: 15em;
> +}
> +</style>
> +<p>
> +  <blink>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</blink>
> +</p>

I didn't put a newline here either.

::: layout/reftests/css-animations/reftest.list:65
(Diff revision 1)
>  == change-animation-name-to-none-in-rule.html change-animation-name-in-rule-ref.html
>  == change-animation-name-to-other-in-rule.html change-animation-name-in-rule-ref.html
>  == change-animation-name-to-non-existent-in-rule.html change-animation-name-in-rule-ref.html
>  == no-style-sharing-with-animations.html no-style-sharing-with-animations-ref.html
> +
> +== continuation-opacity.html continuation-opacity-ref.html

I realised my change here dropped the newline at EOF, you might want to restore it.

	David Baron :dbaron:
	Comment 27 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> I did test it. Without the modification what you suggested (i.e. using
> FirstContinuationOrIBSplitSibling), only A animates.  With the modification
> unfortunately B does not still animate. So there is still something I am
> missing.

That's probably OK, as long as:
 * C animates
 * it matches the non-animation behavior of 'opacity' on that element

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 28 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #27)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > I did test it. Without the modification what you suggested (i.e. using
> > FirstContinuationOrIBSplitSibling), only A animates.  With the modification
> > unfortunately B does not still animate. So there is still something I am
> > missing.
> 
> That's probably OK, as long as:
>  * C animates
>  * it matches the non-animation behavior of 'opacity' on that element

Thank you, dbaron.  As far as I can tell, yes.  But after some more trials[1][2], it turns out 'B' sometimes animates properly, sometimes not.  So I decided to use a test case without 'B' character, i.e. <span id="animating">A <span style="display:block"></span> C</span>, here and tackle the issue in other bug (1408731).
The revised test case worked fine with FirstContinuationOrIBSplitSibling() check, failed with FirstContinuation() actually.

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3baa4f7880a770ac535ee2b90d306676d2cf9974

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eae3c3d089f70cf3a7fa89cc5e839dcacdf20a7
 * compared with visible B, then almost failed (i.e. B animated)
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9918dd53183968b0258f3caa5edb29d808bf9829
 * compared with about:blank (i.e. invisible B), then sometimes failed

	Pulsebot
	Comment 32 • 7 years ago

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41640ce02209
Early return from nsIFrame::HasOpacityInternal if there is no EffectSet for the frame. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/8da89309fd43
Check whether the frame is one of continuation or ib-split-sinbling frames for animating element in nsIFrame::HasOpacityInternal(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/c24097d6d633
Add a reftest for animating opacity on a multiline inline element. r=dbaron

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/41640ce02209
https://hg.mozilla.org/mozilla-central/rev/8da89309fd43
https://hg.mozilla.org/mozilla-central/rev/c24097d6d633

	David Baron :dbaron:
	Comment 34 • 7 years ago

Did you file a followup bug on getting consistent behavior for the B part of block-in-inline splits?

(It should be consistent between animation and non-animation, and the animation behavior should also be internally consistent and not "sometimes".)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 35 • 7 years ago

Oh, I did forget to put a link here, it's bug 1408731.  Thanks for the heads-up.