Closed Bug 62843 Opened 24 years ago Closed 21 years ago

[FIX][CSS3] Need to support '::' for pseudo-elements in the CSS parser

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: attinasi, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: css3, Whiteboard: [Hixie-P3])

Attachments

(1 file, 5 obsolete files)

Patch with all the comments fixed
17.62 KB, patch
Details | Diff | Splinter Review 
CSS3 proposes the use of :: for pseudo-elements instead of the : that was used
for pseudo-elements and pseudo-classes in CSS2. We should put in support for ::
in pseudo-elements now in preparation for CSS3.

NOTE: we must support both : and :: for pseudo-element selectors, but only : for
pseudo-classes.

Daniel, feel free to take a stab at this if you arte bored ;)
Status: NEW → ASSIGNED
Note that we should only support ':' for four pseudo-elements:

   :before
   :after
   :first-line
   :first-letter

All other pseudo-elements should only work with "::".

That is, once CSS3 Selectors has been released. That should happen by 0.9, so 
that should not be a problem. However if CSS3 Selectors fails to go through
last call, then we should not check a fix for this in until it has.
Keywords: css3, mozilla1.0
Even if the WD does not go to last call, it will be public and the test suite
attached to the document will be public too... and people will run the last
mozilla through them.
Anyway, the code parsing pseudos probably needs a little cleaning : if ':ident'
is not a pseudo-class, it is considered to be a pseudo-element (that pseudo can
be unrecognized, of course). A pseudo-elem is added to the selector context
using nsCSSSelector::AddPseudoClass... It is probably the occasion to separate a
little bit the two notions. I am currently trying to work on it but no
re-assignment please...
In pre-release builds we can do what we like. However, in a final, released
version it is imperative that we do not implement features that have WD status
in the same namespace as the final CR or REC. This is the whole point of the
-vendor-XXX stuff for identifiers. 

In addition, there is the slight problem that convincing the Powers That Be that
we should change this kind of bug back to the REC version at the last minute is
an almost impossible task.

Therefore, if we _do_ implement this, I would plead that it be either controlled
by an #ifdef, or a pref, or an environment variable, and that the default be
off. That way it is easy [for me] to test, it is easy to toggle on and off, and
we can make the required changes much closer to the End Game.

Now bearing all that in mind, I would love to see an experimental implementation
of this CSS3 stuff, so go right ahead! :-D
The patch above is intended for future use, when the CSS 3 Selectors Module will
go to CR.
This change looks good to me. Since the likelihood of this changing between now
and the time css3 is published is nil, I don't even mind that it is no #ifdef'd
out. The only result is that selectors written using the double-colon for
pseudo-elements will be parsed as valid now, whereas technically they should not
because CSS2 does not allow it. Because the CSS3 selector module is really close
to being made public, I think we are safe here.
Call me a maniac but here is how I think these changes should be done...

1) The NS_CSS_GETINFO flags should be part of the declaration of the CSS_ATOMs.  
It means that nsCSSAtomList.h should declare at the same time the atom name and 
the NS_CSS_GETINFO flags, similarly to what we have in nsCSSPropList.h for the 
properties and the NS_STYLE_HINT.

2) Rewrite GetPseudoInfoMask() so that it takes into account these new 
declarations and so that we don't have to modify it each time we want to add a 
CSS3 pseudo.

3) Change the test in IsSinglePseudoClass

