| Summary: | [web-animations] keyframe values set to "inherit" should recompute their values when the inherited value changes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
| Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dino, graouts, koivisto, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar, WebExposed, WPTImpact | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 237471 | ||||||||
| Attachments: |
|
||||||||
|
Description
Antoine Quint
2022-03-02 08:31:26 PST
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> (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. |