Summary: | Implement text-underline-offset and text-decoration-thickness | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, dbates, dino, don.olmstead, ews-watchlist, jonlee, saam, simon.fraser, thorton, webkit-bug-importer, zalan | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 190764, 191215, 191239, 191242, 191245, 196841 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2018-10-20 10:40:36 PDT
Created attachment 352858 [details]
Parsing support
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.
Created attachment 352859 [details]
Needs to actually move/resize the decorations
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 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? (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! Created attachment 353253 [details]
WIP
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 Created attachment 353335 [details]
WIP
Created attachment 353579 [details]
WIP
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) Created attachment 353951 [details]
Patch
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 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. Committed r237903: <https://trac.webkit.org/changeset/237903> *** Bug 167326 has been marked as a duplicate of this bug. *** *** Bug 167327 has been marked as a duplicate of this bug. *** 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 (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. (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. Confirmed as a PLT regression. |