Attachment 372811 Details for Bug 198763 – Less code, more words

[patch] Less code, more words

bug-198763-20190624172139.patch (text/plain), 19.41 KB, created by Wenson Hsieh on 2019-06-24 17:21:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-06-24 17:21:39 PDT

Size: 19.41 KB

patch

obsolete

>Subversion Revision: 246765
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 44bf9b656e440ba411f56452db5bc696e881dd1b..d8c07b6a2c708070b76f9ca2b724795db4cb15bd 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,68 @@
>+2019-06-24  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Text autosizing] [iPadOS] Revise our heuristics to determine idempotent text autosizing candidates
>+        https://bugs.webkit.org/show_bug.cgi?id=198763
>+        <rdar://problem/51826266>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch adjusts existing text autosizing heuristics, based on a survey of text on websites in the Alexa top
>+        500 that shrink down to fit the viewport when requesting the desktop version of the site. The new heuristic is
>+        derived from training decision trees against the dataset obtained from this survey, and balances false positives
>+        (cases where layout is broken due to autosizing) against overall accuracy (measured using cross-validation).
>+
>+        See below for more details. Additionally, please refer to the link in the radar for more details, as well as
>+        resources used to generate, validate, and analyze these decision trees.
>+
>+        Test: fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html
>+
>+        * css/StyleResolver.cpp:
>+        (WebCore::StyleResolver::adjustRenderStyleForTextAutosizing):
>+        * rendering/style/RenderStyle.cpp:
>+        (WebCore::RenderStyle::isIdempotentTextAutosizingCandidate const):
>+
>+        Rename AutosizeStatus::shouldSkipSubtree to RenderStyle::isIdempotentTextAutosizingCandidate. We relocate this
>+        logic to RenderStyle, such that we're able to ask the element's RenderStyle questions when determining whether
>+        the element should be autosized.
>+
>+        Of course, this patch additionally revamps the heuristic used to determine whether it is safe to autosize an
>+        element. Our current heuristic in trunk simply checks for the presence of inline block display, out of flow
>+        positioning and a fixed height ancestor; if any of these conditions are satisfied, we opt the element out of
>+        text autosizing. This is an excellent strategy for boosting some runs of text while avoiding autosizing in the
>+        vast majority of cases where increasing font size may lead to layout breakage (e.g. overlapping or clipped text,
>+        content unexpectedly flowing to the next line, etc.). However, it also avoids boosting font sizes in many
>+        scenarios where boosting font sizes is desired; for concrete examples, see the (currently 24) radars about small
>+        font sizes that are duped to <rdar://problem/51826266>.
>+
>+        To help analyze and identify trends in autosizable and non-autosizable text, we assembled a dataset of elements
>+        with text from the Alexa top 500 that either: (1) were too small and could be boosted safely, or (2) would break
>+        layout if boosted. With this labeled dataset, we then trained binary decision trees to classify the data. Each
>+        decision tree was trained with a number of hyperparameters: namely, maximum depth, minimum leaf size, and the
>+        amount of bias towards negative samples (i.e. the ratio of the weight of a non-autosizable sample relative to
>+        the weight of an autosizable sample).
>+
>+        For each 3-tuple of these hyperparameters (800 in total: max depth between 3 and 10, min leaf size between 1 and
>+        10 and bias between 1 and 10), for 5000 iterations each, we split the full dataset into a training dataset and
>+        a cross-validation dataset, trained a decision tree using the training set, and tested against the cross-
>+        validation set to compute average precision, recall, and overall accuracy for each tuple of hyperparameters.
>+
>+        The decision tree introduced in this patch was generated using a hand-picked set of hyperparameters (max depth
>+        10, min leaf size 4, and negative bias 2) to provide a balance between precision scores (limiting layout
>+        breakage) and recall score (ensuring that small text is mostly autosized), while optimizing for overall
>+        accuracy. Cross-validation scores predict that the overall accuracy of this classifier is approximately 70%, up
>+        from the current accuracy in trunk (~53%).
>+
>+        * rendering/style/RenderStyle.h:
>+
>+        Grow the width of `autosizeStatus` from 4 to 8 (notably, this does not increase the size of RenderStyle).
>+
>+        * rendering/style/TextSizeAdjustment.cpp:
>+        (WebCore::AutosizeStatus::updateStatus):
>+        (WebCore::AutosizeStatus::shouldSkipSubtree const): Deleted.
>+        * rendering/style/TextSizeAdjustment.h:
>+
>+        Introduce new text autosizing state flags, and remove some existing ones.
>+
> 2019-06-24  Chris Dumez  <cdumez@apple.com>
> 
>         Pages using Google's anti-flicker optimization may take ~5 seconds to do initial paint
>diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
>index 0248790eeb0a8823eaaa6a926046bbc488b01569..b3e6e1415f55af5aec5123f3ec90155ff2ebc2c0 100644
>--- a/Source/WebCore/css/StyleResolver.cpp
>+++ b/Source/WebCore/css/StyleResolver.cpp
>@@ -879,17 +879,17 @@ static bool hasTextChildren(const Element& element)
> 
> void StyleResolver::adjustRenderStyleForTextAutosizing(RenderStyle& style, const Element& element)
> {
>-    auto newAutosizeStatus = AutosizeStatus::updateStatus(style);
>     if (!settings().textAutosizingEnabled() || !settings().textAutosizingUsesIdempotentMode())
>         return;
> 
>+    AutosizeStatus::updateStatus(style);
>     if (!hasTextChildren(element))
>         return;
> 
>     if (style.textSizeAdjust().isNone())
>         return;
> 
>-    if (newAutosizeStatus.shouldSkipSubtree())
>+    if (!style.isIdempotentTextAutosizingCandidate())
>         return;
> 
>     float initialScale = document().page() ? document().page()->initialScale() : 1;
>diff --git a/Source/WebCore/rendering/style/RenderStyle.cpp b/Source/WebCore/rendering/style/RenderStyle.cpp
>index 14b1ca03874bc8d7261673d4071a23b74fb6ff77..0d4597213fd611e4cc43455a5df47697521c2ebd 100644
>--- a/Source/WebCore/rendering/style/RenderStyle.cpp
>+++ b/Source/WebCore/rendering/style/RenderStyle.cpp
>@@ -492,6 +492,32 @@ bool RenderStyle::equalForTextAutosizing(const RenderStyle& other) const
>         && m_rareNonInheritedData->textOverflow == other.m_rareNonInheritedData->textOverflow;
> }
> 
>+bool RenderStyle::isIdempotentTextAutosizingCandidate() const
>+{
>+    auto fields = OptionSet<AutosizeStatus::Fields>::fromRaw(m_inheritedFlags.autosizeStatus);
>+    if (fields.contains(AutosizeStatus::Fields::DisplayNone))
>+        return false;
>+
>+    if (fields.contains(AutosizeStatus::Fields::FixedHeight)) {
>+        if (whiteSpace() == WhiteSpace::NoWrap)
>+            return true;
>+
>+        if (fields.contains(AutosizeStatus::Fields::Floating))
>+            return fields.contains(AutosizeStatus::Fields::OutOfFlowPosition) && fields.contains(AutosizeStatus::Fields::OverflowXHidden);
>+
>+        if (fields.contains(AutosizeStatus::Fields::FixedWidth))
>+            return !fields.contains(AutosizeStatus::Fields::OutOfFlowPosition);
>+    }
>+
>+    if (fields.contains(AutosizeStatus::Fields::Floating))
>+        return true;
>+
>+    if (fields.contains(AutosizeStatus::Fields::FixedWidth))
>+        return fields.contains(AutosizeStatus::Fields::OverflowYHidden);
>+
>+    return !fields.contains(AutosizeStatus::Fields::OverflowYHidden) && !fields.contains(AutosizeStatus::Fields::FixedMaxWidth);
>+}
>+
> AutosizeStatus RenderStyle::autosizeStatus() const
> {
>     return OptionSet<AutosizeStatus::Fields>::fromRaw(m_inheritedFlags.autosizeStatus);
>diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
>index ec30ea63fa8507972363e1d4b8a113fb71b5eed6..eb2a2f6f9aa2986022d5ad824fb12fa682cac0e1 100644
>--- a/Source/WebCore/rendering/style/RenderStyle.h
>+++ b/Source/WebCore/rendering/style/RenderStyle.h
>@@ -740,7 +740,8 @@ public:
> 
> #if ENABLE(TEXT_AUTOSIZING)
>     TextSizeAdjustment textSizeAdjust() const { return m_rareInheritedData->textSizeAdjust; }
>-    AutosizeStatus autosizeStatus() const;
>+    WEBCORE_EXPORT AutosizeStatus autosizeStatus() const;
>+    WEBCORE_EXPORT bool isIdempotentTextAutosizingCandidate() const;
> #endif
> 
>     TextSecurity textSecurity() const { return static_cast<TextSecurity>(m_rareInheritedData->textSecurity); }
>@@ -1834,9 +1835,9 @@ private:
>         // 48 bits
> 
> #if ENABLE(TEXT_AUTOSIZING)
>-        unsigned autosizeStatus : 4;
>+        unsigned autosizeStatus : 8;
> #endif
>-        // 52 bits
>+        // 56 bits
>     };
> 
>     // This constructor is used to implement the replace operation.
>diff --git a/Source/WebCore/rendering/style/TextSizeAdjustment.cpp b/Source/WebCore/rendering/style/TextSizeAdjustment.cpp
>index 020027800f62a1c8b4305946a7f79d31b7c63a81..cc1da80a5b9743670507e7f545853677589a1934 100644
>--- a/Source/WebCore/rendering/style/TextSizeAdjustment.cpp
>+++ b/Source/WebCore/rendering/style/TextSizeAdjustment.cpp
>@@ -42,30 +42,35 @@ bool AutosizeStatus::contains(Fields fields) const
>     return m_fields.contains(fields);
> }
> 
>-AutosizeStatus AutosizeStatus::updateStatus(RenderStyle& style)
>+void AutosizeStatus::updateStatus(RenderStyle& style)
> {
>-    OptionSet<Fields> result = style.autosizeStatus().fields();
>+    auto result = style.autosizeStatus().fields();
>+
>+    if (style.display() == DisplayType::None)
>+        result.add(Fields::DisplayNone);
>+
>     if (style.hasOutOfFlowPosition())
>-        result.add(Fields::FoundOutOfFlowPosition);
>-    switch (style.display()) {
>-    case DisplayType::InlineBlock:
>-        result.add(Fields::FoundInlineBlock);
>-        break;
>-    case DisplayType::None:
>-        result.add(Fields::FoundDisplayNone);
>-        break;
>-    default: // FIXME: Add more cases.
>-        break;
>-    }
>+        result.add(Fields::OutOfFlowPosition);
>+
>     if (style.height().isFixed())
>-        result.add(Fields::FoundFixedHeight);
>-    style.setAutosizeStatus(result);
>-    return result;
>-}
>+        result.add(Fields::FixedHeight);
> 
>-bool AutosizeStatus::shouldSkipSubtree() const
>-{
>-    return m_fields.containsAny({ Fields::FoundOutOfFlowPosition, Fields::FoundInlineBlock, Fields::FoundFixedHeight, Fields::FoundDisplayNone });
>+    if (style.width().isFixed())
>+        result.add(Fields::FixedWidth);
>+
>+    if (style.maxWidth().isFixed())
>+        result.add(Fields::FixedMaxWidth);
>+
>+    if (style.overflowX() == Overflow::Hidden)
>+        result.add(Fields::OverflowXHidden);
>+
>+    if (style.overflowY() == Overflow::Hidden)
>+        result.add(Fields::OverflowYHidden);
>+
>+    if (style.isFloating())
>+        result.add(Fields::Floating);
>+
>+    style.setAutosizeStatus(result);
> }
> 
> float AutosizeStatus::idempotentTextSize(float specifiedSize, float pageScale)
>diff --git a/Source/WebCore/rendering/style/TextSizeAdjustment.h b/Source/WebCore/rendering/style/TextSizeAdjustment.h
>index 9f6b3fb88b15702a2879ccf335f8f3b132462959..ffc0344b2b4faeb1ede4669b86518e0fddd726ff 100644
>--- a/Source/WebCore/rendering/style/TextSizeAdjustment.h
>+++ b/Source/WebCore/rendering/style/TextSizeAdjustment.h
>@@ -52,10 +52,14 @@ private:
> class AutosizeStatus {
> public:
>     enum class Fields : uint8_t {
>-        FoundOutOfFlowPosition = 1 << 0,
>-        FoundInlineBlock = 1 << 1,
>-        FoundFixedHeight = 1 << 2,
>-        FoundDisplayNone = 1 << 3
>+        DisplayNone = 1 << 0,
>+        FixedHeight = 1 << 1,
>+        FixedWidth = 1 << 2,
>+        Floating = 1 << 3,
>+        OverflowXHidden = 1 << 4,
>+        OverflowYHidden = 1 << 5,
>+        OutOfFlowPosition = 1 << 6,
>+        FixedMaxWidth = 1 << 7
>         // Adding new values requires giving RenderStyle::InheritedFlags::autosizeStatus additional bits.
>     };
> 
>@@ -63,10 +67,9 @@ public:
>     OptionSet<Fields> fields() const { return m_fields; }
> 
>     bool contains(Fields) const;
>-    bool shouldSkipSubtree() const;
> 
>     static float idempotentTextSize(float specifiedSize, float pageScale);
>-    static AutosizeStatus updateStatus(RenderStyle&);
>+    static void updateStatus(RenderStyle&);
> 
> private:
>     OptionSet<Fields> m_fields;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index bbeccf128507a65b937fa247c00036892ccf2884..ffd37d28abd8d2cb21e0352b90a4d7468ff3a474 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-06-24  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        [Text autosizing] [iPadOS] Revise our heuristics to determine idempotent text autosizing candidates
>+        https://bugs.webkit.org/show_bug.cgi?id=198763
>+        <rdar://problem/51826266>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Rebaseline an existing text autosizing test, and introduce some new test cases that correspond to several common
>+        patterns of autosizable (or non-autosizable) text on websites that were surveyed.
>+
>+        * fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-after-changing-initial-scale.html:
>+        * fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip-expected.txt:
>+        * fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html:
>+
> 2019-06-24  Alexey Shvayka  <shvaikalesh@gmail.com>
> 
>         Add Array.prototype.{flat,flatMap} to unscopables
>diff --git a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-after-changing-initial-scale.html b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-after-changing-initial-scale.html
>index 6e40502831d098837a8d773b7547e29a74ee5b95..87745d27c09d2d096654336fafe8065f6d1971d4 100644
>--- a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-after-changing-initial-scale.html
>+++ b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-after-changing-initial-scale.html
>@@ -20,7 +20,6 @@ pre, code {
> .container {
>     border: 1px solid black;
>     padding: 8px 12px 12px 12px;
>-    max-width: 320px;
> }
> 
> #text0, #text5 {
>diff --git a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip-expected.txt b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip-expected.txt
>index 339d190531fc8a37b8587023f8594e3c031d4757..0a6c89ea055795bd06a9d67118e71ac58f8a7c89 100644
>--- a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip-expected.txt
>+++ b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip-expected.txt
>@@ -1,10 +1,34 @@
>+Checking target1:
> PASS result is >= 13
>+Checking target2:
> PASS result is >= 13
>+Checking target3:
> PASS result is 12
>+Checking target4:
>+PASS result is >= 13
>+Checking target5:
>+PASS result is >= 13
>+Checking target6:
> PASS result is 12
>+Checking target7:
>+PASS result is >= result2
>+Checking target8:
>+PASS result is >= 13
>+Checking target9:
>+PASS result is >= 13
>+Checking target10:
>+PASS result is >= 13
>+Checking target11:
>+PASS result is >= 13
>+Checking target12:
> PASS result is 12
>+Checking target13:
>+PASS result is >= 13
>+Checking target14:
> PASS result is 12
>-PASS result is >= result2
>+Checking target15:
>+PASS result is >= 13
>+Checking target16:
> PASS result is >= 13
> PASS successfullyParsed is true
> 
>@@ -16,3 +40,11 @@ Test
> Test
> TestTestTestTest
> Test
>+Test
>+Test
>+Test
>+Test
>+Test
>+Test
>+Test
>+Test
>diff --git a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html
>index 90cfa6d727a4b10a14e04bd20c83c8281425d0ea..96f7fafe94afab326ac6271fd612220af74a5cdc 100644
>--- a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html
>+++ b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-skip.html
>@@ -11,32 +11,42 @@ if (window.internals) {
> <script src="../../../../resources/js-test-pre.js"></script>
> </head>
> <body>
>-<div style="background: green;"><span id="target" style="font-size: 12px;">Test</span></div>
>+<div style="background: green;"><span id="target1" style="font-size: 12px;">Test</span></div>
> <div style="background: green; overflow: auto;"><span id="target2" style="float: right; font-size: 12px;">Test</span></div>
>-<div style="background: green;"><span id="target3" style="display: inline-block; font-size: 12px;">Test</span></div>
>-<div style="background: green;"><span style="display: inline-block; font-size: 12px;"><span id="target4">Test</span></span></div>
>+<div style="background: green;"><span id="target3" style="display: inline-block; font-size: 12px; width: 20px;">Test</span></div>
>+<div style="background: green;"><span style="display: inline-block; height: 12px; font-size: 12px;"><span id="target4">Test</span></span></div>
> <div style="background: green;"><span id="target5" style="position: absolute; left: 0px; top: 0px; font-size: 12px;">Test</span></div>
> <div style="background: green;"><span id="target6" style="display: none; font-size: 12px;">Test</span></div>
> <div style="background: green;"><span id="comparison" style="font-size: 12px;">Test<span>Test<span>Test<span id="target7">Test</span></span></span></span></div>
> <div style="background: green;"><span id="target8" style="font-size: 12px; -webkit-text-size-adjust: 100%">Test</span></div>
>+<div style="background: green;"><span id="target9" style="font-size: 12px; display: inline-block;">Test</span></div>
>+<div style="background: green;"><span id="target10" style="font-size: 12px; width: 20px; height: 12px;">Test</span></div>
>+<div style="background: green;"><span id="target11" style="font-size: 12px; height: 20px; position: fixed; white-space: nowrap;">Test</span></div>
>+<div style="background: green;"><span id="target12" style="font-size: 12px; height: 20px; position: fixed; float: right;">Test</span></div>
>+<div style="background: green;"><span id="target13" style="font-size: 12px; height: 20px; position: fixed; float: right; overflow-x: hidden; width: 100px;">Test</span></div>
>+<div style="background: green;"><span id="target14" style="font-size: 12px; height: 20px; width: 100px; float: right;">Test</span></div>
>+<div style="background: green;"><span id="target15" style="overflow-y: hidden; float: right;">Test</span></div>
>+<div style="background: green;"><span id="target16" style="float: right;">Test</span></div>
> <script>
> let result;
> function check(name, shouldGetAutosized) {
>     let target = document.getElementById(name);
>     target.offsetWidth;
>     result = Number.parseInt(window.getComputedStyle(target).getPropertyValue("font-size"));
>+    debug(`Checking ${name}:`);
>     if (shouldGetAutosized)
>         shouldBeGreaterThanOrEqual("result", "13");
>     else
>         shouldBe("result", "12");
> }
>-check("target", true);
>+check("target1", true);
> check("target2", true);
> check("target3", false);
>-check("target4", false);
>-check("target5", false);
>+check("target4", true);
>+check("target5", true);
> check("target6", false);
> 
>+debug(`Checking target7:`);
> let target = document.getElementById("target7");
> target.offsetWidth;
> let comparison = document.getElementById("comparison");
>@@ -46,6 +56,17 @@ let result2 = Number.parseInt(window.getComputedStyle(comparison).getPropertyVal
> shouldBeGreaterThanOrEqual("result", "result2");
> 
> check("target8", true);
>+check("target9", true);
>+check("target10", true);
>+
>+// Below are some common scenarios where we prefer (or prefer to not) adjust text size. These examples are inspired by
>+// common patterns in real webpages.
>+check("target11", true);
>+check("target12", false);
>+check("target13", true);
>+check("target14", false);
>+check("target15", true);
>+check("target16", true);
> </script>
> <script src="../../../../resources/js-test-post.js"></script>
> </body>

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198763: 371964 | 372807 | 372811 | 372817