Bug 190774 - Implement text-underline-offset and text-decoration-thickness
Summary: Implement text-underline-offset and text-decoration-thickness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 167326 167327 (view as bug list)
Depends on: 190764 191215 191239 191242 191245 196841
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-20 10:40 PDT by Myles C. Maxfield
Modified: 2019-04-11 17:44 PDT (History)
12 users (show)

See Also:


Attachments
Parsing support (30.75 KB, patch)
2018-10-20 10:41 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Needs to actually move/resize the decorations (29.92 KB, patch)
2018-10-20 11:57 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (54.39 KB, patch)
2018-10-28 03:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (125.26 KB, patch)
2018-10-29 17:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (125.26 KB, patch)
2018-10-31 20:50 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (45.68 KB, patch)
2018-11-06 01:21 PST, Myles C. Maxfield
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2018-10-20 10:40:36 PDT
Implement text-decoration-offset and text-decoration-width
Comment 1 Myles C. Maxfield 2018-10-20 10:41:43 PDT
Created attachment 352858 [details]
Parsing support
Comment 2 EWS Watchlist 2018-10-20 10:43:14 PDT
Attachment 352858 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Myles C. Maxfield 2018-10-20 11:57:14 PDT
Created attachment 352859 [details]
Needs to actually move/resize the decorations
Comment 4 EWS Watchlist 2018-10-20 11:58:27 PDT
Attachment 352859 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Don Olmstead 2018-10-22 11:31:18 PDT
Comment on attachment 352859 [details]
Needs to actually move/resize the decorations

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

Informal review

One question is whether there can just be a common base class here since they just mirror each other.

> Source/WebCore/rendering/style/TextDecorationOffset.h:99
> +    enum class Type {
> +        Auto,
> +        FromFont,
> +        Length
> +    };

Do you want to type this into a uint8_t?

> Source/WebCore/rendering/style/TextDecorationThickness.h:102
> +        : m_type(type)

