[web-animations] keyframe values set to "inherit" should recompute their values when the inherited value changes
Created attachment 453615 [details] Patch
Comment on attachment 453615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453615&action=review > Source/WebCore/animation/KeyframeEffect.cpp:786 > + if (value.contains("inherit")) What happens if the value is "finherit"? Shouldn't the test here be for exact equality with "inherit"?
(In reply to Antti Koivisto from comment #2) > Comment on attachment 453615 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453615&action=review > > > Source/WebCore/animation/KeyframeEffect.cpp:786 > > + if (value.contains("inherit")) > > What happens if the value is "finherit"? Shouldn't the test here be for > exact equality with "inherit"? Only properties that have been successfully parsed made it this far in the code, so "finherit" would have been rejected.
However, I think we can expect to see "inherit" in a list of values, hence the contains().
> Only properties that have been successfully parsed made it this far in the > code, so "finherit" would have been rejected. I guess I don't understand what these values are. Many properties take arbitrary strings as values (say, urls).
(In reply to Antoine Quint from comment #4) > However, I think we can expect to see "inherit" in a list of values, hence > the contains(). Can' you give an example of "inherit" on a list?
(In reply to Antti Koivisto from comment #6) > (In reply to Antoine Quint from comment #4) > > However, I think we can expect to see "inherit" in a list of values, hence > > the contains(). > > Can' you give an example of "inherit" on a list? I can't actually think of one, it was more theoretical for CSSValueList. I will however add a check for CSSProperty::isInheritedProperty().
Created attachment 453739 [details] Patch
Comment on attachment 453739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453739&action=review > Source/WebCore/animation/KeyframeEffect.cpp:788 > + for (auto& [property, value] : keyframe.styleStrings) { > + if (CSSProperty::isInheritedProperty(property) && value == "inherit") > + m_inheritedProperties.add(property); > + } Shouldn't this be || rather than &&?
Comment on attachment 453739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453739&action=review >> Source/WebCore/animation/KeyframeEffect.cpp:788 >> + } > > Shouldn't this be || rather than &&? also equalLettersIgnoringASCIICase(value, "inherit")
Committed r290823 (248059@trunk): <https://commits.webkit.org/248059@trunk>
<rdar://problem/89802010>
(In reply to Antti Koivisto from comment #9) > Comment on attachment 453739 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453739&action=review > > > Source/WebCore/animation/KeyframeEffect.cpp:788 > > + for (auto& [property, value] : keyframe.styleStrings) { > > + if (CSSProperty::isInheritedProperty(property) && value == "inherit") > > + m_inheritedProperties.add(property); > > + } > > Shouldn't this be || rather than &&? Actually, use of CSSProperty::isInheritedProperty() is completely wrong here. It doesn't matter where a property on a keyframe is inherited, it matters that it uses the "inherit" keyword.