Bug 232729

Summary: Rename TextDecoration to TextDecorationLine
Product: WebKit Reporter: Nikos Mouchtaris <nmouchtaris>
Component: CSSAssignee: Nikos Mouchtaris <nmouchtaris>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, changseok, dino, dmazzoni, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jcraig, jdiggs, kondapallykalyan, macpherson, menard, mifenton, mmaxfield, ntim, pdr, sabouhallawa, samuel_white, schenney, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 230083    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mmaxfield: review+
Patch
none
Patch none

Description Nikos Mouchtaris 2021-11-04 15:30:25 PDT
Rename TextDecoration to TextDecorationLine
Comment 1 Nikos Mouchtaris 2021-11-04 16:32:13 PDT
Created attachment 443354 [details]
Patch
Comment 2 Nikos Mouchtaris 2021-11-04 18:05:44 PDT
Created attachment 443365 [details]
Patch
Comment 3 Nikos Mouchtaris 2021-11-05 12:36:42 PDT
Created attachment 443424 [details]
Patch
Comment 4 Myles C. Maxfield 2021-11-05 16:57:22 PDT
Why is there no change to CSSProperties.json?
Comment 5 Myles C. Maxfield 2021-11-05 17:01:46 PDT
(In reply to Myles C. Maxfield from comment #4)
> Why is there no change to CSSProperties.json?

Oh, text-decoration-line already appears in CSSProperties.json. It looks like we have text-decoration, which has `"converter": "TextDecoration"`, and we also have -webkit-text-decoration, which is a longhand for text-decoration-line (and 2 others)

🤔
Comment 6 Myles C. Maxfield 2021-11-05 17:06:39 PDT
Neither this bug, nor the changelog in this patch, nor the blocked bug (https://bugs.webkit.org/show_bug.cgi?id=230083) describe how this patch fits into the overall strategy for how we're going to achieve https://bugs.webkit.org/show_bug.cgi?id=230083. It's difficult to review this without understanding how this patch fits into an overall strategy.
Comment 7 Tim Nguyen (:ntim) 2021-11-06 09:53:26 PDT
(In reply to Myles C. Maxfield from comment #6)
> Neither this bug, nor the changelog in this patch, nor the blocked bug
> (https://bugs.webkit.org/show_bug.cgi?id=230083) describe how this patch
> fits into the overall strategy for how we're going to achieve
> https://bugs.webkit.org/show_bug.cgi?id=230083. It's difficult to review
> this without understanding how this patch fits into an overall strategy.

text-decoration right now is implemented as a keyword property: underline/overline/strikethrough/etc.

The spec says those keywords should be for the text-decoration-line property, and text-decoration should be a shorthand for text-decoration-line/color/style/thickness.

Renaming the TextDecoration enum to TextDecorationLine reduces ambiguity.
Comment 8 Radar WebKit Bug Importer 2021-11-11 14:31:21 PST
<rdar://problem/85315983>
Comment 9 Tim Nguyen (:ntim) 2021-11-15 15:09:54 PST
Comment on attachment 443424 [details]
Patch

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

> Source/WebCore/rendering/style/StyleVisualData.cpp:31
> +    , textDecorationLine(RenderStyle::initialTextDecoration().toRaw())

Since this is linked to the property name, rather than the value type, I would rename this specific field in the other bug.
Comment 10 Myles C. Maxfield 2021-11-15 19:56:11 PST
Comment on attachment 443424 [details]
Patch

Oh, this is just the enum. I guess that's fine.
Comment 11 Nikos Mouchtaris 2021-11-16 12:13:26 PST
Created attachment 444420 [details]
Patch
Comment 12 Nikos Mouchtaris 2021-11-16 16:31:51 PST
Created attachment 444448 [details]
Patch
Comment 13 EWS 2021-11-16 17:27:05 PST
Committed r285904 (244317@main): <https://commits.webkit.org/244317@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444448 [details].