Summary: | Move idempotent text autosizing to StyleTreeResolver | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | dino, ews-watchlist, jonlee, koivisto, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2019-05-10 18:00:50 PDT
Created attachment 369628 [details]
WIP
Created attachment 369753 [details]
WIP
#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> |