Closed Bug 1574957 Opened 4 months ago Closed 4 months ago

font-size-adjust animation can cause the value to become negative

Categories

(Core :: DOM: Animation, defect, P3)

70 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: smcgruer, Assigned: boris)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.100 Safari/537.36

Steps to reproduce:

See attached reproduction; font-size-adjust animations can produce negative values for the property, against the spec (https://drafts.csswg.org/css-fonts-3/#propdef-font-size-adjust).

The reproduction uses the Web Animations API, but the exact same setup can be used for CSS Animations or Transitions; timing functions can always produce progress values outside of [0, 1].

Tested in Firefox Nightly (70.0a1 (2019-08-17) (64-bit)). The bug does not reproduce in Chrome stable (76.0.3809.100).

Component: Untriaged → DOM: Animation
Product: Firefox → Core
Priority: -- → P3

We can fix this by one-line change here?

Ideally we want to clamp values at the last minute so that one can use additive animation, and the intermediate values can go negative but then the combined result is clamped. I'm not sure if anyone does that properly though.

We do properly handle such cases (for example, color animations), I thought the line is the place we can do that, but I might be misremembering. (I haven't touched any servo code for a while. :/)

No you're probably right. I don't remember either :)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

We can fix this by one-line change here?

Yes. Something like using NonNegative<> for its Number enum arm. NonNegative clamps the values only when converting it back to the computed value (e.g. NonNegativeLengthPercentage [1]).

[1] https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/servo/components/style/values/computed/length.rs#504-516

(In reply to Boris Chiou [:boris] from comment #5)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

We can fix this by one-line change here?

Yes. Something like using NonNegative<> for its Number enum arm. NonNegative clamps the values only when converting it back to the computed value (e.g. NonNegativeLengthPercentage [1]).

[1] https://searchfox.org/mozilla-central/rev/428cf94f4ddfb80eebc6028023a7d89eb277791b/servo/components/style/values/computed/length.rs#504-516

Oh, we implement ToAnimatedValue already, so just update that line should be ok.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → boris.chiou
Attachment #9086556 - Attachment description: Bug 1574957 - Clamp negative values for font-size-adjust. → Bug 1574957 - Clamp negative values for font-size-adjust animations.

Stephen, did you add a web platform test for this case? I am asking because the patch in comment 7 contains a web platform test. Anyway, thanks for reporting this problem!

Flags: needinfo?(smcgruer)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

Stephen, did you add a web platform test for this case? I am asking because the patch in comment 7 contains a web platform test. Anyway, thanks for reporting this problem!

Err, I see the wpt: https://github.com/web-platform-tests/wpt/blob/master/css/css-fonts/animations/font-size-adjust-interpolation.html

I will drop the added test file.

We can delay this review until we get wpt synced?

Flags: needinfo?(smcgruer)

I am fine with landing before the sync.

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7e658ec1e98
Clamp negative values for font-size-adjust animations. r=hiro
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.