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

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

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.

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.

Reopening...

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

->default owner

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.

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

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.

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.

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

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

Comment on attachment 125171 [details] [diff] [review] patch +nsCSSSelectorList::Clone() ... + nsCSSSelector **sel_cur = &l->mSelectors; That should be nsCSSSelector **sel_cur = &lcopy->mSelectors;

Fixes caillon's comment and jag's comment.

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.

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 on attachment 125663 [details] [diff] [review] patch OK, looks good.

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