WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
190774
Implement text-underline-offset and text-decoration-thickness
https://bugs.webkit.org/show_bug.cgi?id=190774
Summary
Implement text-underline-offset and text-decoration-thickness
Myles C. Maxfield
Reported
2018-10-20 10:40:36 PDT
Implement text-decoration-offset and text-decoration-width
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2018-10-20 10:41:43 PDT
Created
attachment 352858
[details]
Parsing support
EWS Watchlist
Comment 2
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.
Myles C. Maxfield
Comment 3
2018-10-20 11:57:14 PDT
Created
attachment 352859
[details]
Needs to actually move/resize the decorations
EWS Watchlist
Comment 4
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.
Don Olmstead
Comment 5
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?
Myles C. Maxfield
Comment 6
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!
Myles C. Maxfield
Comment 7
2018-10-28 03:30:27 PDT
Created
attachment 353253
[details]
WIP
Myles C. Maxfield
Comment 8
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
Myles C. Maxfield
Comment 9
2018-10-29 17:10:56 PDT
Created
attachment 353335
[details]
WIP
Myles C. Maxfield
Comment 10
2018-10-31 20:50:29 PDT
Created
attachment 353579
[details]
WIP
Myles C. Maxfield
Comment 11
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
)
Myles C. Maxfield
Comment 12
2018-11-06 01:21:08 PST
Created
attachment 353951
[details]
Patch
Don Olmstead
Comment 13
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
Dean Jackson
Comment 14
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.
Myles C. Maxfield
Comment 15
2018-11-06 17:22:53 PST
Committed
r237903
: <
https://trac.webkit.org/changeset/237903
>
Radar WebKit Bug Importer
Comment 16
2018-11-06 17:23:25 PST
<
rdar://problem/45861917
>
Myles C. Maxfield
Comment 17
2018-11-06 17:24:57 PST
***
Bug 167326
has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 18
2018-11-06 17:25:12 PST
***
Bug 167327
has been marked as a duplicate of this bug. ***
Saam Barati
Comment 19
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
Saam Barati
Comment 20
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.
Myles C. Maxfield
Comment 21
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.
Chris Dumez
Comment 22
2019-04-11 16:17:59 PDT
Confirmed as a PLT regression.
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