Changeset 219646 in webkit


Ignore:
Timestamp:
Jul 18, 2017 8:54:38 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
<rdar://problem/10139227>

Reviewed by Simon Fraser.

Source/WebCore:

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:

  • 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

    r219642 r219646  
     12017-07-18  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
    1202017-07-18  Ali Juma  <ajuma@chromium.org>
    221

  • trunk/Source/WebCore/ChangeLog

    r219645 r219646  
     12017-07-18  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        When the minimumFontSize API preference is set, we will increase the font size without increasing
     10        the line height. If the content specifies line-height as an absolute value, there can be two
     11        unfortunate results:
     12
     13        - Adjacent lines in a paragraph can overlap
     14        - If the paragraph (or containin block or whatever) uses overflow: hidden, the first and last lines
     15        can be cut off and potentially indecipherable.
     16
     17        Instead, we should use the minimum font size preference as a signal that we should increase the
     18        line-height as well as the font-size. Eventually, we will want to increase it by an amount
     19        proportional to the increase in font-size (which can be due to minimumFontSize, minimumLogicalFontSize,
     20        text autosizing, etc.). However, because minimumLogicalFontSize is on by default, this would cause
     21        a behavior change on many webpages which use small text, so such a change would be too risky right now.
     22        Instead, we can pretend that minimumFontSize is the only cause that text increases, and use this as the
     23        only signal to boost the corresponding line-height.
     24
     25        Tests: fast/text/line-height-minimumFontSize-text-zoom.html
     26               fast/text/line-height-minimumFontSize-visual.html
     27               fast/text/line-height-minimumFontSize-zoom.html
     28               fast/text/line-height-minimumFontSize.html
     29               fast/text/line-height-minimumFontSize-autosize.html
     30
     31        * css/StyleBuilderCustom.h:
     32        (WebCore::computeBaseSpecifiedFontSize):
     33        (WebCore::computeLineHeightMultiplierDueToFontSize):
     34        (WebCore::StyleBuilderCustom::applyValueLineHeight):
     35        (WebCore::StyleBuilderCustom::applyValueFill):
     36        (WebCore::StyleBuilderCustom::applyValueStroke):
     37        (WebCore::StyleBuilderCustom::applyValueContent):
     38        * rendering/TextAutoSizing.cpp:
     39
    1402017-07-18  Zalan Bujtas  <zalan@apple.com>
    241

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

    r219237 r219646  
    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        std::optional<Length> lineHeightLength;
     708        if (multiplier == 1)
     709            lineHeightLength = lineHeight;
     710        else
     711            lineHeightLength = StyleBuilderConverter::convertLineHeight(styleResolver, value, multiplier);
     712        ASSERT(static_cast<bool>(lineHeightLength));
     713        computedLineHeight = lineHeight.value();
     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

    r217893 r219646  
    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.