[Text autosizing] [iPadOS] Add targeted hacks to address some remaining text autosizi...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Aug 2019 18:32:32 +0000 (18:32 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Aug 2019 18:32:32 +0000 (18:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200271
<rdar://problem/51734741>

Reviewed by Zalan Bujtas.

Source/WebCore:

Makes some targeted adjustments to the text autosizing heuristic, to ensure compatibility with several high-
profile websites. See changes below for more detail.

Tests:  fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html
        fast/text-autosizing/ios/idempotentmode/line-height-boosting.html

* css/StyleResolver.cpp:
(WebCore::StyleResolver::adjustRenderStyleForTextAutosizing):

Avoid clipped sidebar links on sohu.com by not performing line-height boosting in the case where the element
probably has a small, fixed number of lines. See below for more detail. Additionally, don't attempt to adjust
the line height using the boosted font size, in the case where the element is not a candidate for idempotent
text autosizing.

* rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::isIdempotentTextAutosizingCandidate const):

Make various targeted hacks to fix a few websites:

-   Add a special case for top navigation bar links on yandex.ru, where line height greatly exceeds the
    specified font size.

-   Avoid boosting some related video links on v.youku.com by considering the line-clamp CSS property when
    determining the maximum number of lines of text an element is expected to contain.

-   Avoid boosting some front page links on asahi.com, which have non-repeating background images.

-   Add several other adjustments to more aggressively boost pieces of text on Google search results, such as
    taking the `word-break` CSS property into account.

The bottom few pixels of sidebar links on naver.com are also no longer clipped after these changes.

* rendering/style/TextSizeAdjustment.cpp:
(WebCore::AutosizeStatus::probablyContainsASmallFixedNumberOfLines):

Pulls out a piece of the heuristic added to fix sephora.com in r247467 out into a separate helper method. To
recap, this heuristic identifies elements with both a fixed height and fixed line height, for which the fixed
height is close to an integer multiple of the line height.

Also makes several small tweaks in the process: (1) change the max difference between fixed line height and
font size from 6 to 5 to ensure that some multiline caption text on Google search results is boosted, and (2)
replace usages of `lineHeight()` with `specifiedLineHeight()`, which current prevents this function from being
truly idempotent.

(WebCore::AutosizeStatus::updateStatus):
* rendering/style/TextSizeAdjustment.h:

LayoutTests:

Add tests to cover some changes to line height boosting and the idempotent text autosizing candidate heuristic.

* fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases-expected.txt: Added.
* fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html: Added.
* fast/text-autosizing/ios/idempotentmode/line-height-boosting-expected.txt:
* fast/text-autosizing/ios/idempotentmode/line-height-boosting.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248112 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases-expected.txt [new file with mode: 0644]
LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html [new file with mode: 0644]
LayoutTests/fast/text-autosizing/ios/idempotentmode/line-height-boosting-expected.txt
LayoutTests/fast/text-autosizing/ios/idempotentmode/line-height-boosting.html
Source/WebCore/ChangeLog
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/rendering/style/RenderStyle.cpp
Source/WebCore/rendering/style/TextSizeAdjustment.cpp
Source/WebCore/rendering/style/TextSizeAdjustment.h

index 9261b3b..bd94bf4 100644 (file)
@@ -1,3 +1,18 @@
+2019-08-01  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Text autosizing] [iPadOS] Add targeted hacks to address some remaining text autosizing issues
+        https://bugs.webkit.org/show_bug.cgi?id=200271
+        <rdar://problem/51734741>
+
+        Reviewed by Zalan Bujtas.
+
+        Add tests to cover some changes to line height boosting and the idempotent text autosizing candidate heuristic.
+
+        * fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases-expected.txt: Added.
+        * fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html: Added.
+        * fast/text-autosizing/ios/idempotentmode/line-height-boosting-expected.txt:
+        * fast/text-autosizing/ios/idempotentmode/line-height-boosting.html:
+
 2019-08-01  Truitt Savell  <tsavell@apple.com>
 
         Removing expectations for tests that are now consistently passing
