Bug 174406 – Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[iOS] Emails with line-height set have overlapping text when accessibility text size is increased

[Myles C. Maxfield]

Created attachment 315195 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/10139227>

[Simon Fraser (smfr)]

Comment on attachment 315195 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=315195&action=review

> Source/WebCore/style/StyleFontSizeFunctions.cpp:45
> +    Dumb,
> +    Smart

Maybe these could have better names that indicate what makes them dumb or smart.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1290
> +    //ASSERT(scale == [webView->_scrollView zoomScale]);

Odd that you're hitting this.

[Myles C. Maxfield]

Comment on attachment 315195 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=315195&action=review

>> Source/WebCore/style/StyleFontSizeFunctions.cpp:45
>> +    Smart
> 
> Maybe these could have better names that indicate what makes them dumb or smart.

👍

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1290
>> +    //ASSERT(scale == [webView->_scrollView zoomScale]);
> 
> Odd that you're hitting this.

Hitting this every time I open a mail message on iOS using Debug WebKit.

[Myles C. Maxfield]

Created attachment 315207 [details]
Patch

Comment on attachment 315207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315207&action=review

> Source/WebCore/ChangeLog:3
> +        [iOS] Emails with line-height set have overlapping text when accessibility text size is increased

Seems misleading to say [iOS] in the bug title when the fix applies to all platforms.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174406

You should cite the Apple Radar number for this bug as well. I think it has a better title, too!

> Source/WebCore/ChangeLog:22
> +        Test: WebKit2.MinimumFontSizeLineHeight

Strange to use an API test for something like this. Isn’t there JS API exposed to the test tools that allows the to change settings in order to test things like this?

> Source/WebCore/ChangeLog:26
> +        (WebCore::StyleResolver::adjustRenderStyle): Perform the fixup.

Is this the same logic used by text-autosizing when it increases the line height? If so, it’s worth mentioning. If not, it’s worth saying why not.

> Source/WebCore/ChangeLog:36
> +        * style/StyleFontSizeFunctions.cpp: Clean up

Seems unrelated.

> Source/WebCore/ChangeLog:39
> +        (): Deleted.

Doesn’t make sense.

