RESOLVED FIXED 174406
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
Summary Setting the minimum font size preference doesn’t affect absolute line-height ...
Myles C. Maxfield
Reported 2017-07-11 17:25:01 PDT
[iOS] Emails with line-height set have overlapping text when accessibility text size is increased
Attachments
WIP (19.01 KB, patch)
2017-07-11 17:25 PDT, Myles C. Maxfield
no flags
Patch (84.09 KB, patch)
2017-07-11 22:43 PDT, Myles C. Maxfield
no flags
Patch (19.13 KB, patch)
2017-07-11 23:09 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (995.76 KB, application/zip)
2017-07-12 02:12 PDT, Build Bot
no flags
WIP (10.33 KB, patch)
2017-07-13 23:57 PDT, Myles C. Maxfield
no flags
Patch (5.00 KB, patch)
2017-07-14 21:40 PDT, Myles C. Maxfield
no flags
WIP (5.19 KB, patch)
2017-07-16 15:05 PDT, Myles C. Maxfield
no flags
Needs autosizing tests (22.25 KB, patch)
2017-07-17 17:28 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (939.94 KB, application/zip)
2017-07-17 20:16 PDT, Build Bot
no flags
Patch (50.48 KB, patch)
2017-07-17 23:25 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (1.24 MB, application/zip)
2017-07-18 00:24 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.45 MB, application/zip)
2017-07-18 00:31 PDT, Build Bot
no flags
Archive of layout-test-results from ews117 for mac-elcapitan (2.04 MB, application/zip)
2017-07-18 00:43 PDT, Build Bot
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.59 MB, application/zip)
2017-07-18 00:53 PDT, Build Bot
no flags
Patch (50.82 KB, patch)
2017-07-18 15:48 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (945.27 KB, application/zip)
2017-07-18 17:13 PDT, Build Bot
no flags
Patch (50.90 KB, patch)
2017-07-18 17:44 PDT, Myles C. Maxfield
simon.fraser: review+
Myles C. Maxfield
Comment 1 2017-07-11 17:25:23 PDT
Myles C. Maxfield
Comment 2 2017-07-11 17:25:51 PDT
Simon Fraser (smfr)
Comment 3 2017-07-11 17:48:40 PDT
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 4 2017-07-11 20:39:15 PDT
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
Comment 5 2017-07-11 22:43:05 PDT
mitz
Comment 6 2017-07-11 23:05:43 PDT
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
Comment 7 2017-07-11 23:09:03 PDT
mitz
Comment 8 2017-07-12 00:41:33 PDT
(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 9 2017-07-12 02:12:23 PDT
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
Comment 10 2017-07-12 02:12:25 PDT
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
Comment 11 2017-07-13 23:56:00 PDT
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
Comment 12 2017-07-13 23:57:16 PDT
Myles C. Maxfield
Comment 13 2017-07-14 21:40:37 PDT
Myles C. Maxfield
Comment 14 2017-07-14 21:41:14 PDT
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 15 2017-07-15 22:57:24 PDT
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
Comment 16 2017-07-16 15:05:01 PDT
Myles C. Maxfield
Comment 17 2017-07-17 17:28:59 PDT
Created attachment 315737 [details] Needs autosizing tests
Build Bot
Comment 18 2017-07-17 20:16:57 PDT
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
Comment 19 2017-07-17 20:16:59 PDT
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
Comment 20 2017-07-17 23:25:40 PDT
Build Bot
Comment 21 2017-07-18 00:24:05 PDT
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
Comment 22 2017-07-18 00:24:07 PDT
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 23 2017-07-18 00:31:09 PDT
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
Comment 24 2017-07-18 00:31:10 PDT
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 25 2017-07-18 00:43:20 PDT
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
Comment 26 2017-07-18 00:43:21 PDT
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 27 2017-07-18 00:53:38 PDT
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
Comment 28 2017-07-18 00:53:39 PDT
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
Comment 29 2017-07-18 09:27:45 PDT
The tests pass on High Sierra; I have to figure out why they are failing on El Capitan.
Myles C. Maxfield
Comment 30 2017-07-18 15:48:11 PDT
Build Bot
Comment 31 2017-07-18 17:13:45 PDT
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
Comment 32 2017-07-18 17:13:46 PDT
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
Comment 33 2017-07-18 17:44:04 PDT
Simon Fraser (smfr)
Comment 34 2017-07-18 19:21:46 PDT
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 35 2017-07-18 19:31:46 PDT
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
Comment 36 2017-07-18 20:54:45 PDT
Matt Lewis
Comment 37 2017-07-19 09:36:47 PDT
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
Comment 38 2017-07-19 09:42:58 PDT
Reverted r219646 for reason: The test added are failing on all platforms Committed r219656: <http://trac.webkit.org/changeset/219656>
Myles C. Maxfield
Comment 39 2017-07-19 16:38:12 PDT
Darin Adler
Comment 40 2017-07-20 17:51:26 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.