Support ::marker pseudo-element

NEW
Assigned to

Status

()

P4
enhancement
16 years ago
a month ago

People

(Reporter: sluggo, Assigned: mats)

Tracking

(Depends on: 2 bugs, Blocks: 5 bugs, {css3, dev-doc-needed})

Trunk
css3, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
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
Last Resolved: 16 years ago
Resolution: --- → INVALID

Updated

15 years ago
Severity: normal → enhancement
Keywords: css3
Summary: Support CSS3 li::marker pseudo element → Support ::marker pseudo element

Updated

15 years ago
Blocks: 54979

Updated

14 years ago
Depends on: 288704
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)
Keywords: dev-doc-needed
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.
Depends on: 1265639
Comment hidden (advocacy)
Blocks: 1443969
Summary: Support ::marker pseudo element → Support ::marker pseudo-element
(Assignee)

Comment 7

a month ago

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

Assignee: nobody → mats
Attachment #9035146 - Flags: review?(emilio)
(Assignee)

Comment 8

a month ago

Created attachment 9035152 [details] [diff] [review]
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

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?

(Assignee)

Comment 10

a month ago

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

(Assignee)

Comment 15

a month ago

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.

(Assignee)

Comment 17

a month ago

(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

(Assignee)

Comment 18

a month ago

Created attachment 9035477 [details]
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.

(Assignee)

Comment 22

a month ago

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

(Assignee)

Comment 25

a month ago

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

(Assignee)

Comment 26

a month ago

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

(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)
You need to log in before you can comment on or make changes to this bug.