Bug 197808 – Move idempotent text autosizing to StyleTreeResolver

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Move idempotent text autosizing to StyleTreeResolver

[Myles C. Maxfield]

Created attachment 369628 [details]
WIP

[Myles C. Maxfield]

Created attachment 369753 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/50283983>

[Myles C. Maxfield]

#0	0x000000057f348655 in WebCore::FontCascadeDescription::setSpecifiedSize(float) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/platform/graphics/FontCascadeDescription.h:91
#1	0x0000000580836a4b in WebCore::StyleResolver::setFontSize(WebCore::FontCascadeDescription&, float) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:1892
#2	0x000000057fd1b7b2 in WebCore::StyleBuilderCustom::applyValueFontSize(WebCore::StyleResolver&, WebCore::CSSValue&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleBuilderCustom.h:1684
#3	0x000000057fd10a93 in WebCore::StyleBuilder::applyProperty(WebCore::CSSPropertyID, WebCore::StyleResolver&, WebCore::CSSValue&, bool, bool, WebCore::CSSRegisteredCustomProperty const*) at /Users/mmaxfield/Build/Products/Debug-iphonesimulator/DerivedSources/WebCore/StyleBuilder.cpp:5563
#4	0x0000000580835c05 in WebCore::StyleResolver::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue*, WebCore::ApplyCascadedPropertyState&, WebCore::SelectorChecker::LinkMatchMask) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:1791
#5	0x000000058083a073 in WebCore::StyleResolver::CascadedProperties::Property::apply(WebCore::StyleResolver&, WebCore::ApplyCascadedPropertyState&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:2349
#6	0x000000058083a67c in void WebCore::StyleResolver::applyCascadedPropertiesImpl<(WebCore::StyleResolver::CustomPropertyCycleTracking)1>(int, int, WebCore::ApplyCascadedPropertyState&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:2463
#7	0x000000058082fb27 in WebCore::StyleResolver::applyCascadedProperties(int, int, WebCore::ApplyCascadedPropertyState&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:2446
#8	0x000000058082dd12 in WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&, WebCore::Element const&, WebCore::StyleResolver::ShouldUseMatchedPropertiesCache) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:1473
#9	0x00000005808215ea in WebCore::StyleResolver::styleForElement(WebCore::Element const&, WebCore::RenderStyle const*, WebCore::RenderStyle const*, WebCore::RuleMatchingBehavior, WebCore::SelectorFilter const*) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/css/StyleResolver.cpp:399
#10	0x0000000581f49322 in WebCore::Style::TreeResolver::styleForElement(WebCore::Element&, WebCore::RenderStyle const&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/style/StyleTreeResolver.cpp:138
#11	0x0000000581f49670 in WebCore::Style::TreeResolver::resolveElement(WebCore::Element&) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/style/StyleTreeResolver.cpp:199
#12	0x0000000581f4af19 in WebCore::Style::TreeResolver::resolveComposedTree() at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/style/StyleTreeResolver.cpp:501
#13	0x0000000581f4c8a6 in WebCore::Style::TreeResolver::resolve() at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/style/StyleTreeResolver.cpp:723
#14	0x000000058099ee4a in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) at /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:1904

[Myles C. Maxfield]

Created attachment 369914 [details]
WIP

[Myles C. Maxfield]

Created attachment 369915 [details]
WIP

[Myles C. Maxfield]

Created attachment 370290 [details]
WIP

[Myles C. Maxfield]

Created attachment 370498 [details]
Patch

[EWS Watchlist]

Attachment 370498 [details] did not pass style-queue:


ERROR: Source/WebCore/css/StyleResolver.cpp:940:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 370542 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 370542 [details]
Patch

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

This needs some tests.