diff --git a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases-expected.txt b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases-expected.txt
new file mode 100644 (file)
index 0000000..54ac33c
--- /dev/null
@@ -0,0 +1,20 @@
+PASS elementWasAutosized('text1') is true
+PASS elementWasAutosized('text2') is false
+PASS elementWasAutosized('text3') is true
+PASS elementWasAutosized('text4') is true
+PASS elementWasAutosized('text5') is true
+PASS elementWasAutosized('text6') is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+Hello world
+
+Hello world
+
+Hello world
+
+Hello world
+
+Hello world
+
+Hello world
diff --git a/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html b/LayoutTests/fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html
new file mode 100644 (file)
index 0000000..6c107c3
--- /dev/null
@@ -0,0 +1,46 @@
+<!DOCTYPE html><!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<meta name="viewport" content="initial-scale=0.6666">
+<script>
+if (window.internals) {
+    internals.settings.setTextAutosizingEnabled(true);
+    internals.settings.setTextAutosizingUsesIdempotentMode(true);
+}
+</script>
+<style>
+body {
+    font-size: 10px;
+}
+
+.fixed-height {
+    height: 44px;
+}
+</style>
+<script src="../../../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<div style="float: left; width: 100px;" class="fixed-width fixed-height">
+    <p style="line-height: 16px;" class="medium-fixed-line-height fixed-height" id="text1">Hello world</p>
+    <p style="line-height: 12px; height: 12px;" id="text2">Hello world</p>
+</div>
+<p style="width: 100px; word-break: break-word;" id="text3">Hello world</p>
+<p style="" id="text4">Hello world</p>
+<p style="-webkit-textsize-adjust: 100%; line-height: 44px;" id="text5">Hello world</p>
+<p style="background-repeat: no-repeat; background-image: url(data:image/gif;base64,R0lGODlhEAARAJECAOHh4UpKSgAAAAAAACH5BAEAAAIALAAAAAAQABEAAAIllB8Zx63b4otSUWcvyuz5D4biSD7AiZroWSXa5r7CJNOvra1RAQA7);" id="text6">Hello world</p>
+<script>
+function elementWasAutosized(elementID)
+{
+    return parseInt(getComputedStyle(document.getElementById(elementID)).fontSize) > 10;
+}
+
+shouldBe("elementWasAutosized('text1')", "true");
+shouldBe("elementWasAutosized('text2')", "false");
+shouldBe("elementWasAutosized('text3')", "true");
+shouldBe("elementWasAutosized('text4')", "true");
+shouldBe("elementWasAutosized('text5')", "true");
+shouldBe("elementWasAutosized('text6')", "false");
+</script>
+<script src="../../../../resources/js-test-post.js"></script>
+</body>
+</html>
index 2ac7935..ac33c4c 100644 (file)
@@ -1,8 +1,11 @@
 PASS result is >= 18
 PASS result is >= 13
 PASS result is >= 11
-PASS result is >= 12
-PASS result is >= 18
+PASS result is 12
+PASS result is 22
+PASS result is 18
+PASS result is 18
+PASS result is 18
 PASS successfullyParsed is true
 
 TEST COMPLETE
@@ -11,3 +14,6 @@ yes line-height boost
 yes line-height boost
 no line-height boost
 no line-height boost
