CSS parser produces multiple rules for comma-separated selectors

RESOLVED FIXED in mozilla1.5alpha



17 years ago
5 years ago


(Reporter: bzbarsky, Assigned: dbaron)


(Blocks: 1 bug, {helpwanted})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [patch])


(1 attachment, 5 obsolete attachments)



17 years ago
Given a rule like:

foo, bar { decls }

The CSS parser produces two nsICSSRules (which share the decls object).  These
both go in the stylesheet and are both visible through the CSSOM.  Doing
GetCssText() on them returns:

"foo { decls }" and "bar { decls } "

This should not affect round-trippability too much, but seems like behavior that
should be fixed at some point....


17 years ago
Blocks: 77705
We need them to be treated as two separate rules for the cascade, so I'm not
sure how to fix this.

Comment 2

17 years ago
Right.  I'm just filing this so that it's filed and people are aware of the
issue.  Please feel free to future it (I would, at the moment).  I'm fairly
certain that there is no simple solution that does not involve changes to the
way we do cascading and the like....
->pierre.  Probably should be Future.
Assignee: dbaron → pierre

Comment 4

17 years ago
I'm becoming reluctant to keep bugs as Future is there is only a very very remote 
chance for them to get fixed sometime within the next 5 years.  I also regret 
that LATER and REMIND have been deprecated.  So... it's going to be WONTFIX.
Last Resolved: 17 years ago
Resolution: --- → WONTFIX
Resolution: WONTFIX → ---
...and taking. It's a valid issue and should not be forgotten, and SHOULD be
fixed. The WONTFIX resolution indicates that the bug *should not* be fixed.
Assignee: pierre → ian
Keywords: helpwanted
Priority: -- → P5
QA Contact: ian → bzbarsky
Target Milestone: --- → Future
->default owner
Assignee: ian → dbaron
QA Contact: bzbarsky → ian
Created attachment 125167 [details] [diff] [review]
work in progress

This almost compiles.  However, I need to rewrite a good bit of the cascade
stuff at the end of nsCSSStyleSheet.cpp to do the last bit.
Comment on attachment 125167 [details] [diff] [review]
work in progress

>Index: content/html/style/src/nsCSSStyleRule.cpp
>-  virtual nsCSSSelector* FirstSelector(void);
>+  virtual nsCSSSelectorList* Selector(void);
>   virtual void AddSelector(const nsCSSSelector& aSelector);
>   virtual void DeleteSelector(nsCSSSelector* aSelector);
>-  virtual void SetSourceSelectorText(const nsString& aSelectorText);
>-  virtual void GetSourceSelectorText(nsString& aSelectorText) const;

The middle two lines should have been removed as well.
Created attachment 125168 [details] [diff] [review]
work in progress

Attaching corrected patch (one other thing, too), since I may not come back to
this for a while.
Attachment #125167 - Attachment is obsolete: true
Created attachment 125170 [details] [diff] [review]

I realized the easy way to do what I wanted (create |RuleValue| objects
earlier), so here's a complete patch.

I still need to check that what I did to the MathML code (eek, odd dependency!)
will work.

