Bug 194765

Summary: Animation of font-size with rem values is incorrect
Product: WebKit Reporter: Scott Kellum <scott>
Component: AnimationsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: dino, graouts, graouts, koivisto, litherum, mmaxfield, scott, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: All   
OS: Unspecified   
URL: https://codepen.io/scottkellum/pen/28d147aa4b21d84367ce832a986cd549?editors=0100
See Also: https://bugs.webkit.org/show_bug.cgi?id=194749
Attachments:
Description Flags
Test
none
Test
none
patch
graouts: review+
patch none

Description Scott Kellum 2019-02-17 13:28:08 PST
Rem units are intermittently miscalculated on load within animations. This happens in Safari 12 both on my Mac and iOS. Occasionally it will render correctly but on refreshes it more often than not renders incorrectly.

Replicate the bug:
https://codepen.io/scottkellum/pen/28d147aa4b21d84367ce832a986cd549?editors=1100


Possibly related: https://bugs.webkit.org/show_bug.cgi?id=194749


If the animation is toggled off and back on or set late with javascript it will work again.
Comment 1 Radar WebKit Bug Importer 2019-02-18 11:30:48 PST
<rdar://problem/48171742>
Comment 2 Simon Fraser (smfr) 2019-02-18 11:32:27 PST
Seems to only reproduce with Web Animations enabled.
Comment 3 Antoine Quint 2020-05-18 05:27:51 PDT
Created attachment 399640 [details]
Test
Comment 4 Antoine Quint 2020-05-18 05:30:30 PDT
If you remove the animation-delay and animation-play-state values in the animation shorthand you can see the animation running and using what looks like `font-size: 1rem` for the 50% keyframe.
Comment 5 Antoine Quint 2020-05-18 05:51:53 PDT
If you change the keyframes to this:

@keyframes anim {

    0% {
        font-size: 2rem;
    }

    50% {
        font-size: 6rem;
    }

    100% {
        font-size: 2rem;
    }

}

This sets the value to what looks like 0.5rem for the duration of the entire animation. So setting explicit 0% and 100% keyframes show different results.
Comment 6 Antoine Quint 2020-05-18 06:26:04 PDT
The way we compute the styles on the KeyframeListValue objects is all wrong, which explains why we get no animation for the explicit keyframes case, they're all resolving to the same value.
Comment 7 Antoine Quint 2020-05-18 06:26:51 PDT
This happens because CSSPrimitiveValue::computeNonCalcLengthDouble() has no conversionData.rootStyle().
Comment 8 Antoine Quint 2020-05-18 06:55:34 PDT
Created attachment 399643 [details]
Test
Comment 9 Antti Koivisto 2020-05-19 07:37:35 PDT
Created attachment 399733 [details]
patch
Comment 10 Antoine Quint 2020-05-19 07:49:45 PDT
Comment on attachment 399733 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399733&action=review

> LayoutTests/animations/keyframe-rem-unit.html:20
> +    testRunner.waitUntilDone();
> +    await document.getAnimations()[0].finished;
> +    testRunner.notifyDone();

If you write an animation that use a rem unit in the "from" keyframe then you could write this to only wait for the animation's "ready" promise which would run the test one animation frame faster I think. Hopefully that works.
Comment 11 Antoine Quint 2020-05-19 07:50:50 PDT
Does this patch also fix bug 194749?
Comment 12 Antti Koivisto 2020-05-19 08:06:29 PDT
Created attachment 399735 [details]
patch
Comment 13 Antti Koivisto 2020-05-19 08:20:49 PDT
(In reply to Antoine Quint from comment #11)
> Does this patch also fix bug 194749?

Nope, but I think it is related. The base style for 'em' resolution is similarly wrong (it should be the parent element style).
Comment 14 EWS 2020-05-19 09:00:35 PDT
Committed r261861: <https://trac.webkit.org/changeset/261861>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399735 [details].