+no line-height boost
+no line-height boost
+no line-height boost
index bdc3fd1..8efefb6 100644 (file)
@@ -16,6 +16,11 @@ if (window.internals) {
 <div><span id=target3 style="font-size: 6px; line-height: 10px">yes line-height boost</span></div>
 <div><span id=target4 style="font-size: 6px; line-height: 12px">no line-height boost</span></div>
 <div><span id=target5 style="font-size: 18px;">no line-height boost</span></div>
+<div style="height: 54px; overflow: hidden;">
+    <div id=target6 style="font-size: 12px; line-height: 18px;">no line-height boost</div>
+    <div id=target7 style="font-size: 12px; line-height: 18px;">no line-height boost</div>
+    <div id=target8 style="font-size: 12px; line-height: 18px;">no line-height boost</div>
+</div>
 <script>
 document.body.offsetHeight;
 let result = Number.parseInt(window.getComputedStyle(target1).getPropertyValue("line-height"));
@@ -28,10 +33,19 @@ result = Number.parseInt(window.getComputedStyle(target3).getPropertyValue("line
 shouldBeGreaterThanOrEqual("result", "11");
 
 result = Number.parseInt(window.getComputedStyle(target4).getPropertyValue("line-height"));
-shouldBeGreaterThanOrEqual("result", "12");
+shouldBe("result", "12");
 
 result = Number.parseInt(window.getComputedStyle(target5).getPropertyValue("line-height"));
-shouldBeGreaterThanOrEqual("result", "18");
+shouldBe("result", "22");
+
+result = Number.parseInt(window.getComputedStyle(target6).getPropertyValue("line-height"));
+shouldBe("result", "18");
+
+result = Number.parseInt(window.getComputedStyle(target7).getPropertyValue("line-height"));
+shouldBe("result", "18");
+
+result = Number.parseInt(window.getComputedStyle(target8).getPropertyValue("line-height"));
+shouldBe("result", "18");
 </script>
 <script src="../../../../resources/js-test-post.js"></script>
 </body>
index 8a2281e..cf2e3d1 100644 (file)
@@ -1,3 +1,58 @@
+2019-08-01  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Text autosizing] [iPadOS] Add targeted hacks to address some remaining text autosizing issues
+        https://bugs.webkit.org/show_bug.cgi?id=200271
+        <rdar://problem/51734741>
+
+        Reviewed by Zalan Bujtas.
+
+        Makes some targeted adjustments to the text autosizing heuristic, to ensure compatibility with several high-
+        profile websites. See changes below for more detail.
+
+        Tests:  fast/text-autosizing/ios/idempotentmode/idempotent-autosizing-candidate-special-cases.html
+                fast/text-autosizing/ios/idempotentmode/line-height-boosting.html
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::adjustRenderStyleForTextAutosizing):
+
+        Avoid clipped sidebar links on sohu.com by not performing line-height boosting in the case where the element
+        probably has a small, fixed number of lines. See below for more detail. Additionally, don't attempt to adjust
+        the line height using the boosted font size, in the case where the element is not a candidate for idempotent
+        text autosizing.
+
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::isIdempotentTextAutosizingCandidate const):
+
+        Make various targeted hacks to fix a few websites:
+
+        -   Add a special case for top navigation bar links on yandex.ru, where line height greatly exceeds the
+            specified font size.
+
+        -   Avoid boosting some related video links on v.youku.com by considering the line-clamp CSS property when
+            determining the maximum number of lines of text an element is expected to contain.
+
+        -   Avoid boosting some front page links on asahi.com, which have non-repeating background images.
+
+        -   Add several other adjustments to more aggressively boost pieces of text on Google search results, such as
+            taking the `word-break` CSS property into account.
+
+        The bottom few pixels of sidebar links on naver.com are also no longer clipped after these changes.
+
+        * rendering/style/TextSizeAdjustment.cpp:
+        (WebCore::AutosizeStatus::probablyContainsASmallFixedNumberOfLines):
+
+        Pulls out a piece of the heuristic added to fix sephora.com in r247467 out into a separate helper method. To
+        recap, this heuristic identifies elements with both a fixed height and fixed line height, for which the fixed
+        height is close to an integer multiple of the line height.
+
+        Also makes several small tweaks in the process: (1) change the max difference between fixed line height and
+        font size from 6 to 5 to ensure that some multiline caption text on Google search results is boosted, and (2)
+        replace usages of `lineHeight()` with `specifiedLineHeight()`, which current prevents this function from being
+        truly idempotent.
+
+        (WebCore::AutosizeStatus::updateStatus):
+        * rendering/style/TextSizeAdjustment.h:
+
 2019-07-31  Mark Lam  <mark.lam@apple.com>
 
         Rename DOMJIT safe/unsafeFunction to functionWithTypeChecks and functionWithoutTypeChecks.
index b7db7b2..3a52c36 100644 (file)
@@ -901,6 +901,9 @@ bool StyleResolver::adjustRenderStyleForTextAutosizing(RenderStyle& style, const
         if (!lineHeight.isFixed() || lineHeight.value() >= minimumLineHeight)
             return;
 
+        if (AutosizeStatus::probablyContainsASmallFixedNumberOfLines(style))
+            return;
+
         style.setLineHeight({ minimumLineHeight, Fixed });
     };
 
