Closed Bug 205202 Opened 21 years ago Closed 5 years ago

Support ::marker pseudo-element

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: sluggo, Assigned: MatsPalmgren_bugz)

References

(Blocks 4 open bugs, )

Details

(Keywords: css3, dev-doc-complete)

Attachments

(7 files, 5 obsolete files)

22.53 KB, patch
emilio
: review+
Details | Diff | Splinter Review
459 bytes, text/html
Details
59.99 KB, patch
emilio
: feedback+
Details | Diff | Splinter Review
80.04 KB, patch
emilio
: feedback+
Details | Diff | Splinter Review
15.73 KB, patch
emilio
: feedback+
Details | Diff | Splinter Review
499 bytes, text/html
Details
12.17 KB, patch
emilio
: feedback+
Details | Diff | Splinter Review
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:
Summary: Support CSS3 li::marker pseudo class → Support CSS3 li::marker pseudo element
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!
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Severity: normal → enhancement
Keywords: css3
Summary: Support CSS3 li::marker pseudo element → Support ::marker pseudo element
Blocks: 54979
Priority: -- → P1
Target Milestone: --- → Future
Depends on: 288704
QA Contact: ian → style-system
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.
Assignee: dbaron → nobody
Priority: P1 → P4
Target Milestone: Future → ---
Status: ASSIGNED → NEW
Does this still require bug 215083 in order to be implemented?
Flags: needinfo?(dbaron)
I don't think so.  Did it ever?

(Sorry for the delay responding.)
Flags: needinfo?(dbaron)
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.
Blocks: css-pseudo-4
Summary: Support ::marker pseudo element → Support ::marker pseudo-element
Attached patch Test updatesSplinter Review

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

Attachment #9035152 - Flags: review?(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/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?

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

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

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.

Flags: needinfo?(mats)

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

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

Flags: needinfo?(mats)

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.

(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

Attached file 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.

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

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

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.

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

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.

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.

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

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

Attachment #9035146 - Attachment is obsolete: true
Attachment #9035146 - Flags: review?(emilio)
Attachment #9035774 - Flags: review?(emilio)

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)

Attachment #9048336 - Flags: feedback?(emilio)

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 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!
Attachment #9048353 - Flags: feedback?(emilio) → feedback+
Comment on attachment 9035152 [details] [diff] [review]
Test updates

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

r=me on this patch
Attachment #9035152 - Flags: review?(emilio) → review+
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.
Attachment #9048336 - Flags: feedback?(emilio) → feedback-
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)
Attachment #9035774 - Flags: review?(emilio) → review-
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.
Attachment #9048351 - Flags: feedback?(emilio)

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.

Attachment #9048336 - Attachment is obsolete: true

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

Attachment #9035774 - Attachment is obsolete: true
Attachment #9051372 - Flags: feedback?(emilio)

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

Attachment #9051374 - Flags: feedback?(emilio)
Attached file 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 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 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);
}
Attachment #9051372 - Flags: feedback?(emilio) → feedback+
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.
Attachment #9051374 - Flags: feedback?(emilio) → feedback-
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?
Attachment #9051375 - Flags: feedback?(emilio) → feedback+

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

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

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

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

Attachment #9048351 - Attachment is obsolete: true
Attachment #9051374 - Attachment is obsolete: true
Attachment #9053108 - Flags: feedback?(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 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.
Attachment #9053108 - Flags: feedback?(emilio) → feedback+

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

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

Flags: needinfo?(jkratzer)

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

Pushed by mpalmgren@mozilla.com:
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
Depends on: 1538589
Depends on: 1538618
No longer blocks: 1221416

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

Flags: needinfo?(jkratzer)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16094 for changes under testing/web-platform/tests
No longer depends on: 1539656
Regressions: 1539656
Regressions: 1543328
Depends on: 1544959
Regressions: 1552719
No longer regressions: 1552719

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

Regressions: 1574398
Depends on: 1616078
Regressions: 1648365
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: