Bug 197808 - Move idempotent text autosizing to StyleTreeResolver
Summary: Move idempotent text autosizing to StyleTreeResolver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-10 18:00 PDT by Myles C. Maxfield
Modified: 2019-05-28 19:27 PDT (History)
7 users (show)

See Also:


Attachments
WIP (14.34 KB, patch)
2019-05-10 18:01 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (19.26 KB, patch)
2019-05-13 11:31 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (11.52 KB, patch)
2019-05-14 18:05 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (11.51 KB, patch)
2019-05-14 18:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (20.29 KB, patch)
2019-05-20 18:52 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (20.50 KB, patch)
2019-05-23 01:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (23.96 KB, patch)
2019-05-23 18:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (23.98 KB, patch)
2019-05-23 19:13 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.48 MB, application/zip)
2019-05-23 21:17 PDT, Build Bot
no flags Details
WIP (31.49 KB, patch)
2019-05-24 14:25 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (36.43 KB, patch)
2019-05-24 16:34 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (40.19 KB, patch)
2019-05-24 18:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (40.21 KB, patch)
2019-05-24 19:00 PDT, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-05-10 18:00:50 PDT
Move idempotent text autosizing to StyleTreeResolver
Comment 1 Myles C. Maxfield 2019-05-10 18:01:05 PDT
Created attachment 369628 [details]
WIP
Comment 2 Myles C. Maxfield 2019-05-13 11:31:03 PDT
Created attachment 369753 [details]
WIP
Comment 3 Myles C. Maxfield 2019-05-13 11:32:00 PDT
<rdar://problem/50283983>
Comment 4 Myles C. Maxfield 2019-05-13 19:51:24 PDT
#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
Comment 5 Myles C. Maxfield 2019-05-14 18:05:40 PDT
Created attachment 369914 [details]
WIP
Comment 6 Myles C. Maxfield 2019-05-14 18:10:15 PDT
Created attachment 369915 [details]
WIP
Comment 7 Myles C. Maxfield 2019-05-20 18:52:24 PDT
Created attachment 370290 [details]
WIP
Comment 8 Myles C. Maxfield 2019-05-23 01:59:42 PDT
Created attachment 370498 [details]
Patch
Comment 9 Build Bot 2019-05-23 02:02:04 PDT
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.
Comment 10 Myles C. Maxfield 2019-05-23 18:58:16 PDT
Created attachment 370542 [details]
Patch
Comment 11 Simon Fraser (smfr) 2019-05-23 19:06:08 PDT
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<>?
Comment 12 Simon Fraser (smfr) 2019-05-23 19:11:01 PDT
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-
Comment 13 Myles C. Maxfield 2019-05-23 19:13:13 PDT
Created attachment 370543 [details]
WIP
Comment 14 Build Bot 2019-05-23 21:17:43 PDT
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
Comment 15 Build Bot 2019-05-23 21:17:45 PDT
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
Comment 16 Myles C. Maxfield 2019-05-24 14:25:39 PDT
Created attachment 370591 [details]
WIP
Comment 17 Myles C. Maxfield 2019-05-24 16:11:09 PDT
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.
Comment 18 Myles C. Maxfield 2019-05-24 16:34:17 PDT
Created attachment 370606 [details]
Patch
Comment 19 Myles C. Maxfield 2019-05-24 18:44:19 PDT
Created attachment 370615 [details]
Patch
Comment 20 Myles C. Maxfield 2019-05-24 19:00:40 PDT
Created attachment 370617 [details]
Patch
Comment 21 Antti Koivisto 2019-05-26 03:32:04 PDT
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({...});
Comment 22 Myles C. Maxfield 2019-05-28 19:27:28 PDT
Committed r245838: <https://trac.webkit.org/changeset/245838>