Bug 235222

Summary: Setting `content: normal` on a ::marker should make computed style return resolved values
Product: WebKit Reporter: Antoine Quint <graouts>
Component: CSSAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch akeerthi: review+, ews-feeder: commit-queue-

Description Antoine Quint 2022-01-14 00:40:33 PST
Setting `content: normal` on a ::marker should make computed style return resolved values
Comment 1 Antoine Quint 2022-01-14 00:48:49 PST
Created attachment 449145 [details]
Patch
Comment 2 Antoine Quint 2022-01-14 02:27:47 PST
Created attachment 449153 [details]
Patch
Comment 3 Antti Koivisto 2022-01-14 05:20:29 PST
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).
Comment 4 Antoine Quint 2022-01-14 07:19:12 PST
(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.
Comment 5 Antoine Quint 2022-01-14 12:02:48 PST
Created attachment 449196 [details]
Patch
Comment 6 Aditya Keerthi 2022-01-14 12:54:19 PST
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()`).
Comment 7 Antoine Quint 2022-01-14 13:04:08 PST
(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.
Comment 8 Antoine Quint 2022-01-14 13:09:23 PST
Created attachment 449211 [details]
Patch
Comment 9 EWS 2022-01-15 05:26:52 PST
/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).
Comment 10 Antoine Quint 2022-01-15 05:32:20 PST
Committed r288054 (?): <https://commits.webkit.org/r288054>
Comment 11 Radar WebKit Bug Importer 2022-01-15 05:33:16 PST
<rdar://problem/87637233>