from:

     if (!bFoundDoubleColon && 
         (0 == (NS_CSS_GETINFO_CSS1 | NS_CSS_GETINFO_CSS2)
             & GetPseudoInfoMask(pseudo))) {


to:

     if (!bFoundDoubleColon && 
         (NS_CSS_GETINFO_CSS3 & GetPseudoInfoMask(pseudo))) {


Also, but I'm not sure it will ever be used because it doesn't seem to correspond 
to any standard, CSSParserImpl::GetInfoMask() should be updated to return 
NS_CSS_GETINFO_CSS1/CSS2/CSS3. I don't know what CSSP and CSS_FROSTING are for...

Agreed; but please note this is strictly independant of the original code
proposal and the proposed css3 enhancement. Your last comment is more related to
code cleanup...
css3...
Summary: Need to support '::' for pseudo-elements in the CSS parser → [CSS3] Need to support '::' for pseudo-elements in the CSS parser
Adding dependency to 52381 because for nsCSSSelector::ToString output of new
css3 pseudo-elements...
Depends on: 52381
Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
Reassigning to Daniel since he has made the fix.
Assignee: attinasi → glazman
Status: ASSIGNED → NEW
oops; i thought I already had accepted this bug....

note : recent modifications will imply some changes in the attached patch.
Status: NEW → ASSIGNED
setting to moz1.0 for now
Target Milestone: --- → mozilla1.0
Attached patch patch v3 (obsolete) — — Splinter Review 
new patch for this bug.

1. Adds support for css3 "::" notation for pseudo-elements
2. Gathers all atoms for pseudos in nsCSSAtomList.h
3. Adds -moz- to proprietary pseudos
4. Comment out unused pseudo atoms
5. uses cpp magic in IsPseudoClass()

Pierre, Marc and David, can you review please ?
Comment on attachment 59513 [details] [diff] [review]
patch v3

Is the patch missing changes to the htmlAtoms table? It looks like that table
has been changed but I don't see the diffs for that. Seems minor, so
sr=attinasi anyway (assuming you either forgot the diff or I am mistaken)
Attachment #59513 - Flags: superreview+
Marc: right, just a bug in my cvs diff line ; there is nothing to worry about,
I just move some atom definitions from nsHTMLAtoms to nsCSSAtoms.
Grumble *%$¤3!&£### !!! I have just discovered that there are definitions of
pseudos in other header files in content/shared ; why the hell had we at least 3
places of atom definitions for css pseudos (or proprietary extensions) ? That
kills me. Patch v3 is then obsolete.
Comment on attachment 59513 [details] [diff] [review]
patch v3

missing other definitions of atoms ! That seems an endless story, pseudos all
over the place.
Moving from Moz1.0 to future-P1.  All bugs scheduled for Moz1.0 need to
nominated using the nsbeta1 keyword.
Priority: P3 → P1
Target Milestone: mozilla1.0 → Future
Blocks: 128482
cc'ing myself
Daniel, have you given up? :)
Whiteboard: [Hixie-P3]
I started some other pseudo cleanup work in bug 147887 (see also the
dependencies of that bug).  I wonder if the approach there would be better.  I
was never really sure if I liked it myself, though (which is one of the reasons
I never finished the patch).
Blocks: 201137
Attached patch Patch against current trunk (obsolete) — — Splinter Review 
This basically works and fixes this bug and bug 145968.  Three things that
sorta bother me about it:

1)  I didn't attempt to mess with the fact that we parse all pseudos in
    ParsePseudoSelector and then pull out the pseudo-elements.	It seems
    cumbersome, but it works....
2)  We have tons of selectors in ua.css where people just randomly made up
    -moz-* pseudos.  I had to keep parsing them all as pseudo-elements as we
    used to; we are still allowing single-':' forms of these for backwards
    compat.
3)  I don't really like hardcoding the list of CSS2 pseudo-elements in two
    different places.

Thoughts?
Attachment #126221 - Flags: superreview?(dbaron)
Attachment #126221 - Flags: review?(glazman)
Daniel, feel free to assign the bug to me if you are OK with the general
approach....
> We have tons of selectors in ua.css where people just randomly made up
> -moz-* pseudos.  I had to keep parsing them all as pseudo-elements as we
> used to; we are still allowing single-':' forms of these for backwards
> compat.

The original reason for the split was that when Peter Linss was writing the
pseudo parsing code, he wanted to be able to very easily decide whether
something was a pseudo-class or a pseudo-element. Now that we can do that, we
should -- if something starts with only one colon, it's a pseudo-class,
otherwise, it's a pseudo-element. (With the four CSS2 exceptions.)

Is there a good reason to continue supporting the old :-moz- pseudo-elements?
Comment on attachment 126221 [details] [diff] [review]
Patch against current trunk

> static PRBool IsTreePseudoElement(nsIAtom* aPseudo)
> {
>   const char* str;
>   aPseudo->GetUTF8String(&str);
>   static const char moz_tree[] = ":-moz-tree-";
>   return nsCRT::strncmp(str, moz_tree, PRInt32(sizeof(moz_tree)-1)) == 0;
> }
> #endif
> 
>+static PRBool IsMozPseudoElement(nsIAtom* aPseudo)
>+{
>+  const char* str;
>+  aPseudo->GetUTF8String(&str);
>+  static const char moz_pseudo[] = ":-moz-";
>+  return nsCRT::strncmp(str, moz_pseudo, PRInt32(sizeof(moz_pseudo)-1)) == 0;
>+}

Call me a maniac, but you could probably factorize those two statics into
one only DoesPseudoNameStartWith() ?

