Closed Bug 1264396 Opened 8 years ago Closed 8 years ago

Display CSS animation freezes Firefox

Categories

(Core :: CSS Parsing and Computation, defect)

48 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- verified

People

(Reporter: Oriol, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: hang, regression)

Attachments

(2 files, 1 obsolete file)

Attached file testcase
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160413030239

Steps to reproduce:

Open testcase.


Actual results:

Firefox freezes.


Expected results:

Firefox should not freeze.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3baed5c339e31188949641cb7bc7178a71070718&tochange=52c0128aff10f4c50ae3ab34c64cef8844497942
Blocks: 1260655
Flags: needinfo?(bbirtles)
Keywords: regression
Yeah, I suspected this might happen in bug 1260655 comment 11 but couldn't find a test case to produce it. We'll need to make display not animatable.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bbirtles)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
MozReview-Commit-ID: HOVHXfoFv5l
MozReview-Commit-ID: HOVHXfoFv5l
Attachment #8741582 - Flags: review?(cam)
Attachment #8741261 - Attachment is obsolete: true
Regarding the spec status of this, in bug 1064937 we're going to do an investigation into which properties are animatable in different implementations, send the list to www-style, then update all the CSS specs to indicate which properties are never animatable vs properties that are animatable using 'discrete' animation. See, in particular, bug 1064937 comment 4.

In discussion with Google, however, we agree at least that we shouldn't animate CSS animation/transition properties and 'display'. With regards to 'display', it is problematic for at least CSS animations and CSS transitions since it performs playback control there. In discussion with Google earlier in the year they were concerned that it was problematic even for just script-generated animations. I think it's simplest to just disallow 'display' everywhere for now. If it proves possible and desirable to allow it for script-generated animations we can remove the restriction then.
Comment on attachment 8741582 [details] [diff] [review]
Don't allow animation of 'display' property

Review of attachment 8741582 [details] [diff] [review]:
-----------------------------------------------------------------

Please make sure to file a bug on the specs to prevent animation of display.
Attachment #8741582 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)
> Comment on attachment 8741582 [details] [diff] [review]
> Don't allow animation of 'display' property
> 
> Review of attachment 8741582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make sure to file a bug on the specs to prevent animation of display.

Unfortunately, we don't have a concept of 'not animatable' vs 'discrete animation' in CSS yet. CSS simply has 'Animatable: no' which is ambiguous as to which it means. So the spec bug is basically [1] where I need to go and change all CSS specs from using 'Animatable' to 'Animation type' and introduce the 'none' and 'discrete' types. I've added a note to that issue to make sure 'display' gets the 'none' animation type for this.

[1] https://github.com/w3c/csswg-drafts/issues/72
Thanks, sounds good.
https://hg.mozilla.org/mozilla-central/rev/940293404626
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
QA Whiteboard: [good first verify]
I reproduced the initial issue on old Nightly (build ID: 20160413030239).
I verified that the issue is fixed using Firefox-48.0b5 on Windows 10 x64, Ubuntu 14.04 x32, Mac 10.11.
Thanks Cipri for testing!
Changing tracking flag for 48 to verified and closing this bug as Verified Fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.