1457107 - During animation, the CSS visibility is improperly inherited by children (since Firefox 60)

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect)

Description

[CoolCmd]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Install Firefox 60 BETA.
Load the attached test case.
Click the checkbox.


Actual results:

A red rectangle is immediately displayed. After 0.5 second, the content (text) of the rectangle displayed.



Expected results:

Rectangle and its contents should displayed simultaneously, like in Firefox 59, Chrome and Edge.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It sounds a regression I did. :/  I will take a look into this tomorrow.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Heh, was taking the regression range too:

  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4403805b3b66f4a130f21d0d2ae036b8fc9d6f5b&tochange=e43f2f6ea111c2d059d95fa9a71516b869a69698

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I thought I wrote some test cases of visibility animation, but apparently I forgot to write a test case that the visibility animation is in-view.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6ec1d5ea00cf5e6fd1eecdb80b952adcb2f1d83

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

In bug 1237454, we introduced a new change hint that is produced when
visibility property is changed from 'visible' to 'hidden' or from 'hidden'
to 'visible'.  Animations producing this new change hint can be throttled
if the animation element is out-of-view, but can not be throttled in the case
where the element is in-view. In the bug, a test case for a visibility change
animation on out-of-view element was added, but no test for in-view case was
added.

Review commit: https://reviewboard.mozilla.org/r/240204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240204/

Comment 6

[Julien Cristau [:jcristau]]

:hiro are you going to ask for review on this?  Also, how severe is the issue, and should we hold 60 for this (we're at release candidate stage), or is this minor enough that it can wait for 61?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Yep, I think we should uplift to 60.
Brian, would you mind taking time for review this?

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8971454 [details]
Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

https://reviewboard.mozilla.org/r/240204/#review247158

::: dom/animation/KeyframeEffectReadOnly.h:430
(Diff revision 1)
>                                      nsChangeHint_AddOrRemoveTransform |
>                                      nsChangeHint_UpdateTransformLayer);
>    }
> +
> +  // Returns true if this effect causes visibility change.
> +  // (i.e. 'visibility: hidden' -> 'visibility: visible' and vise versa.)

s/vise/vice/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8971454 [details]
Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bd14318ff8e
Don't throttle visibility change animations if the animation element isn't on out-of-view element. r=birtles

Comment 12

[Julien Cristau [:jcristau]]

Please request uplift to mozilla-release ASAP as I'm hoping to build 60 rc2 today.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8971454 [details]
Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1458457
[User impact if declined]: Visibility animation doesn't work as expected (it means the user won't see the animating element on half of animation frames)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet, just landed on autoland
[Needs manual test from QE? If yes, steps to reproduce]:  Nope
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]:  Pretty low risk
[Why is the change risky/not risky?]: It just adds an additional check that visibility animation is accidentally skipped to be restyled.
[String changes made/needed]: None

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8971454 [details]
Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 15

[Julien Cristau [:jcristau]]

Given this is so last minute I'm going to ask for QE verification anyway (comment 0 for STR).

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8971454 [details]
Bug 1457107 - Don't throttle visibility change animations if the animation element isn't on out-of-view element.

animation regression fix, with added testcase, let's take this in 60.0 rc2.

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/c6c8e7bded8b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/c6c8e7bded8b

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5bd14318ff8e

Comment 20

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly (2018-04-26) on Windows 10 x64.
I retested everything using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 on Firefox 60.0 - build 2, 60.0esr - build 6 and latest Nightly. The bug is not reproducing anymore. 

On the other hand, it takes a while for the red rectangle to disappear after you uncheck the box. Is that expected behaviour or there is another reason for this delay?

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Yes, exactly.

From https://developer.mozilla.org/en-US/docs/Web/CSS/visibility#Interpolation

> where values of the timing function between 0 and 1 map to visible and other values of the timing function (which occur only at the start/end of the transition or as a result of cubic-bezier() functions with y values outside of [0, 1]) map to the closer endpoint.

So in the test case, when checking the box, 'transition: hidden -> visible' happens, thus the red box appears soon, whereas when unchecking the box, 'transition: visible -> hidden' happens, thus visibility:hidden is applied only at the end of the transition.

Thanks for confirming!

Comment 22

[Oana Botisan, Desktop QA]

Considering the information from comment 20 and comment 21, I will mark this bug as verified fixed.