[webkit-reviews] review granted: [Bug 187722] [Web Animations] Interpolation between font-styles with a keyword value should be discrete : [Attachment 345205] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 17 17:27:36 PDT 2018
Myles C. Maxfield <mmaxfield at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 187722: [Web Animations] Interpolation between font-styles with a keyword
value should be discrete
https://bugs.webkit.org/show_bug.cgi?id=187722
Attachment 345205: Patch
https://bugs.webkit.org/attachment.cgi?id=345205&action=review
--- Comment #15 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 345205
--> https://bugs.webkit.org/attachment.cgi?id=345205
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/anim
ation-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
More information about the webkit-reviews
mailing list