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

Reporter

Description

•

6 years 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)

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.

Boris Chiou [:boris]

Reporter

Comment 2

•

6 years 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)?

Boris Chiou [:boris]

Reporter

Comment 3

•

6 years 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.

Boris Chiou [:boris]

Reporter

Updated

•

6 years ago

Blocks: 1337599

Comment hidden (obsolete)

(In reply to Boris Chiou [:boris] from comment #3)
> (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.

Also, I suspect this function, nsDOMWindowUtils::AdvanceTimeAndRefresh(), should trigger pending animations for stylo because we do that by PendingAnimationTracker for Gecko [1].

[1] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/base/nsDOMWindowUtils.cpp#2419-2434

Hiroyuki Ikezoe (:hiro)

Comment 5

•

6 years ago

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.

Hiroyuki Ikezoe (:hiro)

Comment 6

•

6 years ago

(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)*

Boris Chiou [:boris]

Reporter

Comment 7

•

6 years 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?

Boris Chiou [:boris]

Reporter

Comment 8

•

6 years 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;

Boris Chiou [:boris]

Reporter

Comment 9

•

6 years 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

Hiroyuki Ikezoe (:hiro)

Comment 10

•

6 years ago

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

Boris Chiou [:boris]

Reporter

Comment 11

•

6 years 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.

Hiroyuki Ikezoe (:hiro)

Comment 12

•

6 years ago

(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.

Bobby Holley (:bholley)

Updated

•

6 years ago

Priority: P2 → --

Brian Birtles (:birtles)

Comment 13

•

6 years ago

P2.5

Priority: -- → P3

Hiroyuki Ikezoe (:hiro)

Comment 14

•

6 years ago

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)

Boris Chiou [:boris]

Reporter

Comment 15

•

6 years ago

(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)

Hiroyuki Ikezoe (:hiro)

Comment 16

•

6 years ago

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: 6 years ago

Resolution: --- → WORKSFORME

Boris Chiou [:boris]

Reporter

Comment 17

•

6 years ago

Thanks, and I think we fix this by Bug 1361938.

Boris Chiou [:boris]

Reporter

Updated

•

6 years ago

Resolution: WORKSFORME → DUPLICATE

Hiroyuki Ikezoe (:hiro)

Comment 19

•

6 years ago

Oh, thanks!  I did fix it and you reviewed it. Funny. :)

Bugzilla

Quick Search

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)

Tracking

()

People

(Reporter: boris, Unassigned)

References

Details

Crash Data

Security

(public)

User Story

Description

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Updated

Comment 13

Comment 14

Comment 15

Comment 16

Comment 17

Updated

Comment 19