Summary: | Setting `content: normal` on a ::marker should make computed style return resolved values | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||
Component: | CSS | Assignee: | Antoine Quint <graouts> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | akeerthi, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, kondapallykalyan, macpherson, menard, pdr, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Antoine Quint
2022-01-14 00:40:33 PST
Created attachment 449145 [details]
Patch
Created attachment 449153 [details]
Patch
Comment on attachment 449153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449153&action=review > Source/WebCore/rendering/style/RenderStyle.h:1474 > + bool hasContentNone() const { return m_nonInheritedFlags.hasExplicitlyClearedContent || styleType() == PseudoId::Before || styleType() == PseudoId::After; } Not quite following the logic of this patch. Like any ::before/after has content:none? > Source/WebCore/style/StyleBuilderCustom.h:1524 > + auto isEquivalentToNone = builderState.style().styleType() == PseudoId::Before || builderState.style().styleType() == PseudoId::After; > + builderState.style().setHasExplicitlyClearedContent(isEquivalentToNone); Seems like the flag should have a new name but I'm not sure what that would be (because above). (In reply to Antti Koivisto from comment #3) > Comment on attachment 449153 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449153&action=review > > > Source/WebCore/rendering/style/RenderStyle.h:1474 > > + bool hasContentNone() const { return m_nonInheritedFlags.hasExplicitlyClearedContent || styleType() == PseudoId::Before || styleType() == PseudoId::After; } > > Not quite following the logic of this patch. Like any ::before/after has > content:none? No, that's just wrong, it should be checking for hasContent() as well. I'll fix this. > > Source/WebCore/style/StyleBuilderCustom.h:1524 > > + auto isEquivalentToNone = builderState.style().styleType() == PseudoId::Before || builderState.style().styleType() == PseudoId::After; > > + builderState.style().setHasExplicitlyClearedContent(isEquivalentToNone); > > Seems like the flag should have a new name but I'm not sure what that would > be (because above). It should probably be: setHasContentNone() I think. I'll modify it. Created attachment 449196 [details]
Patch
Comment on attachment 449196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449196&action=review > Source/WebCore/rendering/style/RenderStyle.h:1474 > + bool hasContentNone() const { return !contentData() && (m_nonInheritedFlags.hasContentNone || styleType() == PseudoId::Before || styleType() == PseudoId::After); } I'm concerned the method name is potentially inaccurate. I understand that the `PseudoId::Before` and `PseudoId::After` checks are because the spec says "normal: For ::before and ::after, this computes to none." But the specified value is not none. I would name this method `hasEffectiveContentNone` or something similar to indicate this is speaking to the computed value. > Source/WebCore/rendering/style/RenderStyle.h:1475 > + bool hasContentNormal() const { return !contentData() && !hasContentNone(); } If we decide to go with the "effective" naming scheme above, this method no longer makes sense without also being renamed. But "effective" doesn't work here either, because "content: normal" could also compute to "contents". Since it only has one call site, I would remove the method entirely. Then, in RenderThemeIOS, update the condition to `return style.hasContent() || style.hasEffectiveContentNone()`). (In reply to Aditya Keerthi from comment #6) > Comment on attachment 449196 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449196&action=review > > > Source/WebCore/rendering/style/RenderStyle.h:1474 > > + bool hasContentNone() const { return !contentData() && (m_nonInheritedFlags.hasContentNone || styleType() == PseudoId::Before || styleType() == PseudoId::After); } > > I'm concerned the method name is potentially inaccurate. > > I understand that the `PseudoId::Before` and `PseudoId::After` checks are > because the spec says "normal: For ::before and ::after, this computes to > none." > > But the specified value is not none. I would name this method > `hasEffectiveContentNone` or something similar to indicate this is speaking > to the computed value. > > > Source/WebCore/rendering/style/RenderStyle.h:1475 > > + bool hasContentNormal() const { return !contentData() && !hasContentNone(); } > > If we decide to go with the "effective" naming scheme above, this method no > longer makes sense without also being renamed. But "effective" doesn't work > here either, because "content: normal" could also compute to "contents". > > Since it only has one call site, I would remove the method entirely. Then, > in RenderThemeIOS, update the condition to `return style.hasContent() || > style.hasEffectiveContentNone()`). Good points, I'll update the patch to do this. Created attachment 449211 [details]
Patch
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/imported/w3c/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Committed r288054 (?): <https://commits.webkit.org/r288054> |