Bug 173166 – CSS transitions added while page is not visible do not start when the page becomes visible

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

CSS transitions added while page is not visible do not start when the page becomes visible.

[Chris Dumez]

<rdar://problem/32250351>

[Chris Dumez]

Created attachment 312444 [details]
WIP Patch

[Chris Dumez]

Created attachment 312449 [details]
Patch

[Darin Adler]

Comment on attachment 312449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312449&action=review

> Source/WebCore/page/animation/ImplicitAnimation.h:58
> +    void reset(const RenderStyle* to, CompositeAnimation*);

Both arguments should be references rather than pointers. Unless CompositeAnimation* can be null?

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 312449 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312449&action=review
> 
> > Source/WebCore/page/animation/ImplicitAnimation.h:58
> > +    void reset(const RenderStyle* to, CompositeAnimation*);
> 
> Both arguments should be references rather than pointers. Unless
> CompositeAnimation* can be null?

OK.

[Chris Dumez]

Created attachment 312452 [details]
Patch

[Darin Adler]

Comment on attachment 312452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312452&action=review

> Source/WebCore/page/animation/CompositeAnimation.cpp:176
>                      m_transitions.set(prop, WTFMove(implicitAnimation));

Not for this patch, I suppose, but a bit peculiar that this uses set rather than add.

[Simon Fraser (smfr)]

Comment on attachment 312452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312452&action=review

> Source/WebCore/page/animation/CompositeAnimation.cpp:173
> +                    if (m_suspended && implicitAnimation->hasStyle())
> +                        implicitAnimation->updatePlayState(AnimPlayStatePaused);

I'm slightly concerned about doing this; pausing transitions is not a very well-tested code path (and historically has led to assertions in the state machine). But if tests don't assert, I don't object.

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 312452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312452&action=review
> 
> > Source/WebCore/page/animation/CompositeAnimation.cpp:173
> > +                    if (m_suspended && implicitAnimation->hasStyle())
> > +                        implicitAnimation->updatePlayState(AnimPlayStatePaused);
> 
> I'm slightly concerned about doing this; pausing transitions is not a very
> well-tested code path (and historically has led to assertions in the state
> machine). But if tests don't assert, I don't object.

Tests do not assert.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Simon Fraser (smfr) from comment #8)
> > Comment on attachment 312452 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=312452&action=review
> > 
> > > Source/WebCore/page/animation/CompositeAnimation.cpp:173
> > > +                    if (m_suspended && implicitAnimation->hasStyle())
> > > +                        implicitAnimation->updatePlayState(AnimPlayStatePaused);
> > 
> > I'm slightly concerned about doing this; pausing transitions is not a very
> > well-tested code path (and historically has led to assertions in the state
> > machine). But if tests don't assert, I don't object.
> 
> Tests do not assert.

And to be clear, this code path was already exercised when hiding a page that contains animated CSS transitions. I am now pausing such transitions in the same way when they are created while the page is hidden.

[WebKit Commit Bot]

Comment on attachment 312452 [details]
Patch

Clearing flags on attachment: 312452

Committed r217997: <http://trac.webkit.org/changeset/217997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Claudio Saavedra]

