Bug 196338 - [SimpleLineLayout] Disable SLL when text-underline-position is not auto.
Summary: [SimpleLineLayout] Disable SLL when text-underline-position is not auto.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-27 21:59 PDT by zalan
Modified: 2019-03-28 15:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.47 KB, patch)
2019-03-27 22:16 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2019-03-28 07:40 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2019-03-28 07:42 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-03-27 21:59:43 PDT
we don't support text-underline-position yet.
Comment 1 zalan 2019-03-27 22:00:03 PDT
<rdar://problem/47975167>
Comment 2 zalan 2019-03-27 22:16:52 PDT
Created attachment 366153 [details]
Patch
Comment 3 Daniel Bates 2019-03-27 22:49:28 PDT
Comment on attachment 366153 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Disable simple line layout unconditionally on non-auto text-underline-position content. We don's support it yet. 

Don?

Changelog order does not look correct. This line should be under the next line. See other change logs.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position-expected.html:1
> +<html>

Ok as-is. Bothers me there is no doctype, but that is just me and no quirks to trip over.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position-expected.html:5
> +<body></body>

Haha. Got it, SLL disabled = no output just like a page with an empty body. 😀

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:1
> +<html>

Ok as-is. Test uses quirks mode, but this seems unnecessary.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:3
> +<title>This tests that simple line layout is disabled for text-underline-position: under</title>

Ok as-is. I personally don't like long web page titles. WPT like it. I don't. Want to know more then ask me. No need to appease me though.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:10
> +    overflow:hidden;

Ok as-is. Inconsistent style. (Alexey would say this adds randomness, but to me it just bothers me though no need to do anything; ok as-is.) Look at other lines if you want to.

> LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:24
> +<div class=first>Pass if after selecting these 2 lines</div>

Ok as-is. Text does not make sense to me. Test does not look like it programmatically selects anything.
Comment 4 zalan 2019-03-28 07:40:42 PDT
Created attachment 366172 [details]
Patch
Comment 5 zalan 2019-03-28 07:42:26 PDT
Created attachment 366173 [details]
Patch
Comment 6 zalan 2019-03-28 07:49:40 PDT
> Ok as-is. I personally don't like long web page titles. WPT like it. I
> don't. Want to know more then ask me. No need to appease me though.
I am curious. Will swing by.

> 
> > LayoutTests/fast/text/simple-line-layout-with-text-underline-position.html:24
> > +<div class=first>Pass if after selecting these 2 lines</div>
> 
> Ok as-is. Text does not make sense to me. Test does not look like it
> programmatically selects anything.
This is for me when in 9months this test starts failing on the bots, I can just read the text and test it quickly.
Comment 7 zalan 2019-03-28 09:02:31 PDT
Committed r243605: <https://trac.webkit.org/changeset/243605>
Comment 8 Simon Fraser (smfr) 2019-03-28 09:45:04 PDT
Comment on attachment 366173 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Disable simple line layout unconditionally on non-auto text-underline-position content. We don't support it yet. 

I would have liked this to describe the symptoms.
Comment 9 zalan 2019-03-28 10:47:19 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 366173 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366173&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Disable simple line layout unconditionally on non-auto text-underline-position content. We don't support it yet. 
> 
> I would have liked this to describe the symptoms.
the disappearing text? (like with most of the SLL patches)
Comment 10 Simon Fraser (smfr) 2019-03-28 15:15:19 PDT
(In reply to zalan from comment #9)
> (In reply to Simon Fraser (smfr) from comment #8)
> > Comment on attachment 366173 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=366173&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Disable simple line layout unconditionally on non-auto text-underline-position content. We don't support it yet. 

"This fixes a bug on Pearson test content where text would disappear after selecting, then using one of the overlay tools".