| Summary: | Setting the minimum font size preference doesn’t affect absolute line-height values, so lines overlap | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, darin, dino, jlewis3, jonlee, mitz, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 174534, 174536 | ||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||
|
Description
Myles C. Maxfield
2017-07-11 17:25:01 PDT
Created attachment 315195 [details]
WIP
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. 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. 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? 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. 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 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
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
Created attachment 315406 [details]
WIP
Created attachment 315531 [details]
Patch
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 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. Created attachment 315627 [details]
WIP
Created attachment 315737 [details]
Needs autosizing tests
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 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
Created attachment 315770 [details]
Patch
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 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
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 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
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 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
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 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
The tests pass on High Sierra; I have to figure out why they are failing on El Capitan. Created attachment 315848 [details]
Patch
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 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
Created attachment 315865 [details]
Patch
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 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. Committed r219646: <http://trac.webkit.org/changeset/219646> 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 Reverted r219646 for reason: The test added are failing on all platforms Committed r219656: <http://trac.webkit.org/changeset/219656> Committed r219665: <http://trac.webkit.org/changeset/219665> 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. |