> void CSSParserImpl::ParsePseudoSelector(PRInt32&  aDataMask,
>                                       nsCSSSelector& aSelector,
>                                       PRInt32& aParsingStatus,
>                                       PRInt32& aErrorCode,
>                                       PRBool aIsNegated)
> {
>-  nsAutoString buffer;
>   if (! GetToken(aErrorCode, PR_FALSE)) { // premature eof
>     REPORT_UNEXPECTED_EOF(NS_LITERAL_STRING("name of pseudo-selector"));
>     aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>     return;
>   }

In error reporting, could you please write "pseudo-class or pseudo-element" 
instead of "pseudo-selector" ? The latter does not exist in Selectors
terminology. Thanks.

>+  PRBool isTree = eCSSToken_Function == mToken.mType &&
>+                  IsTreePseudoElement(pseudo);

Could you just add parenthesis around the  a == b ? Only for readability.
Thanks.

> #endif
>-          // the negation pseudo-class is a function
>-          (nsCSSPseudoClasses::notPseudo == pseudo) ||
>-          // as is the lang pseudo-class
>-          (nsCSSPseudoClasses::lang == pseudo))) {
>-      REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Expected identifier for pseudo-class selector not found"));
>-      UngetToken();
>-      aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>-      return;
>-    }
>+  PRBool isMozPseudo = IsMozPseudoElement(pseudo);
>+  
>+  // If it's a function token, it better be on our "ok" list
>+  if (eCSSToken_Function == mToken.mType &&
>+#ifdef MOZ_XUL
>+      !isTree &&
>+#endif
>+      nsCSSPseudoClasses::notPseudo != pseudo &&
>+      nsCSSPseudoClasses::lang != pseudo) { // There are no other function pseudos
>+    REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Expected identifier for function pseudo-selector but found"));
>+    UngetToken();
>+    aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>+    return;
>+  }

Am I right if I say this is going to reject the following rule copied from
CaScadeS ?

   treechildren:-moz-tree-cell-text(external) {
       font-style: italic ! important;
   }

>+  // If it starts with "::", it better be a pseudo-element
>+  if (parsingPseudoElement &&
>+      !isMozPseudo &&
>+      !nsCSSPseudoElements::IsPseudoElement(pseudo)) {
>+    REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Expected pseudo-element but found"));
>+    UngetToken();
>+    aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>+    return;
>   }

I recommend an extra comment line saying that proprietary -moz-*
pseudo-elements
never start with a double colon.