@@ -921,7 +924,13 @@ bool StyleResolver::adjustRenderStyleForTextAutosizing(RenderStyle& style, const
     fontDescription.setComputedSize(isCandidate ? adjustedFontSize : specifiedFontSize);
     style.setFontDescription(WTFMove(fontDescription));
     style.fontCascade().update(&document().fontSelector());
-    adjustLineHeightIfNeeded(adjustedFontSize);
+
+    // FIXME: We should restore computed line height to its original value in the case where the element is not
+    // an idempotent text autosizing candidate; otherwise, if an element that is a text autosizing candidate contains
+    // children which are not autosized, the non-autosized content will end up with a boosted line height.
+    if (isCandidate)
+        adjustLineHeightIfNeeded(adjustedFontSize);
+
     return true;
 }
 #endif
index 04c977b..249ba47 100644 (file)
@@ -499,17 +499,34 @@ bool RenderStyle::isIdempotentTextAutosizingCandidate() const
     if (fields.contains(AutosizeStatus::Fields::AvoidSubtree))
         return false;
 
+    const float smallMinimumDifferenceThresholdBetweenLineHeightAndSpecifiedFontSizeForBoostingText = 5;
+    const float largeMinimumDifferenceThresholdBetweenLineHeightAndSpecifiedFontSizeForBoostingText = 25;
+
     if (fields.contains(AutosizeStatus::Fields::FixedHeight)) {
         if (fields.contains(AutosizeStatus::Fields::FixedWidth)) {
             if (whiteSpace() == WhiteSpace::NoWrap) {
                 if (width().isFixed())
                     return false;
 
+                if (height().isFixed() && specifiedLineHeight().isFixed()) {
+                    float specifiedSize = specifiedFontSize();
+                    if (height().value() == specifiedSize && specifiedLineHeight().value() == specifiedSize)
+                        return false;
+                }
+
                 return true;
             }
 
-            if (fields.contains(AutosizeStatus::Fields::Floating))
+            if (fields.contains(AutosizeStatus::Fields::Floating)) {
+                if (specifiedLineHeight().isFixed() && height().isFixed()) {
+                    float specifiedSize = specifiedFontSize();
+                    if (specifiedLineHeight().value() - specifiedSize > smallMinimumDifferenceThresholdBetweenLineHeightAndSpecifiedFontSizeForBoostingText
+                        && height().value() - specifiedSize > smallMinimumDifferenceThresholdBetweenLineHeightAndSpecifiedFontSizeForBoostingText) {
+                        return true;
+                    }
+                }
                 return false;
+            }
 
             if (fields.contains(AutosizeStatus::Fields::OverflowXHidden))
                 return false;
@@ -527,8 +544,12 @@ bool RenderStyle::isIdempotentTextAutosizingCandidate() const
         return true;
     }
 
-    if (width().isFixed())
+    if (width().isFixed()) {
+        if (breakWords())
+            return true;
+
         return false;
+    }
 
     if (textSizeAdjust().isPercentage() && textSizeAdjust().percentage() == 100) {
         if (fields.contains(AutosizeStatus::Fields::Floating))
@@ -537,9 +558,15 @@ bool RenderStyle::isIdempotentTextAutosizingCandidate() const
         if (fields.contains(AutosizeStatus::Fields::FixedWidth))
             return true;
 
+        if (specifiedLineHeight().isFixed() && specifiedLineHeight().value() - specifiedFontSize() > largeMinimumDifferenceThresholdBetweenLineHeightAndSpecifiedFontSizeForBoostingText)
+            return true;
+
         return false;
     }
 
+    if (hasBackgroundImage() && backgroundRepeatX() == FillRepeat::NoRepeat && backgroundRepeatY() == FillRepeat::NoRepeat)
+        return false;
+
     return true;
 }
 
index f13877d..a3e52ba 100644 (file)
@@ -42,6 +42,43 @@ bool AutosizeStatus::contains(Fields fields) const
     return m_fields.contains(fields);
 }
 