We want to use {} for these now?
Comment 6 Myles C. Maxfield 2018-10-23 00:22:10 PDT
(In reply to Don Olmstead from comment #5)
> Comment on attachment 352859 [details]
> Needs to actually move/resize the decorations
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352859&action=review
> 
> Informal review
> 
> One question is whether there can just be a common base class here since
> they just mirror each other.

Yeah, my first version of the patch had them be the same class. However, due to https://github.com/w3c/csswg-drafts/issues/3118 I wanted to keep the implementation as flexible as possible in case the whole feature changes.

> 
> > Source/WebCore/rendering/style/TextDecorationOffset.h:99
> > +    enum class Type {
> > +        Auto,
> > +        FromFont,
> > +        Length
> > +    };
> 
> Do you want to type this into a uint8_t?

Yes, you're right!
> 
> > Source/WebCore/rendering/style/TextDecorationThickness.h:102
> > +        : m_type(type)
> 
> We want to use {} for these now?

Yes!
Comment 7 Myles C. Maxfield 2018-10-28 03:30:27 PDT
Created attachment 353253 [details]
WIP
Comment 8 Myles C. Maxfield 2018-10-28 03:32:49 PDT
Things remaining to do:

1. Make sure the repaint rects are correct (investigate RenderStyle::changeAffectsVisualOverflow(), visualOverflowForDecorations() and TextDecorationPainter::paintTextDecoration(), and computeUnderlineOffset())

2. Figure out what to do with maxLogicalBottomForTextDecorationLine()

3. Make sure flipped writing modes (like Mongolian) work

4. Make sure baselines (like CJK centered baseline) work correctly

5. Don's comments
Comment 9 Myles C. Maxfield 2018-10-29 17:10:56 PDT
Created attachment 353335 [details]
WIP
Comment 10 Myles C. Maxfield 2018-10-31 20:50:29 PDT
Created attachment 353579 [details]
WIP
Comment 11 Myles C. Maxfield 2018-10-31 21:04:52 PDT
Things remaining to do:

1. Add some repaint rect code in addTextBoxVisualOverflow()
2. Split this patch up so it's easier to review
3. Write some tests
3a. Tests for animations
3b. Tests for text-shadow and underlines
3c. Tests for flipped writing-mode
3d. Tests for ideographic baseline
3e. Tests for the direction the decoration is offset in the various modes
4. (Optional) Make the underline go behind the text (See https://bugs.webkit.org/show_bug.cgi?id=190764)
Comment 12 Myles C. Maxfield 2018-11-06 01:21:08 PST
Created attachment 353951 [details]
Patch
Comment 13 Don Olmstead 2018-11-06 10:22:05 PST
Comment on attachment 353951 [details]
Patch

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

LGTM. Just wondering about that constant.

> Source/WebCore/rendering/TextDecorationPainter.cpp:288
> +            int offset = computeUnderlineOffset(m_lineStyle.textUnderlinePosition(), m_lineStyle.textUnderlineOffset(), m_lineStyle.fontMetrics(), m_inlineTextBox, m_lineStyle.computedFontSize() / 16);

It worth moving from a magic number to a constant to give some meaning here?

> Source/WebCore/rendering/style/TextDecorationThickness.h:81
> +            const float textDecorationBaseFontSize = 16;

Here's that 16 again
Comment 14 Dean Jackson 2018-11-06 11:19:53 PST
Comment on attachment 353951 [details]
Patch

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

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1703
> +        new PropertyWrapper<TextDecorationThickness>(CSSPropertyTextDecorationThickness, &RenderStyle::textDecorationThickness, &RenderStyle::setTextDecorationThickness),
> +        new PropertyWrapper<TextUnderlineOffset>(CSSPropertyTextUnderlineOffset, &RenderStyle::textUnderlineOffset, &RenderStyle::setTextUnderlineOffset),

Animation! very nice.

>> Source/WebCore/rendering/TextDecorationPainter.cpp:288
>> +            int offset = computeUnderlineOffset(m_lineStyle.textUnderlinePosition(), m_lineStyle.textUnderlineOffset(), m_lineStyle.fontMetrics(), m_inlineTextBox, m_lineStyle.computedFontSize() / 16);
> 
> It worth moving from a magic number to a constant to give some meaning here?

Agreed.
Comment 15 Myles C. Maxfield 2018-11-06 17:22:53 PST
Committed r237903: <https://trac.webkit.org/changeset/237903>
Comment 16 Radar WebKit Bug Importer 2018-11-06 17:23:25 PST
<rdar://problem/45861917>
Comment 17 Myles C. Maxfield 2018-11-06 17:24:57 PST
*** Bug 167326 has been marked as a duplicate of this bug. ***
Comment 18 Myles C. Maxfield 2018-11-06 17:25:12 PST
*** Bug 167327 has been marked as a duplicate of this bug. ***
Comment 19 Saam Barati 2018-11-08 15:31:15 PST
Could this change have a performance impact? It's in the range where we have a 1-1.5% speedometer 2 regression on iOS.

https://trac.webkit.org/log/webkit/trunk?verbose=on&rev=237910&stop_rev=237896
Comment 20 Saam Barati 2018-11-08 15:38:44 PST
(In reply to Saam Barati from comment #19)
> Could this change have a performance impact? It's in the range where we have
> a 1-1.5% speedometer 2 regression on iOS.
> 
> https://trac.webkit.org/log/webkit/
> trunk?verbose=on&rev=237910&stop_rev=237896

This is an even narrower range:
https://trac.webkit.org/log/webkit/trunk?verbose=on&rev=237903&stop_rev=237899

This patch seems like the likely culprit.
Comment 21 Myles C. Maxfield 2018-11-09 14:24:50 PST
(In reply to Saam Barati from comment #20)
> (In reply to Saam Barati from comment #19)
> > Could this change have a performance impact? It's in the range where we have
> > a 1-1.5% speedometer 2 regression on iOS.
> > 
> > https://trac.webkit.org/log/webkit/
> > trunk?verbose=on&rev=237910&stop_rev=237896
> 
> This is an even narrower range:
> https://trac.webkit.org/log/webkit/
> trunk?verbose=on&rev=237903&stop_rev=237899
> 
> This patch seems like the likely culprit.

I could believe it. This patch deals with repaint rects of text; causing text to repaint more often could be a regression.
Comment 22 Chris Dumez 2019-04-11 16:17:59 PDT
Confirmed as a PLT regression.