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

Scott Kellum
Reported 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.
Attachments
Test (428 bytes, text/html)
2020-05-18 05:27 PDT, Antoine Quint 		no flags
Details
Test (247 bytes, text/html)
2020-05-18 06:55 PDT, Antoine Quint 		no flags
Details
patch (3.15 KB, patch)
2020-05-19 07:37 PDT, Antti Koivisto 		graouts: review+
DetailsFormatted DiffDiff
patch (2.98 KB, patch)
2020-05-19 08:06 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-02-18 11:30:48 PST
<rdar://problem/48171742>
Simon Fraser (smfr)
Comment 2 2019-02-18 11:32:27 PST
Seems to only reproduce with Web Animations enabled.
Antoine Quint
Comment 3 2020-05-18 05:27:51 PDT
Created attachment 399640 [details] Test
Antoine Quint
Comment 4 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.
Antoine Quint
Comment 5 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.
Antoine Quint
Comment 6 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.
Antoine Quint
Comment 7 2020-05-18 06:26:51 PDT
This happens because CSSPrimitiveValue::computeNonCalcLengthDouble() has no conversionData.rootStyle().
Antoine Quint
Comment 8 2020-05-18 06:55:34 PDT
Created attachment 399643 [details] Test
Antti Koivisto
Comment 9 2020-05-19 07:37:35 PDT
Created attachment 399733 [details] patch
Antoine Quint
Comment 10 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.
Antoine Quint
Comment 11 2020-05-19 07:50:50 PDT
Does this patch also fix bug 194749?
Antti Koivisto
Comment 12 2020-05-19 08:06:29 PDT
Created attachment 399735 [details] patch
Antti Koivisto
Comment 13 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).
EWS
Comment 14 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].
Note You need to log in before you can comment on or make changes to this bug.