98765 - CSS parser produces multiple rules for comma-separated selectors

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

->pierre.  Probably should be Future.

Comment 4

[Pierre Saslawsky]

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.

Comment 5

[Hixie (not reading bugmail)]

Reopening...

Comment 6

[Hixie (not reading bugmail)]

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

Comment 7

[Hixie (not reading bugmail)]

->default owner

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: work in progress

Attaching corrected patch (one other thing, too), since I may not come back to
this for a while.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 125171 [details] [diff] [review]
patch

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

http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/skin/class
ic/viewers/styleRules/styleRules.css#52 and
http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/skin/moder
n/viewers/styleRules/styleRules.css#52

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[jag (Peter Annema)]

Comment on attachment 125171 [details] [diff] [review]
patch

+nsCSSSelectorList::Clone()

...

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


That should be

nsCSSSelector **sel_cur =  &lcopy->mSelectors;

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Fixes caillon's comment and jag's comment.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125617 [details] [diff] [review]
patch

>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*
>+nsCSSSelectorList::Clone()
>+{
>+  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.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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. 
I'd
> prefer to pass in a non-null function pointer for that first arg and just
make
> it assert (NS_NOTREACHED) or something...

I left this.  It's a very common pattern.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125663 [details] [diff] [review]
patch

OK, looks good.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-06-14 16:50 -0700.