Move idempotent text autosizing to StyleTreeResolver
Created attachment 369628 [details] WIP
Created attachment 369753 [details] WIP
<rdar://problem/50283983>
#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
Created attachment 369914 [details] WIP
Created attachment 369915 [details] WIP
Created attachment 370290 [details] WIP
Created attachment 370498 [details] Patch
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.
Created attachment 370542 [details] Patch
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 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-
Created attachment 370543 [details] WIP
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
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
Created attachment 370591 [details] WIP
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.
Created attachment 370606 [details] Patch
Created attachment 370615 [details] Patch
Created attachment 370617 [details] Patch
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({...});
Committed r245838: <https://trac.webkit.org/changeset/245838>