Bug 187722 – [Web Animations] Interpolation between font-styles with a keyword value should be discrete

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Interpolation between font-sizes with a keyword value should be discrete

[Antoine Quint]

Created attachment 345139 [details]
Patch

[Antoine Quint]

Created attachment 345140 [details]
Patch

[Antoine Quint]

Created attachment 345156 [details]
Patch

[Antoine Quint]

Created attachment 345169 [details]
Patch

[EWS Watchlist]

Comment on attachment 345169 [details]
Patch

Attachment 345169 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8565414

New failing tests:
fast/text/font-face-javascript.html
fast/text/font-selection-font-loading-api-parse.html
accessibility/mac/search-predicate.html
fast/text/font-face-empty-string.html
accessibility/mac/search-predicate-plaintext.html

[EWS Watchlist]

Created attachment 345176 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345169 [details]
Patch

Attachment 345169 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8565321

New failing tests:
fast/text/font-face-javascript.html
fast/text/font-selection-font-loading-api-parse.html
accessibility/mac/search-predicate.html
fast/text/font-face-empty-string.html
accessibility/mac/search-predicate-plaintext.html

[EWS Watchlist]

Created attachment 345178 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 345169 [details]
Patch

Attachment 345169 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8565487

New failing tests:
fast/text/font-face-javascript.html
fast/text/font-selection-font-loading-api-parse.html
fast/text/font-face-empty-string.html

[EWS Watchlist]

Created attachment 345182 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 345169 [details]
Patch

Attachment 345169 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8565846

New failing tests:
fast/text/font-face-javascript.html
fast/text/font-selection-font-loading-api-parse.html
accessibility/mac/search-predicate.html
fast/text/font-face-empty-string.html
accessibility/mac/search-predicate-plaintext.html

[EWS Watchlist]

Created attachment 345184 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 345193 [details]
Patch

[Antoine Quint]

Created attachment 345205 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 345205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345205&action=review

> Source/WebCore/ChangeLog:3
> +        [Web Animations] Interpolation between font-sizes with a keyword value should be discrete

font-style

> Source/WebCore/ChangeLog:8
> +        Animating between "font-size: normal" and another value should yield a discrete interpolation where the from-value

"from-value" is "normal" here.

> Source/WebCore/ChangeLog:12
> +        from an "oblique" value, we implement a custom PropertyWrapper for the "font-size" property where we ensure the

font-style

> Source/WebCore/ChangeLog:15
> +        base value for "normal".

What about blending from "italic" to "oblique"?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3601
> +        && !isItalic(style.fontDescription().italic())

I don't think this is the same. Before it was comparing to 0, but now you're comparing to 20.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2043
> +    return fontNonKeywordStyleFromStyleValue(italic.value());

Seems kind of brittle, relying on fontStyleKeyword() to catch the case where italic is nullopt.

> Source/WebCore/css/CSSFontFaceSet.cpp:317
> +    auto styleSelectionValue = StyleBuilderConverter::convertFontStyleFromValue(*styleValue);

What about lines 315-316?

> Source/WebCore/css/FontSelectionValueInlines.h:126
> +    if (!style || style.value() == normalItalicValue())

Now that we can distinguish between "font-style: oblique 0deg" and "font-style: normal", I think this is not correct. I think it should be "if (!style)". We should add a layout test to make sure getComputedStyle(normal) is not the same as getComputedStyle(oblique 0deg).

> Source/WebCore/css/FontSelectionValueInlines.h:133
>  inline std::optional<FontSelectionValue> fontStyleValue(CSSValueID value)

I think this function is never called. It should be deleted.

> Source/WebCore/css/StyleBuilderConverter.h:1205
> +inline std::optional<FontSelectionValue> StyleBuilderConverter::convertFontStyleFromValue(const CSSValue& value)

This is a little confusing because a naive programmer would think "optional" means that the CSSValue is some unexpected Value. We should add a comment or something to say that the input value needs to parsed and valid, and if the function returns nullopt that means the input said "normal".

> Source/WebCore/css/StyleBuilderConverter.h:1211
>      if (valueID == CSSValueNormal)

Please add a FIXME saying that an optional is not the best data structure for this. Instead it should be a tri-state (eventually).

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:418
> +static inline std::optional<FontSelectionValue> blendFunc(const CSSPropertyBlendingClient* anim, std::optional<FontSelectionValue> from, std::optional<FontSelectionValue> to, double progress)

This is kind of unfortunate because there's nothing saying this is specific to font-style (and therefore nullopt has this special meaning).

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:420
> +    if (!from || !to)

What about blending from italic to oblique?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1465
> +        if (!fromFontItalic || !toFontItalic)

What about blending from italic to oblique?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1466
> +            blendedStyleAxis = (progress < 0.5 ? from : to)->fontDescription().fontStyleAxis();

Why the inequality? The axis gets ignored (inside blendFunc()) if progress < 0.5, so you might as well set it correctly no matter what progress is.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1470
> +        description.setItalic(blendFunc(anim, fromFontItalic, toFontItalic, progress));

Is the blendFunc() actually needed? Can't you just do it here? That would make sure the blending is associated with font-style and not any optional<FontSelectionValue>.

> Source/WebCore/platform/graphics/FontCache.h:113
> +        hasher.add(m_fontSelectionRequest.slope.value_or(normalItalicValue()));

I think we should distinguish between normal and oblique 0deg. It's just good hygiene to make me less likely to create a bug when I modify this code in the future.

> Source/WebCore/platform/graphics/FontCascade.h:155
> +    std::optional<FontSelectionValue> italic() const { return m_fontDescription.italic(); }

Again, a comment might be helpful to describe what nullopt means.

> Source/WebCore/platform/graphics/FontDescription.cpp:-58
> -    : m_fontSelectionRequest { FontCascadeDescription::initialWeight(), FontCascadeDescription::initialStretch(), FontCascadeDescription::initialItalic() }

Shouldn't we make initialItalic() return nullopt?

> Source/WebCore/platform/graphics/FontDescription.cpp:98
> +    if (isItalic)
> +        setItalic(italicValue());
> +    else
> +        setItalic(std::nullopt);

Is this really better than the ternary?

> Source/WebCore/platform/graphics/FontDescription.h:-301
> -    static FontSelectionValue initialItalic() { return normalItalicValue(); }

I would just change the type of this.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.cpp:69
> +    auto requestSlope = m_request.slope.value_or(normalItalicValue());

Cool.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:346
> +inline TextStream& operator<<(TextStream& ts, const FontSelectionValue& fontSelectionValue)
> +{
> +    ts << TextStream::FormatNumberRespectingIntegers(fontSelectionValue.rawValue());
> +    return ts;
> +}

Why wasn't this necessary before?

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:354
> +inline bool operator==(const FontSelectionRequest& a, const FontSelectionRequest& b)

Does the build fail if you change this back?

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:359
> +inline bool operator!=(const FontSelectionRequest& a, const FontSelectionRequest& b)

Does the build fail if you change this back?

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:-626
> -    auto hfont = createGDIFont(family, weight, fontDescription.italic(),

Oh, jeez, this was silently comparing to 0 because of the implicit operator float() inside FontSelectionValue? Scary.

Thanks for fixing!

> Source/WebKitLegacy/win/DOMCoreClasses.cpp:1298
> +    webFontDescription->italic = fontDescription.italic() && fontDescription.italic().value();

Pretty sure you want isItalic()

> LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/discrete-expected.txt:6
> -FAIL Test animating discrete values assert_equals: Animation produces 'from' value just before the middle of the interval expected "normal" but got "oblique 9.75deg"
> +PASS Test animating discrete values 
>  PASS Test discrete animation is used when interpolation fails 
>  PASS Test discrete animation is used only for pairs of values that cannot be interpolated 
> -FAIL Test the 50% switch point for discrete animation is based on the effect easing assert_equals: Animation produces 'from' value at 94% of the iteration time expected "normal" but got "oblique 8.5deg"
> -FAIL Test the 50% switch point for discrete animation is based on the keyframe easing assert_equals: Animation produces 'from' value at 94% of the iteration time expected "normal" but got "oblique 8.5deg"
> +PASS Test the 50% switch point for discrete animation is based on the effect easing 
> +PASS Test the 50% switch point for discrete animation is based on the keyframe easing 

YYYYAAAAYYYYYYYAYAYAYAYAY

[Myles C. Maxfield]

my biggest concern is that saying optional<FontSelectionValue> makes it sound like there may not be a FontSelectionValue, rather than a FontSelectionValue with a sentinel value

[EWS Watchlist]

Comment on attachment 345205 [details]
Patch

Attachment 345205 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8568669

New failing tests:
http/wpt/crypto/rsa-pss-crash.any.html

[EWS Watchlist]

Created attachment 345214 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Comment on attachment 345205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345205&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3601
>> +        && !isItalic(style.fontDescription().italic())
> 
> I don't think this is the same. Before it was comparing to 0, but now you're comparing to 20.

✅

[EWS Watchlist]

Comment on attachment 345205 [details]
Patch

Attachment 345205 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8571576

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 345231 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Antoine Quint]

