If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Comments should be allowed between simple selectors

VERIFIED FIXED in mozilla1.3beta

Status

()

Core
CSS Parsing and Computation
P1
minor
VERIFIED FIXED
17 years ago
15 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: bz)

Tracking

(Blocks: 1 bug, {css1, perf, testcase})

Trunk
mozilla1.3beta
css1, perf, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSS1-B], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

17 years ago
The following selectors are ignored and should not be as far as I can tell:
   p/**/.test
   p/**/#test
   p/**/[test]
   p/**/:first-child

As I understand it, CSS comments may appear anywhere between tokens, including
in between parts of selectors.
(Reporter)

Comment 1

17 years ago
Created attachment 19306 [details]
Simple and self explanatory testcase showing the 4 cases.
(Reporter)

Updated

17 years ago
Keywords: correctness, css1, mozilla1.0, testcase
QA Contact: chrisd → ian
Note that to get this perfect, we might need to change the tokenizer to match
the list of tokens in the CSS2 spec, at least for all multi-character tokens.

Comment 3

17 years ago
I happened upon this bug after trying a suggested workaround that 
richinstyle.com suggested about hiding unsupported or buggy features of css 
from IE.  One clear case for fixing this bug is the use of :focus.  The problem 
is that IE sees it and immediately applies whatever rules you set for it to the 
elements rather than applying them only when they have focus.  For instance:

:link:focus { background: black; color: white; }

as soon as IE4/5 (actually tested on 5.5) sees this, all links will have a 
background of black and a forground of white overriding anything else set 
simply because instead of ignoring :focus (which it doesn't support), it just 
applies it arbitrarily (please correct me if I'm mistaken here).

I tried getting around this by doing:

:link/*hide from IE*/:focus { background: black; color: white; }

However, now Mozilla/NS6 can't see the focus rule either.

Without being able to use this workaround it seems impossible to have a single 
stylesheet, which includes :focus (or other unsafe/advanced css), work across 
browsers.  If this bug is fixed, it allows us to have a safe stylesheet for all 
browsers while, at the same time, being able to use the more advanced features 
of css with Mozilla.  Otherwise, we have to ignore the advanced features or do 
browser detection and apply one of multiple different style sheets.

I vote for this bug to get some priority!

Jake

Comment 4

17 years ago
Daniel mentioned this bug a while ago and since he's now familiar with the code, 
it makes him the perfect guy to fix it.  Reassigned to him.
Assignee: pierre → glazman
Target Milestone: --- → mozilla1.0
Accepting bug assignment
Status: NEW → ASSIGNED
Please note that the fourth test in attached test case is actually incorrect.
The :before part should be in white letters over green background but the
only rule applying to the contents of the paragraph itself is

  p { background : yellow ; color : red }

Proposing new correct test...
(Reporter)

Comment 7

17 years ago
oops. Yes, that paragraph should just have "‌" as content (or any other
zero-width character).
In fact, I prefer, for the purpose of this bug, to give some content to the last
P. It helps showing that the P does not get an unwanted style...
But I understand it does not meet your QA criteria ;-)
Created attachment 32137 [details]
new test case ; WARNING : fix for 77519 has to be in for the correctness of this test
Created attachment 32138 [details] [diff] [review]
proposed patch for 60290
Proposing fix for this bug.

Originally eCSSToken_WhiteSpace represented both whitespace _and_ CSS comments.

So the first thing to do is to separate them in nsCSSScanner.h.
Testing only eCSSToken_WhiteSpace in the for(;;) loop of SelectorMatches() is
not enough because a sequence of simple selectors can end on a whitespace...
eCSSToken_CSSComment is created.

The second thing to do is the modification of nsCSSScanner::ParseCComment so
it returns a eCSSToken_CSSComment instead of a eCSSToken_WhiteSpace.

The third thing to do is the modification of eCSSToken_WhiteSpace handling in
nsCSSParser.cpp : it should also deal with eCSSToken_CSSComment.

The last thing is the modification of SelectorMatches() itself : it should now
accept comments between simple selectors ; of course, it does nothing when it
encounters a comment.

Pierre and Marc, can you r= and sr= please ?
Hmmm... the CSS spec (IIRC) says that comments should be allowed anywhere
between tokens.  Wouldn't an easier way to achieve that (and a way that would
fix the problem more generally) be to (instead of having a token type for
comment) have the scanner, whenever it hits a comment token, skip it and return
the next token?
I hope that in the future we will be able to spare comments in the CSS Object
Model. Editing tools need such a feature if they want to be css-aware and allow
direct manipulation of css styles. So I don't think we should follow your
proposal.
Moving to Moz1.1. Engineers are overloaded with higher priority bugs.
Target Milestone: mozilla1.0 → mozilla1.1
Whiteboard: [CSS1-B]
Attachment #32137 - Attachment is patch: false
Attachment #32137 - Attachment mime type: text/plain → text/html
Keywords: patch

Updated

16 years ago
Blocks: 153699
color /*comment*/ : /*comment*/ rgb( /*comment*/ 100 /*comment*/ , /*comment*/
100 /*comment*/, /*comment*/ 101 /*comment*/)/*comment*/;

There is no real way we can reflect this in the CSSOM as it stands (nor in any
sane OM I can think of for CSS, for that matter, short of just storing the
text).  I think we should fix the problem in the way that provides the most
general solution for possible tokenization bugs with a minimum of effort.  If
and when the storage mechanism for CSS can usefully store comments (and there is
an API for manipulating said comments), we can revisit the parsing issues, armed
with a large suite of parser testcases.

In other words, I agree with comment 12, until such a time as we have somewhere
we can put the comment tokens.  Then we can introduce comment tokens.

_This_ bug, however, should be fixed, preferably with minimal risk and maximal
reward....

Comment 16

15 years ago
Reconfirmed using FizzillaCFM/2002070913. Setting All/All.
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 17

15 years ago
I agree with dbaron (comment 12) and bz (comment 15).
Created attachment 111119 [details] [diff] [review]
an alternate proposal...

This just skips comments.  Once we have someplace to put them, applying
glazou's patch from this bug is trivial (and I added comments that that should
be done).  At that point we will want to introduce "edit" and "normal" mode to
the scanner and parser.
Attachment #111119 - Flags: superreview?(dbaron)
Attachment #111119 - Flags: review?(glazman)
Comment on attachment 111119 [details] [diff] [review]
an alternate proposal...

Just change the NS_NOTREACHED("something went deeply wrong") to a more explicit
message please... It could happen that a sheet starts a comment and forgets to
close it.
Attachment #111119 - Flags: review?(glazman) → review+
Comment on attachment 111119 [details] [diff] [review]
an alternate proposal...

sr=dbaron if you replace the NS_NOTREACHED with a calls to the error reporting
code to report that there was an unterminated comment.
Attachment #111119 - Flags: superreview?(dbaron) → superreview+
Taking.  Will fix that, definitely; I somehow misread "break" as "return"...
Assignee: glazman → bzbarsky
Status: ASSIGNED → NEW
Keywords: perf
Priority: P3 → P1
Target Milestone: mozilla1.1alpha → mozilla1.3beta
Created attachment 111381 [details] [diff] [review]
updated patch
Attachment #32138 - Attachment is obsolete: true
Attachment #111119 - Attachment is obsolete: true
Note that what I checked in differs from the updated patch in that it defines
REPORT_UNEXPECTED_EOF to empty when css error reporting is off.
fixed.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
tested on winXP with trunk build : 2003-01-27-09-trunk
verified fixed 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.