[web-animations] keyframe values set to "inherit" should recompute their values when the inherited value changes

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[web-animations] keyframe values set to "inherit" should recompute their values when the inherited value changes

[Antoine Quint]

Created attachment 453615 [details]
Patch

[Antti Koivisto]

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"?

[Antoine Quint]

(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.

[Antoine Quint]

However, I think we can expect to see "inherit" in a list of values, hence the contains().

[Antti Koivisto]

> 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).

[Antti Koivisto]

(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?

[Antoine Quint]

(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().

[Antoine Quint]

Created attachment 453739 [details]
Patch

[Antti Koivisto]

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 &&?

[Antti Koivisto]

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")

[Antoine Quint]

Committed r290823 (248059@trunk): <https://commits.webkit.org/248059@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/89802010>

[Antoine Quint]

(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.