> Source/WebCore/css/CSSProperties.json:6231
> +        "-webkit-text-autosizing-status": {

Is this detectable at all from web content?

> Source/WebCore/css/StyleResolver.cpp:895
> +static AutosizeStatus applyStyleToAutosizeStatus(AutosizeStatus result, const RenderStyle& style)
> +{
> +    if (style.hasOutOfFlowPosition())
> +        result.foundOutOfFlowPosition = true;
> +    switch (style.display()) {
> +    case DisplayType::InlineBlock:
> +        result.foundInlineBlock = true;
> +        break;
> +    case DisplayType::None:
> +        result.foundDisplayNone = true;
> +        break;
> +    default: // FIXME: Add more cases.
> +        break;
> +    }
> +    if (style.height().isFixed())
> +        result.foundFixedHeight = true;
> +    return result;
> +}

Would be nice to group all the new text autosizing code into its own class.

> Source/WebCore/css/StyleResolver.cpp:931
> +    return false

Hmm?

> Source/WebCore/css/StyleResolver.cpp:1198
> +        for (auto* child = element->firstChild(); child; child = child->nextSibling()) {
> +            if (is<Text>(child)) {
> +                hasTextChildren = true;
> +                break;
> +            }

Would be cleaner as a separate helper function.

> Source/WebCore/css/StyleResolver.cpp:1202
> +            auto pageScale = document().page() ? document().page()->initialScale() : 1.0f;

Why do all the work if initialScale() is 1?

> Source/WebCore/rendering/style/TextSizeAdjustment.h:48
> +struct AutosizeStatus {

Enum and OptionSet<>?

[Simon Fraser (smfr)]

Comment on attachment 370542 [details]
Patch

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

>> Source/WebCore/css/CSSProperties.json:6231
>> +        "-webkit-text-autosizing-status": {
> 
> Is this detectable at all from web content?

Also we should come up with a naming convention for properties that are internal implementation detail. Like -internal-

[Myles C. Maxfield]

Created attachment 370543 [details]
WIP

[EWS Watchlist]

Comment on attachment 370543 [details]
WIP

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

New failing tests:
platform/ipad/fast/text-autosizing/text-size-adjust-inline-style.html

[EWS Watchlist]

Created attachment 370558 [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.14.5

[Myles C. Maxfield]

Created attachment 370591 [details]
WIP

[Myles C. Maxfield]

Looks like on shipping iOS on iPad, -webkit-text-size-adjust:<percentage> works, and -webkit-text-size-adjust:auto is parsed but does nothing. On shipping macOS, it isn't even parsed.

[Myles C. Maxfield]

Created attachment 370606 [details]
Patch

[Myles C. Maxfield]

Created attachment 370615 [details]
Patch

[Myles C. Maxfield]

Created attachment 370617 [details]
Patch

[Antti Koivisto]

Comment on attachment 370617 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:878
> +static inline float idempotentTextSize(float specifiedSize, float pageScale)

'static' is redundant

More of this code could be factored into TextSizeAdjustment files

> Source/WebCore/css/StyleResolver.cpp:1170
> +#if ENABLE(TEXT_AUTOSIZING)

Maybe factor this into a function like adjustRenderStyleForTextAutosizing

> Source/WebCore/css/StyleResolver.cpp:1171
> +    auto newAutosizeStatus = style.autosizeStatus().modifiedStatus(style);

This is bit odd factoring, with style appearing twice. It might read better for example as a static function, AutosizeStatus::updatedStatusForStyle(style)

> Source/WebCore/css/StyleResolver.cpp:1175
> +    if (settings().textAutosizingEnabled() && settings().textAutosizingUsesIdempotentMode() && element && !newAutosizeStatus.shouldSkipSubtree() && !style.textSizeAdjust().isNone() && hasTextChildren(*element) && pageScale != 1.0f) {
> +        auto fontDescription = style.fontDescription();

Big lists of conditions like here of often read best as bool returning lambdas.

> Source/WebCore/rendering/style/RenderStyle.h:1856
> +        // This is the state that the text autosizing code uses to determine whether or not to apply autosizing.
> +        unsigned textAutosizingFoundOutOfFlowPosition : 1;
> +        unsigned textAutosizingFoundInlineBlock : 1;
> +        unsigned textAutosizingFoundFixedHeight : 1;
> +        unsigned textAutosizingFoundDisplayNone : 1;

You could just store the OptionSet directly (with from/toRaw) to a single field

> Source/WebCore/rendering/style/TextSizeAdjustment.cpp:70
> +    return m_fields.contains(Fields::FoundOutOfFlowPosition)
> +        || m_fields.contains(Fields::FoundInlineBlock)
> +        || m_fields.contains(Fields::FoundFixedHeight)
> +        || m_fields.contains(Fields::FoundDisplayNone);

m_fields.containsAny({...});

[Myles C. Maxfield]

Committed r245838: <https://trac.webkit.org/changeset/245838>