205202 - Support ::marker pseudo-element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Kim Scarborough]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.4a) Gecko/20030419
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.4a) Gecko/20030419

If bug 26710 is WONTFIX on the grounds that CSS3 won't use "display: marker",
then Mozilla should support what CSS3 *will* use, namely "li::marker" (see
<http://www.w3.org/TR/css3-lists/#markers>). Currently, there is no way in
Mozilla to make an ordered list like this (for instance):

1) Groucho
2) Chico
3) Harpo

Reproducible: Always

Steps to Reproduce:

Comment 1

[Hixie (not reading bugmail)]

Please don't file bugs for missing features. CSS3 Lists isn't in CR yet -- we
don't know for sure that ::marker will exist. Even if it did, CSS2.1 features,
and even more importantly, CSS2.1 bugs, are a much higher priority than CSS3.

Bugs for new features will be filed if someone implements them.

Thanks!

Comment 2

[Tony Mechelynck [:tonymec]]

In the meantime, we have ::-moz-list-bullet (which is documented but will obviously never be a standard under that name) and ::-moz-list-number (which is not even documented yet). A personalized list like that mentioned in comment #0 can be achieved, but not easily, by means of CSS3 counters conforming to the current W3C "Draft recommendation": see bug 593739 comment #0 for a similar example with the "jumping through hoops" solution I found.

Comment 3

[Gordon P. Hemsley [:GPHemsley]]

Does this still require bug 215083 in order to be implemented?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think so.  Did it ever?

(Sorry for the delay responding.)

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

To support ::marker, I guess we'll need to support the name alias for pseudo element since ::-moz-list-bullet and ::-moz-list-number will become the aliases for ::marker. I don't found the similar alias mechanism like CSS properties.

Comment 6

[Steven Vachon]

Do it.

Comment 7

[Mats Palmgren (inactive)]

