Setting `content: normal` on a ::marker should make computed style return resolved values

[Antoine Quint]

Setting `content: normal` on a ::marker should make computed style return resolved values

[Antoine Quint]

Created attachment 449145 [details]
Patch

[Antoine Quint]

Created attachment 449153 [details]
Patch

[Antti Koivisto]

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).

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 449196 [details]
Patch

[Aditya Keerthi]

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()`).

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 449211 [details]
Patch

[EWS]

/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).

[Antoine Quint]

Committed r288054 (?): <https://commits.webkit.org/r288054>

[Radar WebKit Bug Importer]

<rdar://problem/87637233>