Summary: | Rename TextDecoration to TextDecorationLine | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikos Mouchtaris <nmouchtaris> | ||||||||||||
Component: | CSS | Assignee: | 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
Nikos Mouchtaris
2021-11-04 15:30:25 PDT
Created attachment 443354 [details]
Patch
Created attachment 443365 [details]
Patch
Created attachment 443424 [details]
Patch
Why is there no change to CSSProperties.json? (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) 🤔 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. (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 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 on attachment 443424 [details]
Patch
Oh, this is just the enum. I guess that's fine.
Created attachment 444420 [details]
Patch
Created attachment 444448 [details]
Patch
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]. |