+bool AutosizeStatus::probablyContainsASmallFixedNumberOfLines(const RenderStyle& style)
+{
+    auto& lineHeightAsLength = style.specifiedLineHeight();
+    if (!lineHeightAsLength.isFixed() && !lineHeightAsLength.isPercent())
+        return false;
+
+    auto& maxHeight = style.maxHeight();
+    Optional<Length> heightOrMaxHeightAsLength;
+    if (maxHeight.isFixed())
+        heightOrMaxHeightAsLength = style.maxHeight();
+    else if (style.height().isFixed() && (!maxHeight.isSpecified() || maxHeight.isUndefined()))
+        heightOrMaxHeightAsLength = style.height();
+
+    if (!heightOrMaxHeightAsLength)
+        return false;
+
+    float heightOrMaxHeight = heightOrMaxHeightAsLength->value();
+    if (heightOrMaxHeight <= 0)
+        return false;
+
+    float approximateLineHeight = lineHeightAsLength.isPercent() ? lineHeightAsLength.percent() * style.specifiedFontSize() / 100 : lineHeightAsLength.value();
+    if (approximateLineHeight <= 0)
+        return false;
+
+    float approximateNumberOfLines = heightOrMaxHeight / approximateLineHeight;
+    auto& lineClamp = style.lineClamp();
+    if (!lineClamp.isNone() && !lineClamp.isPercentage()) {
+        int lineClampValue = lineClamp.value();
+        return lineClampValue && std::floor(approximateNumberOfLines) == lineClampValue;
+    }
+
+    const int maximumNumberOfLines = 5;
+    const float thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger = 0.01;
+    return approximateNumberOfLines <= maximumNumberOfLines + thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger
+        && approximateNumberOfLines - std::floor(approximateNumberOfLines) <= thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger;
+}
+
 void AutosizeStatus::updateStatus(RenderStyle& style)
 {
     auto result = style.autosizeStatus().fields();
@@ -50,32 +87,15 @@ void AutosizeStatus::updateStatus(RenderStyle& style)
         if (style.display() == DisplayType::None)
             return true;
 
-        if (!style.lineHeight().isSpecified() || style.whiteSpace() == WhiteSpace::NoWrap)
-            return false;
-
-        const float maximumDifferenceBetweenFixedLineHeightAndFontSize = 6;
-        if (style.lineHeight().isFixed() && style.lineHeight().value() - style.fontDescription().specifiedSize() > maximumDifferenceBetweenFixedLineHeightAndFontSize)
-            return false;
-
-        Optional<Length> heightOrMaxHeightAsLength;
-        if (style.height().isFixed() && style.maxHeight().isAuto())
-            heightOrMaxHeightAsLength = style.height();
-        else if (style.maxHeight().isFixed())
-            heightOrMaxHeightAsLength = style.maxHeight();
-
-        if (!heightOrMaxHeightAsLength)
+        const float maximumDifferenceBetweenFixedLineHeightAndFontSize = 5;
+        auto& lineHeight = style.specifiedLineHeight();
+        if (lineHeight.isFixed() && lineHeight.value() - style.specifiedFontSize() > maximumDifferenceBetweenFixedLineHeightAndFontSize)
             return false;
 
-        float heightOrMaxHeight = heightOrMaxHeightAsLength->value();
-        float computedLineHeight = style.computedLineHeight();
-        if (computedLineHeight <= 0)
+        if (style.whiteSpace() == WhiteSpace::NoWrap)
             return false;
 
-        float approximateNumberOfLines = heightOrMaxHeight / computedLineHeight;
-        const int maximumNumberOfLines = 5;
-        const float thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger = 0.01;
-        return approximateNumberOfLines <= maximumNumberOfLines + thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger
-            && approximateNumberOfLines - std::floor(approximateNumberOfLines) <= thresholdForConsideringAnApproximateNumberOfLinesToBeCloseToAnInteger;
+        return probablyContainsASmallFixedNumberOfLines(style);
     };
 
     if (shouldAvoidAutosizingEntireSubtree())
index 552ca41..247d682 100644 (file)
@@ -68,6 +68,8 @@ public:
     static float idempotentTextSize(float specifiedSize, float pageScale);
     static void updateStatus(RenderStyle&);
 
+    static bool probablyContainsASmallFixedNumberOfLines(const RenderStyle&);
+
 private:
     OptionSet<Fields> m_fields;
 };