> Source/WebCore/css/StyleResolver.cpp:866
> +                if (minimumFontSize > 0 && style.computedFontSize() >= minimumFontSize && style.specifiedFontSize() < minimumFontSize) {

Can you explain the style.specifiedFontSize() < minimumFontSize test?

Did you test how this logic interacts with text zoom?

[Myles C. Maxfield]

Created attachment 315209 [details]
Patch

(In reply to Simon Fraser (smfr) from comment #3)
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1290
> > +    //ASSERT(scale == [webView->_scrollView zoomScale]);
> 
> Odd that you're hitting this.

That’s bug 167649.

[Build Bot]

Comment on attachment 315209 [details]
Patch

Attachment 315209 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4105844

New failing tests:
webrtc/filtering-ice-candidate-after-reload.html

[Build Bot]

Created attachment 315218 [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.12.5

[Myles C. Maxfield]

font sizes are non-negative
                 -%,      %,  Fixed,   Fixed
line-height: normal, number, length, percent
    ->     Normal => RenderStyle::initialLineHeight() => -100%
    ->     Length => text size adjust percentage * ???
    -> Percentage => font size * percentage, fixed
    ->     Number => value * text size adjust percentage, percentage
autosizing: percentage ? specifiedSize * percentage : value, fixed
RenderStyle::computedLineHeight()
    -> Negative => metrics line spacing
    ->        % => font size * value
    ->        _ => value
computed style:
	negative => metrics line spacing / zoom
               % => % * specified size / zoom
               _ => value / zoom

[Myles C. Maxfield]

Created attachment 315406 [details]
WIP

[Myles C. Maxfield]

Created attachment 315531 [details]
Patch

[Myles C. Maxfield]

Things to test:

All the different values of font-size
All the different values of line-height
style->effectiveZoom();
style->textZoom() (frame->textZoomFactor())
absolute sizes and non-absolute sizes
SVG and non-SVG
autosizing percentage

[Myles C. Maxfield]

Comment on attachment 315531 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315531&action=review

> Source/WebCore/css/StyleBuilderConverter.h:1439
> +        auto multiplier = computedFontSize / specifiedFontSize;

This new value should only be set on the computed style, not the specified style.

[Myles C. Maxfield]

Created attachment 315627 [details]
WIP

[Myles C. Maxfield]

Created attachment 315737 [details]
Needs autosizing tests

[Build Bot]

Comment on attachment 315737 [details]
Needs autosizing tests

Attachment 315737 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4139046

New failing tests:
fast/text/line-height-minimumFontSize.html

[Build Bot]

Created attachment 315757 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Myles C. Maxfield]

Created attachment 315770 [details]
Patch

[Build Bot]

Comment on attachment 315770 [details]
Patch

Attachment 315770 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4140146

New failing tests:
fast/text/line-height-minimumFontSize.html
fast/text/line-height-minimumFontSize-text-zoom.html
fast/text/line-height-minimumFontSize-zoom.html
fast/text/line-height-minimumFontSize-visual.html

[Build Bot]

Created attachment 315774 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315770 [details]
Patch

Attachment 315770 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4140166

New failing tests:
fast/text/line-height-minimumFontSize.html
fast/text/line-height-minimumFontSize-text-zoom.html
fast/text/line-height-minimumFontSize-zoom.html
fast/text/line-height-minimumFontSize-visual.html

[Build Bot]

Created attachment 315777 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315770 [details]
Patch

Attachment 315770 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4140176

New failing tests:
fast/text/line-height-minimumFontSize.html
fast/text/line-height-minimumFontSize-text-zoom.html
fast/text/line-height-minimumFontSize-zoom.html
fast/text/line-height-minimumFontSize-visual.html

[Build Bot]

Created attachment 315778 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315770 [details]
Patch

Attachment 315770 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4140184

New failing tests:
fast/text/line-height-minimumFontSize-autosize.html
fast/text/line-height-minimumFontSize-visual.html
fast/text/line-height-minimumFontSize.html
fast/text/line-height-minimumFontSize-zoom.html
fast/text/line-height-minimumFontSize-text-zoom.html

[Build Bot]

Created attachment 315780 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Myles C. Maxfield]

The tests pass on High Sierra; I have to figure out why they are failing on El Capitan.

[Myles C. Maxfield]

Created attachment 315848 [details]
Patch

[Build Bot]

Comment on attachment 315848 [details]
Patch

Attachment 315848 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4144540

New failing tests:
fast/text/line-height-minimumFontSize-autosize.html
fast/text/line-height-minimumFontSize.html

[Build Bot]

Created attachment 315861 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Myles C. Maxfield]

Created attachment 315865 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 315865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315865&action=review

> Source/WebCore/css/StyleBuilderCustom.h:655
> +    auto result = style.specifiedFontSize();

I had to look at the code to see that this was a float. Auto harmful?

> Source/WebCore/css/StyleBuilderCustom.h:707
> +        std::optional<Length> lineHeight = StyleBuilderConverter::convertLineHeight(styleResolver, value, multiplier);

If multiplier is still 1, aren't you just calling StyleBuilderConverter::convertLineHeight() with the same arguments as above, and doing redundant work?

> Source/WebCore/css/StyleBuilderCustom.h:708
> +        ASSERT(static_cast<bool>(lineHeight));

I would much rather read lineHeight != 0

[Myles C. Maxfield]

Comment on attachment 315865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315865&action=review

>> Source/WebCore/css/StyleBuilderCustom.h:708
>> +        ASSERT(static_cast<bool>(lineHeight));
> 
> I would much rather read lineHeight != 0

lineHeight is an optional.

[Myles C. Maxfield]

Committed r219646: <http://trac.webkit.org/changeset/219646>

[Matt Lewis]

The tests added in this patch are failing consistently on all platforms with both text and image failures.

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r219646%20(3042)/results.html

[Matt Lewis]

Reverted r219646 for reason:

The test added are failing on all platforms

Committed r219656: <http://trac.webkit.org/changeset/219656>

[Myles C. Maxfield]

Committed r219665: <http://trac.webkit.org/changeset/219665>

[Darin Adler]

Comment on attachment 315865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315865&action=review

>>> Source/WebCore/css/StyleBuilderCustom.h:708
>>> +        ASSERT(static_cast<bool>(lineHeight));
>> 
>> I would much rather read lineHeight != 0
> 
> lineHeight is an optional.

I would much rather see:

    ASSERT(lineHeight.has_value());

But also, calling value() also does that assertion. So if we are writing the assertion here, it is for documentation purposes rather than for the runtime check, which our version of std::optional takes care of automatically.