>   if (nsCSSPseudoClasses::notPseudo == pseudo) {
>     if (aIsNegated) { // :not() can't be itself negated
>       REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Negation pseudo-class can't be negated"));
>+      UngetToken();
>       aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>       return;
>     }
>     // CSS 3 Negation pseudo-class takes one simple selector as argument
>     ParseNegatedSimpleSelector(aDataMask, aSelector, aParsingStatus, aErrorCode);
>     if (SELECTOR_PARSING_ENDED_OK != aParsingStatus) {
>       return;
>     }
>@@ -2180,29 +2216,47 @@ void CSSParserImpl::ParsePseudoSelector(
>     // XXX are there more pseudo classes which accept arguments ?
>     else {
>       aSelector.AddPseudoClass(pseudo);
>     }
>     if (SELECTOR_PARSING_ENDED_OK != aParsingStatus) {
>       return;
>     }
>   }
>-  else {
>+  else if (nsCSSPseudoElements::IsPseudoElement(pseudo) ||

You are already checking nsCSSPseudoElements::IsPseudoElement(pseudo) above
and could probably store its value in a boolean instead of calling twice
such a lookup method.

>+           isMozPseudo) {
>+    // Pseudo-element.  Make some more sanity checks.
>+    
>     if (aIsNegated) { // pseudo-elements can't be negated
>       REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Pseudo-elements can't be negated"));
>+      UngetToken();
>       aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>       return;
>     }
>+    // CSS2 pseudo-elements are allowed to have a single ':' on them, as are
>+    // various -moz-* pseudo-elements.  Others (CSS3+ pseudo-elements) must
>+    // have |parsingPseudoElement| set.
>+    if (!parsingPseudoElement &&
>+        !isMozPseudo &&
>+        nsCSSPseudoElements::after != pseudo &&
>+        nsCSSPseudoElements::before != pseudo &&
>+        nsCSSPseudoElements::firstLetter != pseudo &&
>+        nsCSSPseudoElements::firstLine != pseudo) {
>+      REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("This pseudo-element must use the \"::\" form: "));
>+      UngetToken();
>+      aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>+      return;      
>+    }
>+
>     if (0 == (aDataMask & SEL_MASK_PELEM)) {
>       aDataMask |= SEL_MASK_PELEM;
>       aSelector.AddPseudoClass(pseudo); // store it here, it gets pulled later
> 
> #ifdef MOZ_XUL
>-      if (eCSSToken_Function == mToken.mType && 
>-          IsTreePseudoElement(mToken.mIdent)) {
>+      if (isTree) {
>         // We have encountered a pseudoelement of the form
>         // -moz-tree-xxxx(a,b,c).  We parse (a,b,c) and add each
>         // item in the list to the pseudoclass list.  They will be pulled
>         // from the list later along with the pseudoelement.
>         if (!ParseTreePseudoElement(aErrorCode, aSelector))
>           aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>           return;
>       }

I wonder if you are not failing to check that the negation of a XUL
pseudo-element
is invalid. Again, is -moz-tree the only XUL pseudo-element ? What above 
-moz-tree-cell-text() I quoted above ? What about all the pseudos in
nsCSSAnonBoxList.h ?

>@@ -587,16 +588,23 @@ void nsCSSSelector::ToStringInternal(nsA
>       aString.Append(PRUnichar('*'));
>     }
>     if (1 < aNegatedIndex) {
>       NS_IF_NEGATED_END(aIsNegated, aString)
>     }
>   } else {
>     // Append the tag name, if there is one
>     if (mTag) {
>+      if (aIsPseudoElem &&
>+          mTag != nsCSSPseudoElements::after &&
>+          mTag != nsCSSPseudoElements::before &&
>+          mTag != nsCSSPseudoElements::firstLine &&
>+          mTag != nsCSSPseudoElements::firstLetter) {
>+        aString.Append(PRUnichar(':'));
>+      }
>       nsAutoString prefix;
>       mTag->ToString(prefix);
>       aString.Append(prefix);
>       NS_IF_NEGATED_END(aIsNegated, aString)
>     }
>     // Append the id, if there is one
>     if (mIDList) {
>       nsAtomList* list = mIDList;

Yeah. Unfortunately, there is no good way to determine if we should output
the double-colon notation or not. I guess you have no other choice than
the output of ":" for those pseudos to preserve backwards compatibility :-/

>-input[type=image]:focused {
>+input[type=image]:focus {

This is unrelated to the current bug, right ?

Sorry, but the question about the XUL pseudos and their negations is
important enough to ask you to clarify this issue. In the meantime, I
don't r+ or r-, and wait for your answer.
Comment on attachment 126221 [details] [diff] [review]
Patch against current trunk

> In error reporting, could you please write "pseudo-class or pseudo-element" 

Sure.

> Could you just add parenthesis around the  a == b ?

Done.

> Am I right if I say this is going to reject the following rule copied from
CaScadeS ?

No.  This rul falls under the isTree boolean (that's a prefix match on
"-moz-tree-", recall).

> I recommend an extra comment line saying that proprietary -moz-* pseudo-elements never start with a double colon.

Well, they could.  My patch, as-is, supports them both with a single colon and
a double one, just like CSS2 pseudo-elements.

> You are already checking nsCSSPseudoElements::IsPseudoElement(pseudo) above
> and could probably store its value

Indeed.

> What about all the pseudos in nsCSSAnonBoxList.h ?

Aha!  That's what I was looking for!  Patch using that coming up.

> I wonder if you are not failing to check that the negation of a XUL
> pseudo-element is invalid.

I check that at the top of the block.  Anything starting with ':' that's not a
known pseudo-class (as tested by nsCSSPseudoClasses::IsPseudoClass) cannot be
negated.

> This is unrelated to the current bug, right ?

It's totally related.  The rule is supposed to say :focus.  The only reason it
was not caught before is that we did not reject unknown
pseudo-class/pseudo-element selectors as we should have, so that there was no
css error message.  With this patch, there is.
Attachment #126221 - Attachment is obsolete: true
Attachment #126221 - Flags: superreview?(dbaron)
Attachment #126221 - Flags: review?(glazman)
Attached patch Better patch (obsolete) — — Splinter Review 
This more or less addresses my concerns #2 and #3 as well as Daniel's comments.
 The only qualm I have is the -moz-* things I marked as "CSS2 Selectors" in
nsCSSPseudoElementList.h.  Perhaps those should migrate to nsCSSAnonBoxList.h
(except -moz-selection)?
Comment on attachment 126307 [details] [diff] [review]
Better patch

There is one more outstanding issue, which is line 540 of xul.css: the
:-moz-deck-hidden selector that uses is not implemented anywhere and gives a
parsing error with this patch; I am tempted to just remove that rule.
Attachment #126307 - Flags: superreview?(dbaron)
Attachment #126307 - Flags: review?(glazman)
Comment on attachment 126307 [details] [diff] [review]
Better patch

>Index: content/shared/public/nsCSSPseudoElementList.h

The comments refer to CSS3_PSEUDO_ELEMENT, which doesn't exist.  They also need
to explain the difference between CSS_PSEUDO_ELEMENT and CSS2_PSEUDO_ELEMENT
(i.e., that the latter allows single-colon).



>+    // CSS2 pseudo-elements are allowed to have a single ':' on them, as are
>+    // various -moz-* pseudo-elements (anonymous boxes).  Others (CSS3+
>+    // pseudo-elements) must have |parsingPseudoElement| set.
>+    if (!parsingPseudoElement &&
>+        !isAnonBox &&
>+        !nsCSSPseudoElements::IsCSS2PseudoElement(pseudo)) {

The |isAnonBox| check should be temporary, and I think you should have an XXX
comment that we'll fix that once we convert the users.	Likewise for
nsCSSPseudoElementList, where we should convert our internal callers and then
change CSS2_PSEUDO_ELEMENT to CSS_PSEUDO_ELEMENT.

You should probably include the moz-deck-hidden change in the patch.

(I haven't read most of the patch yet, just the edges.)
> The comments refer to CSS3_PSEUDO_ELEMENT, which doesn't exist.

That's because we don't implement any CSS3 pseudo-elements that are not in CSS2
yet.... I suppose we could just call all speudo-elements that are not CSS2
pseudo-elements CSS_PSEUDO_ELEMENT, but then we have to change things again when
CSS4 (in the admittedly remote future) introduces something that only applies to
CSS4 pseudo-elements....  Either way is fine with me; let me know which you prefer.

Let me know whether you want an updated patch or whether it's OK to wait till
after you've read the rest and had a chance to comment on the whole thing together.
Uh, yeah, I _really_ wouldn't worry too much about CSS4 at this point. :-)
Taking.  I'd really like to get this in for 1.5a...

One thing I've changed is that I have:

  if (IsPseudoElement(mTag) && !nsCSSPseudoElements::IsCSS2PseudoElement(mTag)) {
    aString.Append(PRUnichar(':'));
  }

instead of:

  if (aIsPseudoElement && !nsCSSPseudoElements::IsCSS2PseudoElement(mTag)) {
    aString.Append(PRUnichar(':'));
  }

because when the selector is "a:before", aIsPseudoElement is true when mTag is
"a".... (so it would serialize as ":a:before").
Assignee: glazman → bzbarsky
Status: ASSIGNED → NEW
Summary: [CSS3] Need to support '::' for pseudo-elements in the CSS parser → [FIX][CSS3] Need to support '::' for pseudo-elements in the CSS parser
Target Milestone: Future → mozilla1.5alpha
I think I prefer just CSS_PSEUDO_ELEMENT and CSS2_PSEUDO_ELEMENT.
Comment on attachment 126307 [details] [diff] [review]
Better patch

> #ifdef MOZ_XUL
>-      if (eCSSToken_Function == mToken.mType && 
>-          IsTreePseudoElement(mToken.mIdent)) {
>+      if (isTree) {
>         // We have encountered a pseudoelement of the form
>         // -moz-tree-xxxx(a,b,c).  We parse (a,b,c) and add each
>         // item in the list to the pseudoclass list.  They will be pulled
>         // from the list later along with the pseudoelement.
>         if (!ParseTreePseudoElement(aErrorCode, aSelector))
>           aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
>           return;
>       }

The indentation lies.  What's up?

Fix that (somehow), fix my previous comments, file a bug on transitioning of
our internal use (for both anon-boxes and the things marked CSS2_PSEUDO_ELEMENT
that shouldn't be), and r+sr=dbaron.
Attachment #126307 - Flags: superreview?(dbaron)
Attachment #126307 - Flags: superreview+
Attachment #126307 - Flags: review?(glazman)
Attachment #126307 - Flags: review+
The return should be inside the if statement there, so that we will do an
end-of-selector check even after a tree pseudo-element.
Attachment #126307 - Attachment is obsolete: true
Bug 211657 on converting stylesheets to the '::' form; I'll also post to the
relevant newsgroups (.style and .xpfe).

Patch is checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
What about netscape.public.dev.skins?
Didn't know that existed.  Message posted there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: