stylo: Before change style is not correct in test_animations_omta_start.html if we disable e10s

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
P2
normal
24 days ago
23 days ago

People

(Reporter: boris, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

24 days ago
After enabling OMTA, we can pass all the tests in test_animations_omta_start.html with e10s. However, I got 3 test failures without e10s (--disable-e10s), and the failures are [1]:

TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_omta_start.html | transition that interrupted animation is correct - got "", expected "0.4" 
TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_omta_start.html | transition that interrupted animation is correct - got "", expected "0.7"
TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_omta_start.html | transition that interrupted animation is correct - got "1", expected "0.7"

I dumped some log, and noticed that we didn't trigger a transition on the child element [2]. With e10s, we triggered a transition from 0.4 to 1.0 for opacity; however, without e10s, we tried to trigger a transition from 1.0 to 1.0 for opacity, so it failed and nothing happened. Looks like we cannot get correct before change style at 0.4s, from cascade?

There is a workaround, just call "getComputedStyle(parent).opacity" before "child.style.opacity = "1.0"", then everything looks good for both e10s and non-e10s. I'm not sure the root cause right now, so I would like to skip test_animations_omta_start.html in Bug 1334036 until we fix this problem.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#167-176
[2] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#152,164
Comment hidden (obsolete)
(Reporter)

Comment 2

23 days ago
(In reply to Boris Chiou [:boris] from comment #1)
> On possible root cause:
>   Looks like CanThrottle() has a different behavior if we disable e10s. With
> e10s, after we advance 4000ms and wait for paints, we should update layer,
> so CanThrottle() should return false. However without e10s, CanThrottle()
> still return true, so we didn't update layer in time. Maybe I haven't
> handled the animation generation properly.

OK, CanThrottle has the similar behavior in Gecko, so this is expected.

BTW, I notice that if I change waitForAllPaints to waitForAllPaintsFlushed, then we can get the correct before change style. Is it a waitForAllPaints() bug in stylo (on OMTA)?
(Reporter)

Comment 3

23 days ago
(In reply to Boris Chiou [:boris] from comment #2)
> (In reply to Boris Chiou [:boris] from comment #1)
> > On possible root cause:
> >   Looks like CanThrottle() has a different behavior if we disable e10s. With
> > e10s, after we advance 4000ms and wait for paints, we should update layer,
> > so CanThrottle() should return false. However without e10s, CanThrottle()
> > still return true, so we didn't update layer in time. Maybe I haven't
> > handled the animation generation properly.
> 
> OK, CanThrottle has the similar behavior in Gecko, so this is expected.
> 
> BTW, I notice that if I change waitForAllPaints to waitForAllPaintsFlushed,
> then we can get the correct before change style. Is it a waitForAllPaints()
> bug in stylo (on OMTA)?

Therefore, I guess we should flush pending layout notifications (for OMTA) after advancing 4000ms and calling waitForAllPaints(), for non-e10s.
(Reporter)

Updated

23 days ago
Blocks: 1337599
Comment hidden (obsolete)
The point we should investigate is after this waitForAllPaints() [1]. At this point we should check that the opacity value is surely 'inherit' (without flushing style).

And after that we should also check the opacity value is surely 0.4 after advanceTimeAndRefresh(0) [2].  I am confident that the opacity is 0.4 here since getComputedStyle(child).opacity is called right before it.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#149
[2] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#165

Anyway, we should not flush style explicitly if we do the test becomes meaningless.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Anyway, we should not flush style explicitly if we do the test becomes
> meaningless.
* if we do it (flushing style)*
(Reporter)

Comment 7

23 days ago
I tried dump more log (without e10s)

After calling advance 4000ms and wait for all paintings, we call
  child.style.opacity = "1.0"; [1]

1. Gecko:
  a) We call PostRestyleForThrottledAnimations().
  b) A new restyle:
     b-1) Call GeckoRestyleManager::UpdateOnlyAnimationStyles():
            do interpolate -> "Opacity(0) + Opacity(1) at 0.4 => Opacity(0.4)" on parent element
     b-2) Call ProcessingPendingStyles:
            try to trigger a transition, the before change style of child element is opacity(0.4)

2. Stylo:
  a) We call PostRestyleForThrottledAnimations().
  b) A new restyle:
     b-1) animation-only restyle: we do interpolate -> "Opacity(0) + Opacity(1) at 0.4 => Opacity(0.4)" on parent element
     b-2) normal restyle: try to trigger a transition, but the before change style of child element is "opacity(1)",
                          so there is no transition, test failed

What I did in the previous comments (i.e. use WaitForAllPaintsFlushed or call getComputedStyle(parent).opacity before child.style.opacity = "1.0") is to split the restyle (b) into two restyles (c) and (d):
  c) A new restyle:
     c-1) animation-only restyle: update parent element, opacity -> 0.4
     c-2) normal restyle: nothing
  d) Another new restyle caused by 'child.style.opacity = "1.0"';
     d-1) animation-only restyle: nothing"
     d-2) normal restyle: try to trigger a transition, the before change style of child element is "opacity(0.4)", pass!

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#152


As you mentioned, we should check the opacity value of child is 0.4 after WaitForAllPaints(). I notice that if the animation only restyle (for parent) and the normal restyle (for child) in the same tick, the bug happens. It seems we didn't cascade properly for the child element?
(Reporter)

Comment 8

23 days ago
(In reply to Boris Chiou [:boris] from comment #7)
> I tried dump more log (without e10s)
> 
> After calling advance 4000ms and wait for all paintings, we call
>   child.style.opacity = "1.0"; [1]
and getComputedStyle(child).opacity;
(Reporter)

Comment 9

23 days ago
(In reply to Boris Chiou [:boris] from comment #7)
> What I did in the previous comments (i.e. use WaitForAllPaintsFlushed or
> call getComputedStyle(parent).opacity before child.style.opacity = "1.0") is
> to split the restyle (b) into two restyles (c) and (d):
>   c) A new restyle:
>      c-1) animation-only restyle: update parent element, opacity -> 0.4
>      c-2) normal restyle: nothing
>   d) Another new restyle caused by 'child.style.opacity = "1.0"';
>      d-1) animation-only restyle: nothing"
>      d-2) normal restyle: try to trigger a transition, the before change
> style of child element is "opacity(0.4)", pass!

With e10s, we are in the above case (i.e. two restyles). We update animation-only (c) just after calling advance 4000ms and WaitForAllPaints [1], and then do the other restyle (d) just after 'child.style.opacity = "1.0"'. [2]

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#150-151
[2] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#152,164
I am wondering that the todo(opacity, "0.4",...) check is passed on stylo on non-e10s? If it's passed, I think that's the bug.

After looking more closely the test case, it seems to me that there is no chance to get opacity property of parent element for child.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta_start.html#157
(Reporter)

Comment 11

23 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> I am wondering that the todo(opacity, "0.4",...) check is passed on stylo on
> non-e10s? If it's passed, I think that's the bug.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/
> test_animations_omta_start.html#157

The todo(...) is still not passed on stylo.

> 
> After looking more closely the test case, it seems to me that there is no
> chance to get opacity property of parent element for child.

So is that a bug on stylo? If so, I'd like to land Bug 1334036 first and skip test_animations_omta_start.html for now.
(In reply to Boris Chiou [:boris] from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > I am wondering that the todo(opacity, "0.4",...) check is passed on stylo on
> > non-e10s? If it's passed, I think that's the bug.
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/
> > test_animations_omta_start.html#157
> 
> The todo(...) is still not passed on stylo.
> 
> > 
> > After looking more closely the test case, it seems to me that there is no
> > chance to get opacity property of parent element for child.
> 
> So is that a bug on stylo? If so, I'd like to land Bug 1334036 first and
> skip test_animations_omta_start.html for now.

Yeah, we need to figure out how to get up-to-dated inherit animating value running on the compositor when a child element style is changed.
You need to log in before you can comment on or make changes to this bug.