Attachment: [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Test updates

FYI, the WPT "marker-font-properties.html" really PASS on all platforms
but there's some baseline alignment issue still on OSX/Windows.
It might be that the test is just making bad assumptions since
WebKit seems to have a similar problem given the comment:
https://searchfox.org/mozilla-central/rev/c3ebaf6de2d481c262c04bb9657eaf76bf47e2ac/testing/web-platform/tests/css/css-pseudo/marker-font-properties.html#30

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 9035146 [details] [diff] [review]
[css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

Review of attachment 9035146 [details] [diff] [review]:

::: layout/base/nsGenConList.cpp
@@ +60,5 @@

}

• if (pseudo == nsCSSPseudoElements::marker()) {
• *aContent = aFrame->GetContent()->GetParent();
• return -2;
• }

If its return value is smaller than ::before, why not move it before that?

Comment 10

[Mats Palmgren (inactive)]

Sure, I can add it first in PseudoCompareType() instead, if that's what
you're suggesting.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9035146 [details] [diff] [review]
[css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

Review of attachment 9035146 [details] [diff] [review]:

::: layout/base/RestyleManager.h
@@ +290,4 @@

 nsTArray<RefPtr<nsIContent>> mContents;
 nsTArray<RefPtr<nsIContent>> mBeforeContents;
 nsTArray<RefPtr<nsIContent>> mAfterContents;

• nsTArray<RefPtr<nsIContent>> mMarkerContents;

I don't think that per spec ::marker is supposed to get animated and such. Probably birtles can confirm.

::: layout/style/nsComputedDOMStyle.cpp
@@ +135,5 @@

   if (EffectSet::GetEffectSet(aElement, CSSPseudoElementType::after)) {
     return true;
   }

• } else if (aPseudo == nsCSSPseudoElements::marker()) {

•  if (EffectSet::GetEffectSet(aElement, CSSPseudoElementType::marker)) {
  

If marker cannot get animations like ::before or ::after then neither this doesn't need to get added either.

::: servo/components/selectors/parser.rs
@@ +2370,5 @@

     ) -> Result<PseudoElement, SelectorParseError<'i>> {
         match_ignore_ascii_case! { &name,
             "before" => return Ok(PseudoElement::Before),
             "after" => return Ok(PseudoElement::After),

•            "marker" => return Ok(PseudoElement::Marker),
  

This is test code, can go away.

::: servo/components/style/matching.rs
@@ +778,5 @@

                 // actually having a useful pseudo-element.  Check for that
                 // case.
                 let pseudo = PseudoElement::from_eager_index(i);
                 let new_pseudo_should_exist =

•                    new.as_ref().map_or(false, |s| pseudo.should_exist(new_primary_style, s));
  

Instead of passing the primary style here we should instead add the logic that uses it to TElement::may_generate_pseudo on dom.rs, which would allow us to stop selector-matching the marker altogether.

But this may be problematic, let me elaborate in the bug.

::: servo/components/style/servo/selector_parser.rs
@@ +81,5 @@

     use self::PseudoElement::*;
     dest.write_str(match *self {
         After => "::after",
         Before => "::before",

•        Marker => "::marker",
  

This is servo-only code, so not needed either.

@@ +204,5 @@

 /// Note: Keep this in sync with EAGER_PSEUDO_COUNT.
 #[inline]
 pub fn cascade_type(&self) -> PseudoElementCascadeType {
     match *self {

•        PseudoElement::After | PseudoElement::Before | PseudoElement::marker | PseudoElement::Selection => {
  

Also I don't think this would build (lowercase marker vs. Marker).

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9035146 [details] [diff] [review]
[css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

Review of attachment 9035146 [details] [diff] [review]:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1714,2 @@

*/
void nsCSSFrameConstructor::CreateGeneratedContentItem(

So we're supporting random 'content: "foo"' and counters and such on markers? that seems really wild.

https://drafts.csswg.org/css-pseudo-4/#marker-pseudo doesn't say anything like that, nor other browsers support it either.

Without supporting this we may be able to end up with a much simpler setup.

@@ +5273,5 @@

                                       ComputedStyle& aStyle,
                                       nsIFrame* aParentFrame, nsAtom* aTag,
                                       uint32_t aFlags) {

• // A ::marker pseudo creates a nsBulletFrame.
• if (aTag == nsGkAtoms::mozgeneratedcontentmarker) {

This is a bug, and it's pretty unsafe. I can do document.createElement("_moz_generated_content_marker"), and then start putting nsBulletFrames everywhere, which is not good.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

So I'm a bit confused about why the approach of making it an eager pseudo-element instead of using the same setup we have for all the pseudo-elements that are not ::before / ::after / ::first-letter / ::first-line (which are the 'weird' pseudos).

Mats, could you elaborate on why taking this approach instead of the regular approach other psuedo-elements take (that is, nsIAnonymousContentCreator + SetPseudoElementType)?

Seems to me off-hand that it'd be both easier and more efficient to implement ::marker like that.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

We may not even need nsIAnonymousContentCreator's CreateAnonymousContent really, if creating bullets is already very special-casey, as long as we teach nsBlockFrame that it may get a native anonymous bullet child as a special-case out-of-band).

Comment 15

[Mats Palmgren (inactive)]

Well, ::marker is a "tree-abiding pseudo element" just like ::before/::after
https://drafts.csswg.org/css-pseudo-4/#treelike

You're right that the spec for ::marker there currently says:
"... however at the moment marker box layout is not fully defined, so only these properties are allowed."
which means 'content' is excluded.

I think this is a mistake and filed a spec issue to include it:
https://github.com/w3c/csswg-drafts/issues/3499

As you can see in my patch, I haven't included support for 'content' yet,
but I see no problems in supporting it. In fact, I think it would be
easier to implement if ::marker is treated just like a variation of
::before/::after and hopefully this will make it easier to fix some
decades old bugs in our implementation. (Ideally, we could delete
nsBulletFrame entirely and create boxes for the generated content
the same way as ::before/::after).

Comment 16

[Emilio Cobos Álvarez [:emilio]]

The issue with supporting ::marker this way is that I suspect it's going to cause a lot of headache in the style system.

In particular, we either match it / compute it / store it eagerly for every element like we do for ::before and ::after, which is not great because it's inefficient, or we don't, but then when display changes to list-item we need to selector-match, which is not something we're very well prepared to do right now...

For example I'm pretty sure that with your patch dynamically toggling display to list-item using the style attribute will not make ::marker appear properly.

Comment 17

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

In particular, we either match it / compute it / store it eagerly for every element like we do for ::before and ::after, which is not great because it's inefficient

Hmm, shouldn't we try to fix this performance issue then? Perhaps we could
create it lazily only if it generates a box or if someone explicitly queries
its style through CSSOM?

Just curious, was this perf issue also present in the old style system,
or is it a new issue with Stylo?

FYI, the CSSWG has already resolved that ::marker exists on all elements:
https://github.com/w3c/csswg-drafts/issues/1793#issuecomment-380762861

Comment 18

[Mats Palmgren (inactive)]

Attachment: Testcase #1

For example I'm pretty sure that with your patch dynamically toggling display to list-item using the style attribute will not make ::marker appear properly.

This simple testcase seems to work fine, fwiw.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

(In reply to Mats Palmgren (:mats) from comment #18)

This simple testcase seems to work fine, fwiw.

Sure, I suspect you need some sort of #x::marker rule, which then would not properly apply.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

(In reply to Mats Palmgren (:mats) from comment #17)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

In particular, we either match it / compute it / store it eagerly for every element like we do for ::before and ::after, which is not great because it's inefficient

Hmm, shouldn't we try to fix this performance issue then? Perhaps we could
create it lazily only if it generates a box or if someone explicitly queries
its style through CSSOM?

Just curious, was this perf issue also present in the old style system,
or is it a new issue with Stylo?

This applied to the old style engine as well as far as I'm aware (I'd say the old style engine would be worse in the dynamic restyle case even, IIRC how it worked).

I'm not aware of any engine doing something smarter / better than us, fwiw, but I could see WebKit and Blink not excited about adding another one like this (I'm definitely not excited).

There are multiple issues with lazily creating them. The first is detecting we need to reframe due to one of them changing, the second is not losing some style system optimizations. Right now we match them but not store them if the ::before / ::after pseudo-element would not generate a box. That memory optimization already introduces some weirdness, like needing to check whether the pseudo-element inherits the 'display' or 'content' properties:

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/servo/components/style/style_resolver.rs#119

Otherwise our style system optimizations wouldn't be sound.

We don't resolve them in display: none subtrees though, and for that case and for the case where we don't store it because the style has no content or is display: none we do resolve them lazily if somebody queries them via CSSOM.

FYI, the CSSWG has already resolved that ::marker exists on all elements:
https://github.com/w3c/csswg-drafts/issues/1793#issuecomment-380762861

Yeah, that's fine, but it doesn't create a box for (almost) all elements like ::before and ::after.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Given the list marker exists whether or not rules match, and only for some kinds of frames, I'd say it's better to copy the setup for ::placeholder rather than the one for ::before / ::after. ::placeholder is similarly a tree-abiding pseudo.

Comment 22

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)

Sure, I suspect you need some sort of #x::marker rule, which then would not properly apply.

Adding #x::marker { color: green; } to the testcase make the "1." green.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Oh, looking at the patch I see how it works. The reason it works is because you're storing the marker style for every element (you're not touching eager_pseudo_is_definitely_not_generated).

Which I guess is the same thing we do for ::first-line / ::first-letter, but it's not great... I can try to think a bit more about it while I review the other patches, but I still think it may be a less complex setup to do what ::placeholder does.

Comment 24

[Emilio Cobos Álvarez [:emilio]]

I think it's worse than what we do for those, since we have a global |::marker rule and thus every element will end up with a style for it.

Comment 25

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

Yeah, that's fine, but it doesn't create a box for (almost) all elements like ::before and ::after.

Well, I'm contemplating to propose that ::marker should apply to all
elements, not just list items. Given that the pseudo exists anyway,
it seems like a missed opportunity to not be able to create a box
for it in other situations. I'll wait to see what the CSSWG says
about applying 'content' first though.

Comment 26

[Mats Palmgren (inactive)]

Attachment: [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

• if (aTag == nsGkAtoms::mozgeneratedcontentmarker) {

This is a bug, and it's pretty unsafe. I can do document.createElement("_moz_generated_content_marker"), and then start putting nsBulletFrames everywhere, which is not good.

Ah, good catch. I changed it to check for the pseudo style instead:

• if (aStyle.GetPseudo() == nsCSSPseudoElements::marker()) {

(Also addressed Xidorn's aesthetic nit in comment 9.)

Comment 27

[Mats Palmgren (inactive)]

Attachment: addendum - eager_pseudo_is_definitely_not_generated

I'll fold this into the existing patch.
This updates eager_pseudo_is_definitely_not_generated() to handle ::marker.

(abusing the feedback flag for review since this patch queue is still managed by MQ)

Comment 28

[Mats Palmgren (inactive)]

FYI, fantasai says "We do intend that it will eventually apply." about the 'content' property on ::marker.
https://github.com/w3c/csswg-drafts/issues/3499#issuecomment-453857880

Comment 29

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements.

Comment 30

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-lists][css-pseudo] Rename various uses of bullet with marker to avoid any misleading association with nsBulletFrame (idempotent patch).

Comment 31

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9048353 [details] [diff] [review]
part 4 - [css-lists][css-pseudo] Rename various uses of bullet with marker to avoid any misleading association with nsBulletFrame (idempotent patch).

Review of attachment 9048353 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks!

Comment 32

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9035152 [details] [diff] [review]
Test updates

Review of attachment 9035152 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on this patch

Comment 33

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9048336 [details] [diff] [review]
addendum - eager_pseudo_is_definitely_not_generated

Review of attachment 9048336 [details] [diff] [review]:
-----------------------------------------------------------------

This is not needed because you're not making ::marker an eager pseudo.

Comment 34

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9035774 [details] [diff] [review]
[css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser.

Review of attachment 9035774 [details] [diff] [review]:
-----------------------------------------------------------------

So as it turns out, the reason your patches don't make the test-cases I pointed out should fail in comment 12, is because you didn't turn it into an eager pseudo at all (even though you added code to handle it in a few places).

The place to do that would be:

  https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/servo/components/style/gecko/pseudo_element_definition.mako.rs#23

But as I said I think not doing that is ok. So I think this patch is mostly ok, but should clean up a bunch of code.

I'm also unsure about making more pseudos animatable, worth checking out with birtles. If you decide to make it animatable then there are quite a few assertions that you need to update. I'd probably do it in a separate bug if we decide to do so.

Also, this is adding an element which is not reachable from AllChildrenIterator. I'm pretty sure that's wrong. The only reason updating the inherited style works is because we special-case the marker in nsBlockFrame::UpdatePseudoElementStyles. That still leaves a stale style in the element, which is bad. It still mostly works because for getComputedStyle we end up using the frame's style.

I'm pretty sure nsBlockFrame::ResolveMarkerStyle and the marker bit in UpdatePseudoElementStyles can go away with the ChildIterator change, which is great.

::: layout/base/RestyleManager.cpp
@@ +1858,5 @@
>  
>  void RestyleManager::AnimationsWithDestroyedFrame ::
>      StopAnimationsForElementsWithoutFrames() {
>    StopAnimationsWithoutFrame(mContents, CSSPseudoElementType::NotPseudo);
> +  StopAnimationsWithoutFrame(mMarkerContents, CSSPseudoElementType::marker);

I don't think you want to make ::marker animatable, at least just yet. Please talk with Birtles about that, if you want to.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1737,5 @@
>      return;
>    }
>  
> +  nsAtom* elemName = nullptr;
> +  nsAtom* property;

I'd either initialize both or none.

::: layout/style/ServoStyleSet.cpp
@@ +1257,5 @@
>      }
> +  } else if (aPseudoType == CSSPseudoElementType::marker) {
> +    if (Element* pseudo = nsLayoutUtils::GetMarkerPseudo(aElement)) {
> +      elementForStyleResolution = pseudo;
> +      pseudoTypeForStyleResolution = CSSPseudoElementType::NotPseudo;

I think this is only needed to handle animations properly, so this could go away.

::: layout/style/nsComputedDOMStyle.cpp
@@ +135,5 @@
>        if (EffectSet::GetEffectSet(aElement, CSSPseudoElementType::after)) {
>          return true;
>        }
> +    } else if (aPseudo == nsCSSPseudoElements::marker()) {
> +      if (EffectSet::GetEffectSet(aElement, CSSPseudoElementType::marker)) {

If we decide not to animate marker for now, then this bit goes away.

::: layout/style/res/ua.css
@@ -129,3 @@
>    font-variant-numeric: tabular-nums;
> -  /* Prevent the element from being selected when clicking on the marker. */
> -  -moz-user-select: none;

I guess this gets handled ok via:

  https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/layout/generic/nsFrame.cpp#4018

Right?

::: servo/components/selectors/parser.rs
@@ +2197,5 @@
>      #[derive(Clone, Debug, Eq, PartialEq)]
>      pub enum PseudoElement {
>          Before,
>          After,
> +        Marker,

This is just test code so I wouldn't bother.

::: servo/components/style/gecko/pseudo_element.rs
@@ +48,5 @@
>  
>  impl PseudoElement {
>      /// Returns the kind of cascade type that a given pseudo is going to use.
>      ///
> +    /// In Gecko we only compute ::before, ::after and ::marker eagerly.

This is wrong, you're not making ::marker an eager pseudo. I'll explain below or above, depending on the other comments :)

@@ +178,5 @@
>      }
>  
>      /// Whether this pseudo-element should actually exist if it has
>      /// the given styles.
> +    pub fn should_exist(&self, primary_style: &ComputedValues, style: &ComputedValues) -> bool {

Let's add a `debug_assert!(self.is_eager());` here for clarity.

@@ +187,5 @@
>          if self.is_before_or_after() && style.ineffective_content_property() {
>              return false;
>          }
>  
> +        if *self == PseudoElement::Marker {

And this code can go away.

::: servo/components/style/invalidation/element/invalidator.rs
@@ +541,5 @@
>          if let Some(anon_content) = self.element.xbl_binding_anonymous_content() {
>              any_descendant |= self.invalidate_dom_descendants_of(anon_content, invalidations);
>          }
>  
> +        if let Some(marker) = self.element.marker_pseudo_element() {

I wonder if we should make this code more generic and just doing it while we append NAC... Seems like it'd be faster for elements which don't have before / after / ::marker.

In any case not this bug's business, so this is fine.

::: servo/components/style/matching.rs
@@ +778,5 @@
>                      // actually having a useful pseudo-element.  Check for that
>                      // case.
>                      let pseudo = PseudoElement::from_eager_index(i);
>                      let new_pseudo_should_exist =
> +                        new.as_ref().map_or(false, |s| pseudo.should_exist(new_primary_style, s));

this may or may not need to change, depending on whether we make them lazy or not.

::: servo/components/style/servo/selector_parser.rs
@@ +100,5 @@
>      }
>  }
>  
>  /// The number of eager pseudo-elements. Keep this in sync with cascade_type.
> +pub const EAGER_PSEUDO_COUNT: usize = 4;

This is servo code, so these changes are not needed.

@@ +204,5 @@
>      /// Note: Keep this in sync with EAGER_PSEUDO_COUNT.
>      #[inline]
>      pub fn cascade_type(&self) -> PseudoElementCascadeType {
>          match *self {
> +            PseudoElement::After | PseudoElement::Before | PseudoElement::marker | PseudoElement::Selection => {

This wouldn't even build (s/marker/Marker)

Comment 35

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9048351 [details] [diff] [review]
part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements.

Review of attachment 9048351 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok on the surface, but it seems it doesn't work that well on my testing, a simple test ends up showing missing content and such.

Also, this needs a bunch of tests. I'd probably land this in a separate bug.

Comment 36

[Mats Palmgren (inactive)]

Hmm, it turns out that simply touching some of the <summary> tests
makes TVg1 and TVg3 fail on win32 only (not win64, nor any other platform):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc8a5ea160ca68d321cd27cb33356ef59d13487b&group_state=expanded&selectedJob=233540764
That is, make some change to them in one patch, then revert that
change in the next so that the file is exactly as the original.
I assume the test framework checks the date of the file or which
files are included in the patches or something to decide whether
to run these tests.

I assumed that it was my changes here or bug 288704 that made
them fail but it turns out it's an existing bug. Sigh.
Filed bug 1534867.

Comment 37

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items. Alias :-moz-list-bullet/number to that in the parser

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)

Also, this is adding an element which is not reachable from
AllChildrenIterator.

OK, I added it there.

I'm pretty sure nsBlockFrame::ResolveMarkerStyle and the marker bit in
UpdatePseudoElementStyles can go away with the ChildIterator change, which
is great.

OK, removed those.

I don't think you want to make ::marker animatable, at least just yet.
Please talk with Birtles about that, if you want to.

Sure, we can do animation in a separate follow-up bug.

• /* Prevent the element from being selected when clicking on the marker. */
• -moz-user-select: none;

I guess this gets handled ok via:

https://searchfox.org/mozilla-central/rev/
3e0f1d95fcf8832413457e3bec802113bdd1f8e8/layout/generic/nsFrame.cpp#4018

Right?

Yes.

::: servo/components/selectors/parser.rs
@@ +2197,5 @@

 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum PseudoElement {
     Before,
     After,

•    Marker,
  

This is just test code so I wouldn't bother.

OK.

::: servo/components/style/gecko/pseudo_element.rs

• pub fn should_exist(&self, primary_style: &ComputedValues, style: &ComputedValues) -> bool {

Let's add a debug_assert!(self.is_eager()); here for clarity.

OK, I reverted this change and added the assertion.

::: servo/components/style/matching.rs

•                    new.as_ref().map_or(false, |s| pseudo.should_exist(new_primary_style, s));
  

this may or may not need to change, depending on whether we make them lazy
or not.

Removed.

::: servo/components/style/servo/selector_parser.rs

+pub const EAGER_PSEUDO_COUNT: usize = 4;

This is servo code, so these changes are not needed.

Oh. It would be nice if we could just "hg rm" all Servo-only code in m-c.
(I'm assuming Servo lives somewhere upstream in github.)

====

Additionally, I fixed an existing bug with display:block ::markers, e.g.:
data:text/html,<style>::-moz-list-number { display:block; }</style><ol><li>a

We need to make its inline-size shrink-wrap in this case (the change to
ReflowBullet). We also need to skip the CSS2 block margin calculations
for this (and other) cases since the end-margin will be very large
otherwise (the change in ReflowInput.cpp).

Comment 38

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements

This looks ok on the surface, but it seems it doesn't work that well on my testing, a simple test ends up showing missing content and such.

Yeah, it turns out ReflowBullet doesn't work very well for inlines.
I've fixed this by blockifying 'display' for outside bullets.
I think that's acceptable (for outside bullets) also from a spec
perspective, which says they "count as absolutely positioned"
which implies blockification.

I think some parts of position:marker [1] are questionable though,
such as the stacking order for painting which I believe are not
web-compatible. (I'll file a spec issue about this)
Blockification seems like a reasonable minimum requirement though.

[1] https://drafts.csswg.org/css-lists-3/#position-marker

So, the only new change here is the blockification in
style_adjuster.rs

Comment 39

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-lists][css-pseudo] Add web-platform tests for ::marker 'content'

Comment 40

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=203f63498a9c8d60d227e070dc8d4a4bf4f1832b
(note that the TVg2 and TVg3 failures can be ignored: bug 1534867)

Comment 41

[Mats Palmgren (inactive)]

Attachment: Testcase #2

BTW, the dynamic 'class' test still doesn't work with these patches.
It seems an additional reflow is required before it renders correctly.
I think we can probably fix that as a follow-up bug in case it's
rare in practice... unless you can think of an easy fix. WDYT?

Comment 42

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9051374 [details] [diff] [review]
part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements

Review of attachment 9051374 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/style_adjuster.rs
@@ +205,5 @@
>  
>          blockify_if!(self.style.floated());
>          blockify_if!(self.style.out_of_flow_positioned());
> +        blockify_if!(self.style.pseudo == Some(&PseudoElement::Marker) &&
> +                     self.style.get_list().clone_list_style_position() == ListStylePosition::Outside);

Shouldn't this look at the parent list style position instead?

Comment 43

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9051372 [details] [diff] [review]
part 1 - [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items.  Alias :-moz-list-bullet/number to that in the parser

Review of attachment 9051372 [details] [diff] [review]:
-----------------------------------------------------------------

Can we land a bunch of tests for the tricky dynamic changes and such?

 * Changing the generation of ::marker via changing display on the parent tweaking the style attribute.
 * The test that's failing.
 * Probably a few others...

Looks good to me with these changes.

::: dom/base/ChildIterator.cpp
@@ +336,1 @@
>    if (mPhase == eAtBegin || mPhase == eAtBeforeKid) {

I think you need / can remove the eAtBegin check here now.

@@ +363,5 @@
>  }
>  
>  nsIContent* AllChildrenIterator::GetNextChild() {
>    if (mPhase == eAtBegin) {
> +    Element* markerContent = nsLayoutUtils::GetMarkerPseudo(mOriginalContent);

For consistency, you should set mPhase to eAtBeforeKid, and remove eAtBegin check below.

@@ +459,2 @@
>  
>      Element* beforeContent = nsLayoutUtils::GetBeforePseudo(mOriginalContent);

And should probably set eAtMarkerKid here, and remove the eAtExplicitKids check below.

::: layout/base/RestyleManager.cpp
@@ +2718,5 @@
> +        !nsLayoutUtils::GetMarkerPseudo(aElement)) {
> +      RefPtr<ComputedStyle> pseudoStyle =
> +          aRestyleState.StyleSet().ProbePseudoElementStyle(
> +              *aElement, PseudoStyleType::marker, styleFrame->Style());
> +      if (pseudoStyle) {

The reason this doesn't work as you mentioned is because we're returning the cached style instead of computing it again.

So either make the marker pseudo uncacheable or, even better, pass the up-to-date parent style here. I think we shouldn't probe this all the time if the element wasn't restyled, so that needs a bit more work. Fine to leave as-is, I can fix as a followup.

::: layout/base/RestyleManager.h
@@ +263,5 @@
>          MOZ_ASSERT(aContent->NodeInfo()->NameAtom() ==
>                     nsGkAtoms::mozgeneratedcontentafter);
>          mAfterContents.AppendElement(aContent->GetParent());
> +      } else if (pseudoType == PseudoStyleType::marker) {
> +        MOZ_ASSERT(aContent->NodeInfo()->NameAtom() ==

Given animations are a followup, let's remove this.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +9741,5 @@
>    NS_ASSERTION(!allowFirstPseudos || !aFrame->IsXULBoxFrame(),
>                 "can't be both block and box");
>  
> +  if (listItem) {
> +    if (auto markerFrame = nsLayoutUtils::GetMarkerFrame(aContent)) {

auto*

@@ +9742,5 @@
>                 "can't be both block and box");
>  
> +  if (listItem) {
> +    if (auto markerFrame = nsLayoutUtils::GetMarkerFrame(aContent)) {
> +      for (auto childFrame : aFrameItems) {

auto*

@@ +9743,5 @@
>  
> +  if (listItem) {
> +    if (auto markerFrame = nsLayoutUtils::GetMarkerFrame(aContent)) {
> +      for (auto childFrame : aFrameItems) {
> +        if (markerFrame == childFrame) {

Maybe:

if (markerFrame != childFrame) {
 continue;
}

And deident the rest.

::: layout/generic/nsBulletFrame.cpp
@@ +168,1 @@
>                                       hasBullet);

nit: indentation

::: layout/style/AnimationCommon.h
@@ +125,5 @@
>             (mTarget.mPseudoType == PseudoStyleType::before &&
> +            aOther.mTarget.mPseudoType == PseudoStyleType::after) ||
> +           (mTarget.mPseudoType == PseudoStyleType::marker &&
> +            aOther.mTarget.mPseudoType == PseudoStyleType::before) ||
> +           (mTarget.mPseudoType == PseudoStyleType::marker &&

Let's remove this too.

::: layout/style/GeckoBindings.cpp
@@ +487,5 @@
>      return PseudoStyleType::after;
>    }
>  
> +  if (aElementOrPseudo->IsGeneratedContentContainerForMarker()) {
> +    aElementOrPseudo = aElementOrPseudo->GetParent()->AsElement();

Let's remove this too.

::: layout/style/nsCSSPseudoElements.cpp
@@ +118,5 @@
>      case PseudoStyleType::before:
>        return NS_LITERAL_STRING("::before");
>      case PseudoStyleType::after:
>        return NS_LITERAL_STRING("::after");
> +    case PseudoStyleType::marker:

This is only used for animations, so let's remove.

::: servo/components/style/gecko/pseudo_element_definition.mako.rs
@@ +201,5 @@
>              }
>              "-moz-placeholder" => {
>                  return Some(PseudoElement::Placeholder);
>              }
> +            "-moz-list-bullet" => {

nit: Can do:

"-moz-list-bullet" | "-moz-list-number" => {
    return Some(PseudoElement::Marker);
}

Comment 44

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9051374 [details] [diff] [review]
part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements

I don't think the style adjuster change is right. Please add a reftest using:

li {
  list-style-position: outside;
}
li::marker {
  list-style-position: inside;
}

and the other way around too.

Comment 45

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9051375 [details] [diff] [review]
part 5 - [css-lists][css-pseudo] Add web-platform tests for ::marker 'content'

Review of attachment 9051375 [details] [diff] [review]:
-----------------------------------------------------------------

Can we also add to WPT the dynamic style change tests?

::: testing/web-platform/tests/css/css-pseudo/marker-content-001.html
@@ +19,5 @@
> +ib { font-size: 32pt; }
> +</style>
> +</head>
> +<body>
> +<ol><li></li><li>B</li><li><ib>C</ib></li></ol>

Does ib stand for ib-split? There's no ib-split here, right? Can we use span?

::: testing/web-platform/tests/css/css-pseudo/marker-content-008.html
@@ +10,5 @@
> +li {
> +  list-style-position: outside;
> +}
> +li::marker {
> +  content: "a" "b";

Can you add a test for content: url() as well?

Comment 46

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #42)

•    blockify_if!(self.style.pseudo == Some(&PseudoElement::Marker) &&
  

•                 self.style.get_list().clone_list_style_position() == ListStylePosition::Outside);
  

Shouldn't this look at the parent list style position instead?

Yeah, I think you're right.

Comment 47

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #43)

if (mPhase == eAtBegin || mPhase == eAtBeforeKid) {

I think you need / can remove the eAtBegin check here now.

OK, yeah, that's unnecessary since it's never true.

nsIContent* AllChildrenIterator::GetNextChild() {
if (mPhase == eAtBegin) {

• Element* markerContent = nsLayoutUtils::GetMarkerPseudo(mOriginalContent);

For consistency, you should set mPhase to eAtBeforeKid, and remove eAtBegin
check below.

Hmm, no I don't think that would work. If mPhase == eAtBegin,
and we have no ::marker, then setting it to eAtBeforeKid and only
checking mPhase == eAtMarkerKid in the next clause means we won't
check for a ::before element.

I guess we could unconditionally set it to eAtMarkerKid, but doesn't
that make the code slightly harder to follow given there's now
a dependency between that assignment and the next clause?
It's a matter of taste I guess...

 Element* beforeContent = nsLayoutUtils::GetBeforePseudo(mOriginalContent);

And should probably set eAtMarkerKid here, and remove the eAtExplicitKids
check below.

No, I don't think that works. It means we won't try to find
a ::marker element. Did you mean eAtBeforeKid perhaps?

::: layout/base/RestyleManager.cpp

•  RefPtr<ComputedStyle> pseudoStyle =
  

•      aRestyleState.StyleSet().ProbePseudoElementStyle(
  

•          *aElement, PseudoStyleType::marker, styleFrame->Style());
  

•  if (pseudoStyle) {
  

The reason this doesn't work as you mentioned is because we're returning the
cached style instead of computing it again.

Ah, OK. I'll leave this for you to fix in a follow-up then. Thanks.

Comment 48

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #44)

li {
list-style-position: outside;
}
li::marker {
list-style-position: inside;
}

and the other way around too.

I've added a WPT for these combinations (and more), testing/web-platform/tests/css/css-pseudo/marker-list-style-position.html: https://hg.mozilla.org/try/rev/50ff1edc6a35f93b53866dcfba5011cf1c951530

(In reply to Emilio Cobos Álvarez (:emilio) from comment #45)

+<ol><li></li><li>B</li><li><ib>C</ib></li></ol>

Does ib stand for ib-split? There's no ib-split here, right? Can we use span?

I've changed it to <span> in all these tests.

::: testing/web-platform/tests/css/css-pseudo/marker-content-008.html

• content: "a" "b";

Can you add a test for content: url() as well?

Sure, I've added a couple of url() tests too.
(and updated the assertion condition here as a result)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae0e82413db621d7111cf518a3a114a4c6d2a1ff
(The Try run has a depressing amount of orange, but none due to my changes AFAICT.)

Comment 49

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements

Updated style_adjuster to look at the parent list-style-position instead.

Comment 50

[Emilio Cobos Álvarez [:emilio]]

(In reply to Mats Palmgren (:mats) from comment #47)

For consistency, you should set mPhase to eAtBeforeKid, and remove eAtBegin
check below.

Hmm, no I don't think that would work. If mPhase == eAtBegin,
and we have no ::marker, then setting it to eAtBeforeKid and only
checking mPhase == eAtMarkerKid in the next clause means we won't
check for a ::before element.

I guess we could unconditionally set it to eAtMarkerKid, but doesn't
that make the code slightly harder to follow given there's now
a dependency between that assignment and the next clause?
It's a matter of taste I guess...

Yeah, it's ok to not do this.

 Element* beforeContent = nsLayoutUtils::GetBeforePseudo(mOriginalContent);

And should probably set eAtMarkerKid here, and remove the eAtExplicitKids
check below.

No, I don't think that works. It means we won't try to find
a ::marker element. Did you mean eAtBeforeKid perhaps?

Yeah, I did mean that, sorry :)

::: layout/base/RestyleManager.cpp

•  RefPtr<ComputedStyle> pseudoStyle =
  

•      aRestyleState.StyleSet().ProbePseudoElementStyle(
  

•          *aElement, PseudoStyleType::marker, styleFrame->Style());
  

•  if (pseudoStyle) {
  

The reason this doesn't work as you mentioned is because we're returning the
cached style instead of computing it again.

Ah, OK. I'll leave this for you to fix in a follow-up then. Thanks.

Make sure to file it and ni? me please :)

Comment 51

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9053108 [details] [diff] [review]
part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements

Review of attachment 9053108 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, please file a csswg issue requesting to add the blockification and such?

::: layout/base/nsCSSFrameConstructor.cpp
@@ +5316,5 @@
>                                            nsIFrame* aParentFrame,
>                                            uint32_t aFlags) {
> +  // A ::marker pseudo creates a nsBulletFrame, unless 'content' was set.
> +  if (aStyle.GetPseudoType() == PseudoStyleType::marker &&
> +      aStyle.StyleContent()->ContentCount() == 0) {

nit: I'd probably just write !aStyle.StyleContent()->ContentCount(), but it's a matter of preference I guess :)

::: servo/components/style/style_adjuster.rs
@@ +205,5 @@
>  
>          blockify_if!(self.style.floated());
>          blockify_if!(self.style.out_of_flow_positioned());
> +        blockify_if!(self.style.pseudo == Some(&PseudoElement::Marker) &&
> +                     layout_parent_style.get_list().clone_list_style_position() == ListStylePosition::Outside);

self.style.get_parent_list().clone_list_style_position() instead.

layout_parent_style will always be the same in this case (layout_parent_style is the parent style but jumping across display: contents), but it's conceptually the wrong thing to do.

Comment 52

[Emilio Cobos Álvarez [:emilio]]

I'd also consider landing the content patches in a separate bug, but no big deal I guess?

Comment 53

[Emilio Cobos Álvarez [:emilio]]

Jason & co., can you add ::marker to the CSS fuzzers if it's not there already?

Comment 54

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #51)

self.style.get_parent_list().clone_list_style_position() instead.

layout_parent_style will always be the same in this case
(layout_parent_style is the parent style but jumping across display:
contents), but it's conceptually the wrong thing to do.

Yeah, I agree.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b415c553b7b2
part 1 - [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items.  Alias :-moz-list-bullet/number to that in the parser.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c4a873d52c
part 2 - [css-lists][css-pseudo] Add support for the ::marker pseudo element on list items.  Test updates.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/556f58dde212
part 3 - [css-lists][css-pseudo] Add support for the 'content' CSS property on ::marker pseudo elements.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1731591a0a
part 4 - [css-lists][css-pseudo] Rename various uses of bullet with marker to avoid any misleading association with nsBulletFrame (idempotent patch).  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bad25ccc8d0
part 5 - [css-lists][css-pseudo] Add web-platform tests for ::marker 'content'.  r=emilio

Comment 56

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b415c553b7b2
https://hg.mozilla.org/mozilla-central/rev/b5c4a873d52c
https://hg.mozilla.org/mozilla-central/rev/556f58dde212
https://hg.mozilla.org/mozilla-central/rev/ae1731591a0a
https://hg.mozilla.org/mozilla-central/rev/9bad25ccc8d0

Comment 58

[Jason Kratzer [:jkratzer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #53)

Jason & co., can you add ::marker to the CSS fuzzers if it's not there already?

Emilio, we weren't fuzzing this previously but it has been added. Thanks for the heads up.

Comment 59

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16094 for changes under testing/web-platform/tests

Comment 60

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/16094
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Uj0YJ2quSJiE9Bu8NSAT4g)

Comment 62

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/16094
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/eRyN_rLBSrO_9xZza6L8GQ)

Comment 64

[Rachel Andrew]

I have updated the MDN docs page for ::marker, there is also a note in the 68 release notes.