Closed Bug 1267547 Opened 6 years ago Closed 6 years ago

Need an automation test which checks behavior of script animations on display:none element

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: hiro, Unassigned)

Details

Attachments

(1 file)

Script animations keep running even when the target element has display:none, but should not cause any restyles.  We need a test to check this behavior.
A test case revealed that compositor animations keep running on the compositor even when the target element has display:none.  Thanks for the test!  It does not really matter for now, but we need to fix it at some point.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> A test case revealed that compositor animations keep running on the
> compositor even when the target element has display:none.  Thanks for the
> test!  It does not really matter for now, but we need to fix it at some
> point.

I might be wrong.  isRunningOnCompositor flag might be not cleared in that case?
We need to clear isRunningOnCompositor flag in nsFrame::DestroyFrom.
Currently isRunningOnCompositor flag is not cleared when 'display:none'
is set to the target element.
We need to fix it at some point.

Review commit: https://reviewboard.mozilla.org/r/49095/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49095/
Attachment #8745739 - Flags: review?(bbirtles)
Comment on attachment 8745739 [details]
MozReview Request: Bug 1267547 - Test to check script animations keep running on display:none element but does not cause restyles. r?birtles

https://reviewboard.mozilla.org/r/49095/#review45941

r=me

Please retrigger a few more times on other platforms on try to make sure we're not introducing a new intermittent here.

::: dom/animation/test/chrome/test_restyles.html:381
(Diff revision 1)
> +
> +    yield animation.ready;
> +
> +    div.style.display = 'none';
> +
> +    // We need a frame to apply display:none style.

// We need to wait a frame to apply display:none

?

(Otherwise some people might think 'need a frame' means 'need an nsFrame'?)

::: dom/animation/test/chrome/test_restyles.html:398
(Diff revision 1)
> +    is(markers.length, 0,
> +       'Script animations on "display: none" element should not update styles');
> +
> +    div.style.display = '';
> +
> +    // We need a frame to unapply display:none style.

s/need a frame/need to wait a frame/

Likewise two places in the next test.
Attachment #8745739 - Flags: review?(bbirtles) → review+
Comment on attachment 8745739 [details]
MozReview Request: Bug 1267547 - Test to check script animations keep running on display:none element but does not cause restyles. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49095/diff/1-2/
https://reviewboard.mozilla.org/r/49095/#review45941

> // We need to wait a frame to apply display:none
> 
> ?
> 
> (Otherwise some people might think 'need a frame' means 'need an nsFrame'?)

Thanks!  Actually I was sometimes confused it last year. ;-)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> We need to clear isRunningOnCompositor flag in nsFrame::DestroyFrom.

Filed bug 1268385.
https://hg.mozilla.org/mozilla-central/rev/b3087c375b1f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.