(In reply to WebKit Commit Bot from comment #11)
> Comment on attachment 312452 [details]
> Patch
> 
> Clearing flags on attachment: 312452
> 
> Committed r217997: <http://trac.webkit.org/changeset/217997>

This patch is causing the following test to fail in several platforms:

  transitions/created-while-suspended.html [ Failure ]

See for the failure: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r218230%20(1716)/transitions/created-while-suspended-pretty-diff.html

I suspect that the expectation just needs to be updated?

[Darin Adler]

Seems possible that failure is what Chris fixed in bug 173302.

[Claudio Saavedra]

(In reply to Darin Adler from comment #14)
> Seems possible that failure is what Chris fixed in bug 173302.

I tested the patch in that bug and the test output is the same, so it doesn't seem to be the case.

[Chris Dumez]

(In reply to Claudio Saavedra from comment #13)
> (In reply to WebKit Commit Bot from comment #11)
> > Comment on attachment 312452 [details]
> > Patch
> > 
> > Clearing flags on attachment: 312452
> > 
> > Committed r217997: <http://trac.webkit.org/changeset/217997>
> 
> This patch is causing the following test to fail in several platforms:
> 
>   transitions/created-while-suspended.html [ Failure ]
> 
> See for the failure:
> https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/
> r218230%20(1716)/transitions/created-while-suspended-pretty-diff.html
> 
> I suspect that the expectation just needs to be updated?

This looks to me like a progression from my patch. Before my patch, the transition that was added while suspended would not actually be resumed when resuming animations.
The test would still pass because it relies on internals.animationsAreSuspended() which claims animations were resumed.

With my patch, you see an extra:
#### Transition ended on element with id: box

line after resuming the animations because the transition was *actually* resumed and the animation has time to finish because we wait 8 seconds after resuming the animations:
setTimeout(function () { testRunner.notifyDone();}, 8000);

So I think this test merely needs rebaselining. However, I think this test should be rewriting so it does not wait 8 seconds, this is way too slow. We can probably finish the test when the transition ends.

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Claudio Saavedra from comment #13)
> > (In reply to WebKit Commit Bot from comment #11)
> > > Comment on attachment 312452 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 312452
> > > 
> > > Committed r217997: <http://trac.webkit.org/changeset/217997>
> > 
> > This patch is causing the following test to fail in several platforms:
> > 
> >   transitions/created-while-suspended.html [ Failure ]
> > 
> > See for the failure:
> > https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/
> > r218230%20(1716)/transitions/created-while-suspended-pretty-diff.html
> > 
> > I suspect that the expectation just needs to be updated?
> 
> This looks to me like a progression from my patch. Before my patch, the
> transition that was added while suspended would not actually be resumed when
> resuming animations.
> The test would still pass because it relies on
> internals.animationsAreSuspended() which claims animations were resumed.
> 
> With my patch, you see an extra:
> #### Transition ended on element with id: box
> 
> line after resuming the animations because the transition was *actually*
> resumed and the animation has time to finish because we wait 8 seconds after
> resuming the animations:
> setTimeout(function () { testRunner.notifyDone();}, 8000);
> 
> So I think this test merely needs rebaselining. However, I think this test
> should be rewriting so it does not wait 8 seconds, this is way too slow. We
> can probably finish the test when the transition ends.

Will find some time today to improve the test.

[Chris Dumez]

(In reply to Chris Dumez from comment #17)
> (In reply to Chris Dumez from comment #16)
> > (In reply to Claudio Saavedra from comment #13)
> > > (In reply to WebKit Commit Bot from comment #11)
> > > > Comment on attachment 312452 [details]
> > > > Patch
> > > > 
> > > > Clearing flags on attachment: 312452
> > > > 
> > > > Committed r217997: <http://trac.webkit.org/changeset/217997>
> > > 
> > > This patch is causing the following test to fail in several platforms:
> > > 
> > >   transitions/created-while-suspended.html [ Failure ]
> > > 
> > > See for the failure:
> > > https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/
> > > r218230%20(1716)/transitions/created-while-suspended-pretty-diff.html
> > > 
> > > I suspect that the expectation just needs to be updated?
> > 
> > This looks to me like a progression from my patch. Before my patch, the
> > transition that was added while suspended would not actually be resumed when
> > resuming animations.
> > The test would still pass because it relies on
> > internals.animationsAreSuspended() which claims animations were resumed.
> > 
> > With my patch, you see an extra:
> > #### Transition ended on element with id: box
> > 
> > line after resuming the animations because the transition was *actually*
> > resumed and the animation has time to finish because we wait 8 seconds after
> > resuming the animations:
> > setTimeout(function () { testRunner.notifyDone();}, 8000);
> > 
> > So I think this test merely needs rebaselining. However, I think this test
> > should be rewriting so it does not wait 8 seconds, this is way too slow. We
> > can probably finish the test when the transition ends.
> 
> Will find some time today to improve the test.

https://bugs.webkit.org/show_bug.cgi?id=173370