Closed Bug 1457107 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Animation, defect)

60 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

(Reporter: CoolCmd, Assigned: hiro)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file testcase.html (deleted) —
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.
Version: 59 Branch → 60 Branch
It sounds a regression I did. :/ I will take a look into this tomorrow.
Flags: needinfo?(hikezoe)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Component: Layout → DOM: Animation
Flags: needinfo?(hikezoe)
: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?
Flags: needinfo?(hikezoe)
Yep, I think we should uplift to 60. Brian, would you mind taking time for review this?
Flags: needinfo?(hikezoe) → needinfo?(bbirtles)
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/
Attachment #8971454 - Flags: review+
Flags: needinfo?(bbirtles)
Thank you!
Pushed by hikezoe@mozilla.com: 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
Please request uplift to mozilla-release ASAP as I'm hoping to build 60 rc2 today.
Flags: needinfo?(hikezoe)
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
Flags: needinfo?(hikezoe)
Attachment #8971454 - Flags: approval-mozilla-beta?
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]:
Attachment #8971454 - Flags: approval-mozilla-release?
Given this is so last minute I'm going to ask for QE verification anyway (comment 0 for STR).
Flags: qe-verify+
Flags: in-testsuite+
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.
Attachment #8971454 - Flags: approval-mozilla-release?
Attachment #8971454 - Flags: approval-mozilla-release+
Attachment #8971454 - Flags: approval-mozilla-esr60+
Attachment #8971454 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
Flags: needinfo?(hikezoe)
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!
Flags: needinfo?(hikezoe)
Considering the information from comment 20 and comment 21, I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: