Changeset 219665 in webkit


Ignore:
Timestamp:
Jul 19, 2017 4:38:06 PM (7 years ago)
Author:
mmaxfield@apple.com
Message:

Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap
​https://bugs.webkit.org/show_bug.cgi?id=174406
Source/WebCore:

Reviewed by Simon Fraser.
<rdar://problem/10139227>

Reviewed by NOBODY.

When the minimumFontSize API preference is set, we will increase the font size without increasing
the line height. If the content specifies line-height as an absolute value, there can be two
unfortunate results:

  • Adjacent lines in a paragraph can overlap
  • If the paragraph (or containin block or whatever) uses overflow: hidden, the first and last lines

can be cut off and potentially indecipherable.

Instead, we should use the minimum font size preference as a signal that we should increase the
line-height as well as the font-size. Eventually, we will want to increase it by an amount
proportional to the increase in font-size (which can be due to minimumFontSize, minimumLogicalFontSize,
text autosizing, etc.). However, because minimumLogicalFontSize is on by default, this would cause
a behavior change on many webpages which use small text, so such a change would be too risky right now.
Instead, we can pretend that minimumFontSize is the only cause that text increases, and use this as the
only signal to boost the corresponding line-height.

Tests: fast/text/line-height-minimumFontSize-text-zoom.html

fast/text/line-height-minimumFontSize-visual.html
fast/text/line-height-minimumFontSize-zoom.html
fast/text/line-height-minimumFontSize.html
fast/text/line-height-minimumFontSize-autosize.html

  • css/StyleBuilderCustom.h:

(WebCore::computeBaseSpecifiedFontSize):
(WebCore::computeLineHeightMultiplierDueToFontSize):
(WebCore::StyleBuilderCustom::applyValueLineHeight):
(WebCore::StyleBuilderCustom::applyValueFill):
(WebCore::StyleBuilderCustom::applyValueStroke):
(WebCore::StyleBuilderCustom::applyValueContent):

  • rendering/TextAutoSizing.cpp:

LayoutTests:

<rdar://problem/10139227>

Reviewed by Simon Fraser.

  • fast/text/line-height-minimumFontSize-autosize-expected.text: Added.
  • fast/text/line-height-minimumFontSize-autosize.html: Added.
  • fast/text/line-height-minimumFontSize-expected.txt: Added.
  • fast/text/line-height-minimumFontSize-text-zoom-expected.html: Added.
  • fast/text/line-height-minimumFontSize-text-zoom.html: Added.
  • fast/text/line-height-minimumFontSize-visual-expected.html: Added.
  • fast/text/line-height-minimumFontSize-visual.html: Added.
  • fast/text/line-height-minimumFontSize-zoom-expected.html: Added.
  • fast/text/line-height-minimumFontSize-zoom.html: Added.
  • fast/text/line-height-minimumFontSize.html: Added.
