WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(84.09 KB, patch)
2017-07-11 22:43 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(19.13 KB, patch)
2017-07-11 23:09 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
WIP
(10.33 KB, patch)
2017-07-13 23:57 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(5.00 KB, patch)
2017-07-14 21:40 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
WIP
(5.19 KB, patch)
2017-07-16 15:05 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Needs autosizing tests
(22.25 KB, patch)
2017-07-17 17:28 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(50.48 KB, patch)
2017-07-17 23:25 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
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
Details
Patch
(50.82 KB, patch)
2017-07-18 15:48 PDT
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(50.90 KB, patch)
2017-07-18 17:44 PDT
,
Myles C. Maxfield
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2017-07-11 17:25:23 PDT
Created
attachment 315195
[details]
WIP
Myles C. Maxfield
Comment 2
2017-07-11 17:25:51 PDT
<
rdar://problem/10139227
>
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
Created
attachment 315207
[details]
Patch
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
Created
attachment 315209
[details]
Patch
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
Created
attachment 315406
[details]
WIP
Myles C. Maxfield
Comment 13
2017-07-14 21:40:37 PDT
Created
attachment 315531
[details]
Patch
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
Created
attachment 315627
[details]
WIP
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
Created
attachment 315770
[details]
Patch
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
Created
attachment 315848
[details]
Patch
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
Created
attachment 315865
[details]
Patch
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
Committed
r219646
: <
http://trac.webkit.org/changeset/219646
>
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
Committed
r219665
: <
http://trac.webkit.org/changeset/219665
>
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.
Top of Page
Format For Printing
XML
Clone This Bug