1379582 - Pref off frames() timing function on release channels

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Questions have been raised over the suitability of the frames() name.[1]

Unfortunately, that discussion has still not reached a conclusion. As a result Chrome have decided not to ship to release yet.[2][3] We should do the same by introducing a pref for this feature and turning it off for release/beta.

[1] https://github.com/w3c/csswg-drafts/issues/1301
[2] https://groups.google.com/a/chromium.org/d/msg/blink-dev/j7l8_DjMsVE/0HKZgZXNAQAJ
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=646265#c17

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Manual test case

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1379582 - Disable frames() timing function using a pref on release/beta channels;

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8885134 [details]
Bug 1379582 - Disable frames() timing function using a pref on release/beta channels;

https://reviewboard.mozilla.org/r/155990/#review161112

::: servo/components/style/values/specified/transform.rs:143
(Diff revision 1)
> +    unsafe { bindings::Gecko_IsFramesTimingEnabled() }
> +}
> +
> +#[cfg(feature = "servo")]
> +#[inline]
> +fn allow_frames_timing() -> bool { true }

I guess the reason why we enable this on servo by default is that servo is still experimental state?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8885134 [details]
> Bug 1379582 - Disable frames() timing function using a pref on release/beta
> channels;
> 
> https://reviewboard.mozilla.org/r/155990/#review161112
> 
> ::: servo/components/style/values/specified/transform.rs:143
> (Diff revision 1)
> > +    unsafe { bindings::Gecko_IsFramesTimingEnabled() }
> > +}
> > +
> > +#[cfg(feature = "servo")]
> > +#[inline]
> > +fn allow_frames_timing() -> bool { true }
> 
> I guess the reason why we enable this on servo by default is that servo is
> still experimental state?

Yeah, I'm really not sure how this should work but that's my understanding (looking at how we seem to have paint worklets turned on by default in Servo, but not in Gecko, since the spec is not stable yet).

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Servo PR #17682

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8885134 [details]
Bug 1379582 - Disable frames() timing function using a pref on release/beta channels;

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

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch for beta

I've prepared this patch for beta that drops the Stylo-related changes.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e619693d43133b520d53a7fc4980c6fa9307828

I'll request approval after we've successfully landed on central and assuming the try run is ok.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1772d538281
Disable frames() timing function using a pref on release/beta channels; r=hiro

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1772d538281

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8885518 [details] [diff] [review]
Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]:　https://github.com/w3c/csswg-drafts/issues/1301 (i.e. issues have been raised about a feature we were about to ship and hence we want to hold back shipping to allow the standards discussion to play out unconstrained). See https://groups.google.com/forum/#!topic/mozilla.dev.platform/mRE4cQqchDQ
[User impact if declined]:　Possible broken content in future (since 55 may ship with a feature that 56 disables). Standards process overly-constrained by shipping implementation impacts future web content.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It prefs off a feature.
[String changes made/needed]: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8885518 [details] [diff] [review]
Patch for beta

disable a new feature that's still being debated, beta55+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc07a77aa27c

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Brian Birtles (:birtles) from comment #10)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]:  No

Setting qe-verify- based on Brian's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 14

[Chris Mills [:cmills]]

I've noted this change in the browser compat data on relevant pages:

https://developer.mozilla.org/en-US/docs/Web/CSS/transition-timing-function
https://developer.mozilla.org/en-US/docs/Web/CSS/animation-timing-function
https://developer.mozilla.org/en-US/docs/Web/CSS/transition
https://developer.mozilla.org/en-US/docs/Web/CSS/animation
https://developer.mozilla.org/en-US/docs/Web/CSS/single-transition-timing-function

I mentioned it here too:
https://developer.mozilla.org/en-US/docs/Web/CSS/single-transition-timing-function#The_frames()_class_of_timing-functions

I've also add a note to the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56#CSS

Let me know if this looks OK. Thanks!

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Looks good although just to clarify, we've never shipped frames() beyond beta so I wonder how important the release note is.

Also, a minor nit on the single-transition-timing-function#The_frames()_class_of_timing-functions link, for frames(n), n must be >= 2 so the comment that reads, "There must be at least one frame" is not quite right. We can update that after the spec issue is resolved, however.

Comment 16

[Chris Mills [:cmills]]

(In reply to Brian Birtles (:birtles) from comment #15)
> Looks good although just to clarify, we've never shipped frames() beyond
> beta so I wonder how important the release note is.

ah, good point. Yes, we can delete the release note then. I've recorded it in the experimental features page for now:

https://developer.mozilla.org/en-US/Firefox/Experimental_features

> 
> Also, a minor nit on the
> single-transition-timing-function#The_frames()_class_of_timing-functions
> link, for frames(n), n must be >= 2 so the comment that reads, "There must
> be at least one frame" is not quite right. We can update that after the spec
> issue is resolved, however.

I've updated it to "at least two" for now. Happy to change it again later if needs be.