Location:
trunk
Files:
10 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r219663 r219665  
     12017-07-19  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap
     4        https://bugs.webkit.org/show_bug.cgi?id=174406
     5        <rdar://problem/10139227>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * fast/text/line-height-minimumFontSize-autosize-expected.text: Added.
     10        * fast/text/line-height-minimumFontSize-autosize.html: Added.
     11        * fast/text/line-height-minimumFontSize-expected.txt: Added.
     12        * fast/text/line-height-minimumFontSize-text-zoom-expected.html: Added.
     13        * fast/text/line-height-minimumFontSize-text-zoom.html: Added.
     14        * fast/text/line-height-minimumFontSize-visual-expected.html: Added.
     15        * fast/text/line-height-minimumFontSize-visual.html: Added.
     16        * fast/text/line-height-minimumFontSize-zoom-expected.html: Added.
     17        * fast/text/line-height-minimumFontSize-zoom.html: Added.
     18        * fast/text/line-height-minimumFontSize.html: Added.
     19
     202017-07-18  Myles C. Maxfield  <mmaxfield@apple.com>
     21
     22        Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap
     23        https://bugs.webkit.org/show_bug.cgi?id=174406
     24        <rdar://problem/10139227>
     25
     26        Reviewed by Simon Fraser.
     27
     28        * fast/text/line-height-minimumFontSize-autosize-expected.text: Added.
     29        * fast/text/line-height-minimumFontSize-autosize.html: Added.
     30        * fast/text/line-height-minimumFontSize-expected.txt: Added.
     31        * fast/text/line-height-minimumFontSize-text-zoom-expected.html: Added.
     32        * fast/text/line-height-minimumFontSize-text-zoom.html: Added.
     33        * fast/text/line-height-minimumFontSize-visual-expected.html: Added.
     34        * fast/text/line-height-minimumFontSize-visual.html: Added.
     35        * fast/text/line-height-minimumFontSize-zoom-expected.html: Added.
     36        * fast/text/line-height-minimumFontSize-zoom.html: Added.
     37        * fast/text/line-height-minimumFontSize.html: Added.
     38
    1392017-07-19  Chris Dumez  <cdumez@apple.com>
    240

  • trunk/Source/WebCore/ChangeLog

    r219663 r219665  
     12017-07-19  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap
     4        https://bugs.webkit.org/show_bug.cgi?id=174406
     5
     6        Reviewed by Simon Fraser.
     7        <rdar://problem/10139227>
     8
     9        Reviewed by NOBODY.
     10
     11        When the minimumFontSize API preference is set, we will increase the font size without increasing
     12        the line height. If the content specifies line-height as an absolute value, there can be two
     13        unfortunate results:
     14
     15        - Adjacent lines in a paragraph can overlap
     16        - If the paragraph (or containin block or whatever) uses overflow: hidden, the first and last lines
     17        can be cut off and potentially indecipherable.
     18
     19        Instead, we should use the minimum font size preference as a signal that we should increase the
     20        line-height as well as the font-size. Eventually, we will want to increase it by an amount
     21        proportional to the increase in font-size (which can be due to minimumFontSize, minimumLogicalFontSize,
     22        text autosizing, etc.). However, because minimumLogicalFontSize is on by default, this would cause
     23        a behavior change on many webpages which use small text, so such a change would be too risky right now.
     24        Instead, we can pretend that minimumFontSize is the only cause that text increases, and use this as the
     25        only signal to boost the corresponding line-height.
     26
     27        Tests: fast/text/line-height-minimumFontSize-text-zoom.html
     28               fast/text/line-height-minimumFontSize-visual.html
     29               fast/text/line-height-minimumFontSize-zoom.html
     30               fast/text/line-height-minimumFontSize.html
     31               fast/text/line-height-minimumFontSize-autosize.html
     32
     33        * css/StyleBuilderCustom.h:
     34        (WebCore::computeBaseSpecifiedFontSize):
     35        (WebCore::computeLineHeightMultiplierDueToFontSize):
     36        (WebCore::StyleBuilderCustom::applyValueLineHeight):
     37        (WebCore::StyleBuilderCustom::applyValueFill):
     38        (WebCore::StyleBuilderCustom::applyValueStroke):
     39        (WebCore::StyleBuilderCustom::applyValueContent):
     40        * rendering/TextAutoSizing.cpp:
     41
     422017-07-18  Myles C. Maxfield  <mmaxfield@apple.com>
     43
     44        Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap
     45        https://bugs.webkit.org/show_bug.cgi?id=174406
     46        <rdar://problem/10139227>
     47
     48        Reviewed by Simon Fraser.
     49
     50        When the minimumFontSize API preference is set, we will increase the font size without increasing
     51        the line height. If the content specifies line-height as an absolute value, there can be two
     52        unfortunate results:
     53
     54        - Adjacent lines in a paragraph can overlap
     55        - If the paragraph (or containin block or whatever) uses overflow: hidden, the first and last lines
     56        can be cut off and potentially indecipherable.
     57
     58        Instead, we should use the minimum font size preference as a signal that we should increase the
     59        line-height as well as the font-size. Eventually, we will want to increase it by an amount
     60        proportional to the increase in font-size (which can be due to minimumFontSize, minimumLogicalFontSize,
     61        text autosizing, etc.). However, because minimumLogicalFontSize is on by default, this would cause
     62        a behavior change on many webpages which use small text, so such a change would be too risky right now.
     63        Instead, we can pretend that minimumFontSize is the only cause that text increases, and use this as the
     64        only signal to boost the corresponding line-height.
     65
     66        Tests: fast/text/line-height-minimumFontSize-text-zoom.html
     67               fast/text/line-height-minimumFontSize-visual.html
     68               fast/text/line-height-minimumFontSize-zoom.html
     69               fast/text/line-height-minimumFontSize.html
     70               fast/text/line-height-minimumFontSize-autosize.html
     71
     72        * css/StyleBuilderCustom.h:
     73        (WebCore::computeBaseSpecifiedFontSize):
     74        (WebCore::computeLineHeightMultiplierDueToFontSize):
     75        (WebCore::StyleBuilderCustom::applyValueLineHeight):
     76        (WebCore::StyleBuilderCustom::applyValueFill):
     77        (WebCore::StyleBuilderCustom::applyValueStroke):
     78        (WebCore::StyleBuilderCustom::applyValueContent):
     79        * rendering/TextAutoSizing.cpp:
     80
    1812017-07-19  Chris Dumez  <cdumez@apple.com>
    282

  • trunk/Source/WebCore/css/StyleBuilderCustom.h

    r219656 r219665  
    651651}
    652652
     653static inline float computeBaseSpecifiedFontSize(const Document& document, const RenderStyle& style, bool percentageAutosizingEnabled)
     654{
     655    float result = style.specifiedFontSize();
     656    auto* frame = document.frame();
     657    if (frame && style.textZoom() != TextZoomReset)
     658        result *= frame->textZoomFactor();
     659    result *= style.effectiveZoom();
     660    if (percentageAutosizingEnabled)
     661        result *= style.textSizeAdjust().multiplier();
     662    return result;
     663}
     664
     665static inline float computeLineHeightMultiplierDueToFontSize(const Document& document, const RenderStyle& style, const CSSPrimitiveValue& value)
     666{
     667    bool percentageAutosizingEnabled = document.settings().textAutosizingEnabled() && style.textSizeAdjust().isPercentage();
     668
     669    if (value.isLength()) {
     670        auto minimumFontSize = document.settings().minimumFontSize();
     671        if (minimumFontSize > 0) {
     672            auto specifiedFontSize = computeBaseSpecifiedFontSize(document, style, percentageAutosizingEnabled);
     673            if (specifiedFontSize < minimumFontSize) {
     674                // FIXME: There are two settings which are relevant here: minimum font size, and minimum logical font size (as
     675                // well as things like the zoom property, text zoom on the page, and text autosizing). The minimum logical font
     676                // size is nonzero by default, and already incorporated into the computed font size, so if we just use the ratio
     677                // of the computed : specified font size, it will be > 1 in the cases where the minimum logical font size kicks
     678                // in. In general, this is the right thing to do, however, this kind of blanket change is too risky to perform
     679                // right now. https://bugs.webkit.org/show_bug.cgi?id=174570 tracks turning this on. For now, we can just pretend
     680                // that the minimum font size is the only thing affecting the computed font size.
     681
     682                // This calculation matches the line-height computed size calculation in
     683                // TextAutoSizingValue::adjustTextNodeSizes().
     684                auto scaleChange = minimumFontSize / specifiedFontSize;
     685                return scaleChange;
     686            }
     687        }
     688    }
     689
     690    if (percentageAutosizingEnabled)
     691        return style.textSizeAdjust().multiplier();
     692    return 1;
     693}
     694
    653695inline void StyleBuilderCustom::applyValueLineHeight(StyleResolver& styleResolver, CSSValue& value)
    654696{
    655     float multiplier = styleResolver.style()->textSizeAdjust().isPercentage() ? styleResolver.style()->textSizeAdjust().multiplier() : 1.f;
    656     std::optional<Length> lineHeight = StyleBuilderConverter::convertLineHeight(styleResolver, value, multiplier);
     697    std::optional<Length> lineHeight = StyleBuilderConverter::convertLineHeight(styleResolver, value, 1);
    657698    if (!lineHeight)
    658699        return;
    659700
    660     styleResolver.style()->setLineHeight(Length { lineHeight.value() });
     701    Length computedLineHeight;
     702    if (lineHeight.value().isNegative())
     703        computedLineHeight = lineHeight.value();
     704    else {
     705        auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
     706        auto multiplier = computeLineHeightMultiplierDueToFontSize(styleResolver.document(), *styleResolver.style(), primitiveValue);
     707        if (multiplier == 1)
     708            computedLineHeight = lineHeight.value();
     709        else {
     710            std::optional<Length> lineHeight = StyleBuilderConverter::convertLineHeight(styleResolver, value, multiplier);
     711            ASSERT(static_cast<bool>(lineHeight));
     712            computedLineHeight = lineHeight.value();
     713        }
     714    }
     715
     716    styleResolver.style()->setLineHeight(WTFMove(computedLineHeight));
    661717    styleResolver.style()->setSpecifiedLineHeight(WTFMove(lineHeight.value()));
    662718}
    … …  
    734790    styleResolver.style()->setHasExplicitlySetWritingMode(true);
    735791}
    736    
     792
    737793inline void StyleBuilderCustom::applyValueWebkitTextOrientation(StyleResolver& styleResolver, CSSValue& value)
    738794{
    … …  
    12111267        localValue = downcast<CSSPrimitiveValue>(list.item(1));
    12121268    }
    1213    
     1269
    12141270    if (!localValue)
    12151271        return;
    1216    
     1272
    12171273    Color color;
    12181274    auto paintType = SVG_PAINTTYPE_RGBCOLOR;
    … …  
    12551311        localValue = downcast<CSSPrimitiveValue>(list.item(1));
    12561312    }
    1257    
     1313
    12581314    if (!localValue)
    12591315        return;
    1260    
     1316
    12611317    Color color;
    12621318    auto paintType = SVG_PAINTTYPE_RGBCOLOR;
    … …  
    13531409        return;
    13541410    }
    1355    
     1411
    13561412    bool didSet = false;
    13571413    for (auto& item : downcast<CSSValueList>(value)) {

  • trunk/Source/WebCore/rendering/TextAutoSizing.cpp

    r219656 r219665  
    143143            specifiedLineHeight = lineHeightLength.value();
    144144
     145        // This calculation matches the line-height computed size calculation in StyleBuilderCustom::applyValueLineHeight().
    145146        int lineHeight = specifiedLineHeight * scaleChange;
    146147        if (lineHeightLength.isFixed() && lineHeightLength.value() == lineHeight)
Note: See TracChangeset for help on using the changeset viewer.