I also intentionally broke a small piece of inspector (I made
|inDOMUtils::GetRuleWeight| always say that the weight was zero.  Rules don't
have a unique weight anymore.  I also may have unintentionally broken other
things in inspector.  I need to look more closely, and probably file a bug on
the inspector module owner about changing the UI in response to these changes.
Attachment #125168 - Attachment is obsolete: true
Created attachment 125171 [details] [diff] [review]

I think the MathML change is right (although I cc:ed rbs just in case).

I decided to just remove the "Weight" column from inspector -- I suspect it's
not all that useful anyway (and the rules are sorted already).	(I think having
the full selector there, which this causes, is more useful than "Weight",
anyway.)  Someone who knows inspector (caillon?) should probably double-check
that I removed the right things, and also that this is what you want to do.
Attachment #125170 - Attachment is obsolete: true
Priority: P3 → P2
Whiteboard: [patch]
Target Milestone: Future → mozilla1.5alpha
A few further notes on the patch:
 * I think the original design was based on nsIStyleRule having methods like
|GetWeight|, that aren't needed and have been removed.  I don't see any need for
separate |nsIStyleRule| objects to match the different parts of the selector --
and there shouldn't be any problem with an element matching a single rule
multiple times.
 * I removed |mWeightedRules| from |RuleCascadeData|, which was unused after the
construction of the object, except that it owned references to the rules.  Since
the sheets own references to the rules (as do the rule nodes), I don't think
these references are needed.

I think it's otherwise pretty clear -- I just changed CSSStyleRuleImpl objects
so they own a nsCSSSelectorList (renamed from the SelectorList struct inside
nsCSSParser.cpp -- which already had the right destructor, so I don't need to
worry about ownership very much).  This means that there's a loop removed from
CSSParserImpl::ParseRuleSet and a loop added to InsertRuleByWeight (in
nsCSSStyleSheet.cpp) and a lot of changes to function signatures and member
variables.  In order to hold the rule / selector pair across the three parts of
CSSRuleProcessor::GetRuleCascade, I started creating the RuleValue objects
(half-initialized) in the first part (CascadeSheetRulesInto) rather than the
third (AddRule).
Comment on attachment 125171 [details] [diff] [review]

r=caillon on the inspector portion of this patch, if you also remove the
olcWeight stuff in the themes:

ic/viewers/styleRules/styleRules.css#52 and

Comment 15

16 years ago
I'll review this patch this coming weekend (I have exams until then).

Comment 16

16 years ago
Comment on attachment 125171 [details] [diff] [review]



+    nsCSSSelector **sel_cur = &l->mSelectors;

That should be

nsCSSSelector **sel_cur =  &lcopy->mSelectors;

Comment 18

16 years ago
Comment on attachment 125617 [details] [diff] [review]

>Index: content/html/style/src/nsCSSParser.cpp

>@@ -688,25 +632,25 @@ CSSParserImpl::ParseStyleAttribute(const
>   if (nsnull != declaration) {
>     // Create a style rule for the delcaration
>     nsICSSStyleRule* rule = nsnull;
>-    NS_NewCSSStyleRule(&rule, nsCSSSelector());
>+    NS_NewCSSStyleRule(&rule, nsnull);
>     rule->SetDeclaration(declaration);
>     *aResult = rule;
>   }

As long as you're here, could we change the test to 
|if (declration)|, remove the "rule" temporary (just do
NS_NewCSSStyleRule(aResult, nsnull)), and maybe do something sane on OOM?

>@@ -1516,75 +1460,59 @@ PRBool CSSParserImpl::ParseRuleSet(PRInt
>+  nsICSSStyleRule* rule = nsnull;
>+  NS_NewCSSStyleRule(&rule, slist);
>+  if (nsnull != rule) {
>+    rule->SetLineNumber(linenum);
>+    rule->SetDeclaration(declaration);
>+    (*aAppendFunc)(rule, aData);
>+    NS_RELEASE(rule);
>   }

I know you just copied this code, but that may make more sense as an nsCOMPtr
(and the check could be |if (rule)|).

What if "rule" ends up null here (OOM)?  It looks like we will leak slist in
that case...

>Index: content/html/style/src/nsCSSStyleRule.cpp
>-    mNext->ToString(aString, aSheet, IsPseudoElement(mTag), PR_FALSE);
>+    mNext->ToStringInternal(aString, aSheet, IsPseudoElement(mTag), PR_FALSE);

I know you just renamed this, but please pass '0' instead of 'PR_FALSE' (that
arg is a PRInt8, not a PRBool).

As a separate matter, I see no reason to use a PRInt8 for that state var; a
PRInt32 or PRIntn may be better...

A bunch of the nsCSSSelectorList code could do null-checks of pointers without
comparing to nsnull... (I know you just copied this code; either way is fine
with me).

>+  nsCSSSelectorList *list;
>+  nsCSSSelectorList **list_cur = &list;
>+  for (nsCSSSelectorList *l = this; l; l = l->mNext) {
>+    nsCSSSelectorList *lcopy = new nsCSSSelectorList();
>+    if (!lcopy) {
>+      delete list;

I think you want to init |list| to nsnull before entering the loop -- otherwise
you could end up deleting a garbage pointer here.

Also, you want to assign into *list_cur before entering the selector-copying
inner loop.  That way if a selector allocation fails, you won't leak lcopy and
whatever selectors you've allocated up to now.	Alternately, you need to delete
both |list| and |lcopy| in that inner error case.

All the mWeight stuff can be removed, right?

>Index: content/html/style/src/nsCSSStyleSheet.cpp
>+      mRuleArrays(nsnull, nsnull, RuleArraysDestroy, nsnull, 64),

Hmmm... I guess this is OK as long as we don't try to Clone() mRuleArrays.  I'd
prefer to pass in a non-null function pointer for that first arg and just make
it assert (NS_NOTREACHED) or something...

>+  nsObjectHashtable mRuleArrays; // of nsISupportsArray

You mean of nsAutoVoidArray, right?

>Index: content/html/style/src/nsICSSStyleRule.h
>+ * A selector group is the unit of selectors that each style rule has.

Why bother mentioning the term "group"?  The only place that refers to
nsCSSSelectorList as a "selector group" is the ParseSelectorGroup code in
nsCSSParser.... I'd just call it a "selector list" here.

>+  /**
>+   * Push a copy of |aSelector| on to the beginning of |mSelectors|,
>+   * setting its |mNext| to the current value of |mSelectors|.
>+   */
>+  void AddSelector(const nsCSSSelector& aSelector);

Make it clear that this does NOT update mWeight?

r+sr=me with those changes.
Attachment #125617 - Flags: superreview?(bz-bugspam)
Attachment #125617 - Flags: superreview+
Attachment #125617 - Flags: review?(bz-bugspam)
Attachment #125617 - Flags: review+
Created attachment 125663 [details] [diff] [review]

Patch with bz's comments addressed, except for:

> As long as you're here, could we change the test to 
> |if (declration)|, remove the "rule" temporary (just do
> NS_NewCSSStyleRule(aResult, nsnull)), and maybe do something sane on OOM?

I didn't remove the temporary.	It's even more useful when dealing with OOM.

> Hmmm... I guess this is OK as long as we don't try to Clone() mRuleArrays. 
> prefer to pass in a non-null function pointer for that first arg and just
> it assert (NS_NOTREACHED) or something...

I left this.  It's a very common pattern.
Attachment #125617 - Attachment is obsolete: true

Comment 20

16 years ago
Comment on attachment 125663 [details] [diff] [review]

OK, looks good.
Attachment #125663 - Flags: superreview+
Attachment #125663 - Flags: review+
Fix checked in to trunk, 2003-06-14 16:50 -0700.
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED


16 years ago
Blocks: 183038
No longer blocks: 522921
You need to log in before you can comment on or make changes to this bug.