(In reply to Myles C. Maxfield from comment #15)
> Comment on attachment 345205 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345205&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        base value for "normal".
> 
> What about blending from "italic" to "oblique"?

Yes, "italic" as either the from or to value should yield a discrete animation. I will update this patch to cover this case, which is covered by more WPT tests so we can get more progressions!

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3601
> > +        && !isItalic(style.fontDescription().italic())
> 
> I don't think this is the same. Before it was comparing to 0, but now you're
> comparing to 20.

Discussed this in person, this is correct, the ! here was important.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2043
> > +    return fontNonKeywordStyleFromStyleValue(italic.value());
> 
> Seems kind of brittle, relying on fontStyleKeyword() to catch the case where
> italic is nullopt.

Do you suggest an explicit !italic check? If so, what should the fallback be?

> > Source/WebCore/css/CSSFontFaceSet.cpp:317
> > +    auto styleSelectionValue = StyleBuilderConverter::convertFontStyleFromValue(*styleValue);
> 
> What about lines 315-316?

Will switch them to "auto" as well in a new patch.

> > Source/WebCore/css/FontSelectionValueInlines.h:126
> > +    if (!style || style.value() == normalItalicValue())
> 
> Now that we can distinguish between "font-style: oblique 0deg" and
> "font-style: normal", I think this is not correct. I think it should be "if
> (!style)". We should add a layout test to make sure getComputedStyle(normal)
> is not the same as getComputedStyle(oblique 0deg).

With the current state of this patch, they are, which is unchanged behavior. But yes, we should be able to distinguish that now. I did some testing in Firefox Nightly and Chrome Canary and there are inconsistencies with "oblique 0", "oblique 0deg" and "oblique 20deg". This is likely worthy of a new WPT test if none exists. At any rate, I think this should be covered as a targeted followup.

> > Source/WebCore/css/FontSelectionValueInlines.h:133
> >  inline std::optional<FontSelectionValue> fontStyleValue(CSSValueID value)
> 
> I think this function is never called. It should be deleted.

Indeed, removing in an updated patch.

> > Source/WebCore/css/StyleBuilderConverter.h:1205
> > +inline std::optional<FontSelectionValue> StyleBuilderConverter::convertFontStyleFromValue(const CSSValue& value)
> 
> This is a little confusing because a naive programmer would think "optional"
> means that the CSSValue is some unexpected Value. We should add a comment or
> something to say that the input value needs to parsed and valid, and if the
> function returns nullopt that means the input said "normal".

Will add a comment to that effect in an updated patch.

> > Source/WebCore/css/StyleBuilderConverter.h:1211
> >      if (valueID == CSSValueNormal)
> 
> Please add a FIXME saying that an optional is not the best data structure
> for this. Instead it should be a tri-state (eventually).

I added one in FontSelectionRequest for the slope value with a link to webkit.org/b/187774.

> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:418
> > +static inline std::optional<FontSelectionValue> blendFunc(const CSSPropertyBlendingClient* anim, std::optional<FontSelectionValue> from, std::optional<FontSelectionValue> to, double progress)
> 
> This is kind of unfortunate because there's nothing saying this is specific
> to font-style (and therefore nullopt has this special meaning).

I'll add a comment here to explain it's the dedicated blendFunc for that property.

> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:420
> > +    if (!from || !to)
> 
> What about blending from italic to oblique?
> 
> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1465
> > +        if (!fromFontItalic || !toFontItalic)
> 
> What about blending from italic to oblique?

I will introduce a canInterpolate() method for PropertyWrapperFontStyle which will take care of that.

> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1466
> > +            blendedStyleAxis = (progress < 0.5 ? from : to)->fontDescription().fontStyleAxis();
> 
> Why the inequality? The axis gets ignored (inside blendFunc()) if progress <
> 0.5, so you might as well set it correctly no matter what progress is.

We discussed this in person, and now that we need to handle discrete animations not just when a "normal" value is present, but also an "italic" value, this is the right approach.

> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1470
> > +        description.setItalic(blendFunc(anim, fromFontItalic, toFontItalic, progress));
> 
> Is the blendFunc() actually needed? Can't you just do it here? That would
> make sure the blending is associated with font-style and not any
> optional<FontSelectionValue>.

Actually, we do need the blendFunc as part of the CSSPropertyAnimation machinery. However, I will update the blendFunc to not have the discrete animation part so that it's generic.

> > Source/WebCore/platform/graphics/FontCache.h:113
> > +        hasher.add(m_fontSelectionRequest.slope.value_or(normalItalicValue()));
> 
> I think we should distinguish between normal and oblique 0deg. It's just
> good hygiene to make me less likely to create a bug when I modify this code
> in the future.

I think we should handle this in the followup where we tackle the computed style issue above.

> > Source/WebCore/platform/graphics/FontCascade.h:155
> > +    std::optional<FontSelectionValue> italic() const { return m_fontDescription.italic(); }
> 
> Again, a comment might be helpful to describe what nullopt means.
> 
> > Source/WebCore/platform/graphics/FontDescription.cpp:-58
> > -    : m_fontSelectionRequest { FontCascadeDescription::initialWeight(), FontCascadeDescription::initialStretch(), FontCascadeDescription::initialItalic() }
> 
> Shouldn't we make initialItalic() return nullopt?

I suppose we could do that as well. Will change in an upcoming patch.

> > Source/WebCore/platform/graphics/FontDescription.cpp:98
> > +    if (isItalic)
> > +        setItalic(italicValue());
> > +    else
> > +        setItalic(std::nullopt);
> 
> Is this really better than the ternary?

I'll use a ternary with explicit std::optional<FontSelectionValue> constructors and move the function back to the header file.

> > Source/WebCore/platform/graphics/FontDescription.h:-301
> > -    static FontSelectionValue initialItalic() { return normalItalicValue(); }
> 
> I would just change the type of this.
> 
> > Source/WebCore/platform/graphics/FontSelectionAlgorithm.cpp:69
> > +    auto requestSlope = m_request.slope.value_or(normalItalicValue());
> 
> Cool.
> 
> > Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:346
> > +inline TextStream& operator<<(TextStream& ts, const FontSelectionValue& fontSelectionValue)
> > +{
> > +    ts << TextStream::FormatNumberRespectingIntegers(fontSelectionValue.rawValue());
> > +    return ts;
> > +}
> 
> Why wasn't this necessary before?

Honestly, I'm not certain.

> > Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:354
> > +inline bool operator==(const FontSelectionRequest& a, const FontSelectionRequest& b)
> 
> Does the build fail if you change this back?

Yes.

> > Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:359
> > +inline bool operator!=(const FontSelectionRequest& a, const FontSelectionRequest& b)
> 
> Does the build fail if you change this back?

Yes.

> > Source/WebCore/platform/graphics/win/FontCacheWin.cpp:-626
> > -    auto hfont = createGDIFont(family, weight, fontDescription.italic(),
> 
> Oh, jeez, this was silently comparing to 0 because of the implicit operator
> float() inside FontSelectionValue? Scary.
> 
> Thanks for fixing!
> 
> > Source/WebKitLegacy/win/DOMCoreClasses.cpp:1298
> > +    webFontDescription->italic = fontDescription.italic() && fontDescription.italic().value();
> 
> Pretty sure you want isItalic()

That would be better.

> > LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/discrete-expected.txt:6
> > -FAIL Test animating discrete values assert_equals: Animation produces 'from' value just before the middle of the interval expected "normal" but got "oblique 9.75deg"
> > +PASS Test animating discrete values 
> >  PASS Test discrete animation is used when interpolation fails 
> >  PASS Test discrete animation is used only for pairs of values that cannot be interpolated 
> > -FAIL Test the 50% switch point for discrete animation is based on the effect easing assert_equals: Animation produces 'from' value at 94% of the iteration time expected "normal" but got "oblique 8.5deg"
> > -FAIL Test the 50% switch point for discrete animation is based on the keyframe easing assert_equals: Animation produces 'from' value at 94% of the iteration time expected "normal" but got "oblique 8.5deg"
> > +PASS Test the 50% switch point for discrete animation is based on the effect easing 
> > +PASS Test the 50% switch point for discrete animation is based on the keyframe easing 
> 
> YYYYAAAAYYYYYYYAYAYAYAYAY

[Antoine Quint]

Created attachment 345269 [details]
Patch

[Antoine Quint]

Created attachment 345273 [details]
Patch

[Antoine Quint]

Created attachment 345283 [details]
Patch

[Antoine Quint]

Committed r233935: <https://trac.webkit.org/changeset/233935>

[Radar WebKit Bug Importer]

<rdar://problem/42354557>