251911 2023-02-08 02:27:27 -0800 [web-animations] line-height should not transition from default value to a number 2023-02-08 12:34:09 -0800 1 1 1 Unclassified WebKit Animations WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://github.com/web-platform-tests/wpt/pull/38416 InRadar P2 Normal --- 1 graouts graouts dino graouts webkit-bug-importer oldest_to_newest 1931969 0 464906 graouts 2023-02-08 02:27:27 -0800 Created attachment 464906 Reduction We incorrectly interpolate the "line-height" property when it animates from its initial value (normal) to a number. This was reported in https://stackoverflow.com/questions/75151629/css-transitions-in-safari/. 1931970 1 graouts 2023-02-08 02:27:47 -0800 rdar://104346766 1931972 2 graouts 2023-02-08 02:33:24 -0800 The problem is that BuilderConverter::convertLineHeight() loses the `<number>` type here: if (primitiveValue.isNumber()) return Length(primitiveValue.doubleValue() * 100.0, LengthType::Percent); Additionally, the `normal` value is also converted to a percent value here: if (valueID == CSSValueNormal) return RenderStyle::initialLineHeight(); Where RenderStyle::initialLineHeight() builds this value: // Returning -100% percent here means the line-height is not set. static Length initialLineHeight() { return Length(-100.0f, LengthType::Percent); } This means that when we get to deciding whether it's OK to interpolate between `normal` and `0`, which is what the original Stack Overflow example does, we consider two percent values and decide it's OK to interpolate. We are going to need to: 1. identify the `normal` value with special logic 2. preserve the `number` type somehow Finally, looking at the spec for `line-height` I see it takes in `<length-percentage>` as a value and yet the animation wrapper does not specify this. So there's likely another bug here where we would fail to yield a transition between "line-height: 10px" and "line-height: 10%" I expect. 1931973 3 graouts 2023-02-08 02:38:01 -0800 Actually, a transition between "line-height: 10px" and "line-height: 10%" works because we convert the "10%" to a Fixed value as well so we end up having the same type. This code is pretty weird :) 1931978 4 graouts 2023-02-08 03:16:00 -0800 Alright, so all we need to do is to have a custom `canInterpolate()` override for `line-height` and return false if any value maps to RenderStyle::initialLineHeight(). The default LengthPropertyWrapper logic will otherwise apply since <number> values map to LengthType::Percent and <length-percentage> values map to LengthType::Fixed. 1931983 5 graouts 2023-02-08 04:55:26 -0800 Pull request: https://github.com/WebKit/WebKit/pull/9807 1931984 6 graouts 2023-02-08 04:55:53 -0800 Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/38416 1932109 7 ews-feeder 2023-02-08 12:34:04 -0800 Committed 260028@main (221eb1d58b67): <https://commits.webkit.org/260028@main> Reviewed commits have been landed. Closing PR #9807 and removing active labels. 464906 2023-02-08 02:27:27 -0800 2023-02-08 02:27:27 -0800 Reduction line-height-transition-from-default-to-number.html text/html 424 graouts PHN0eWxlPgoKICAgIGRpdiB7CiAgICAgICAgdHJhbnNpdGlvbjogbGluZS1oZWlnaHQgMXM7CiAg ICB9CgogICAgZGl2LmNoYW5nZWQgewogICAgICAgIGxpbmUtaGVpZ2h0OiA1OwogICAgfQoKPC9z dHlsZT4KCjxkaXY+PC9kaXY+Cgo8c2NyaXB0PgoKY29uc3QgdGFyZ2V0ID0gZG9jdW1lbnQucXVl cnlTZWxlY3RvcigiZGl2Iik7CnNldFRpbWVvdXQoKCkgPT4gewogICAgdGFyZ2V0LmNsYXNzTmFt ZSA9ICJjaGFuZ2VkIjsKICAgIGNvbnNvbGUubG9nKGBudW1iZXIgb2YgcnVubmluZyBhbmltYXRp b25zID0gJHtkb2N1bWVudC5nZXRBbmltYXRpb25zKCkubGVuZ3RofWApOwogICAgY29uc29sZS5s b2coYGNvbXB1dGVkIGxpbmUtaGVpZ2h0ID0gJHtnZXRDb21wdXRlZFN0eWxlKHRhcmdldCkubGlu ZUhlaWdodH1gKTsKfSkKCjwvc2NyaXB0Pg==