Closed Bug 1362292 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1361938

People

(Reporter: boris, Unassigned)

References

Details

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
(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)?
(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.
Blocks: 1337599
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)*
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?
(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;
(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
(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.
Priority: P2 → --
P2.5
Priority: -- → P3
Boris, do three tests in comment 0 still fail?  All of them pass locally with disabling e10s on my linux box.
Flags: needinfo?(boris.chiou)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Boris, do three tests in comment 0 still fail?  All of them pass locally
> with disabling e10s on my linux box.

Me, too. And it seems someone enabled it already [1]. I didn't see any "skip-if" for this test file.

[1] http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/layout/style/test/mochitest.ini#64
Flags: needinfo?(boris.chiou)
OK, great.  The remaining issue in the test is bug 1039799 (which is marked as todo), is the same behavior on gecko. Let's close this bug.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Thanks, and I think we fix this by Bug 1361938.
Resolution: WORKSFORME → DUPLICATE
Oh, thanks!  I did fix it and you reviewed it. Funny. :)
You need to log in before you can comment on or make changes to this bug.