User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100405 Minefield/3.7a4pre Build Identifier: inDOMUtils' getCSSStyleRules should take an optional pseudo-element argument so that it can fetch the CSS rules that apply to an element and pseudo-element combination. This would let Firebug and DOM Inspector show the pseudo-element rules that apply to an inspected element. Reproducible: Always

This is spun off bug 185431. (In reply to bug 185431, comment #13) > A small patch for the inpsector's domUtils that adds an extra argument to > getCSSStyleRules for the psuedo element, much like getComputedStyle. Firebug > and DOM Inspector could call this function for all the pseudo elements they > want to support. A comment: this doesn't really mesh with the way getCSSStyleRules works otherwise, which would behave more like your other suggestion: > the other option is to return all the pseudo element rules that apply to that > element in the call to getCSSStyleRules. This is more complicated, however, so why not create a separate method that keeps the querying on a per-pseudo basis approach?

actually, I think it makes more sense this way, at least with respect to the way Firebug and DOM Inspector use it. I think the array returned by getCSSStyleRules is ordered in a certain way (according to the cascade and which rules override other rules). A psuedo-element rule shouldn't override a regular rule that applies to an element and vice versa. I think it would be easier for Firebug to interpret this difference if it queried for each pseudo-element separately. That was my impression, I could definitely be wrong though.

updated to Boris's comments Boris, thanks for taking a look, a couple questions: 1) "nsIDOMElement.h" is #included twice in inDOMUtils.cpp, is that intentional or does anyone even care? :-) 2) Does adding these lines leak any memory since 'forget()' might not be called?: >+ if(sContext) { >+ *aRuleNode = sContext->GetRuleNode(); >+ sContext.forget(aStyleContext); >+ }

> A comment: this doesn't really mesh with the way getCSSStyleRules works Why not?

> 1) "nsIDOMElement.h" is #included twice in inDOMUtils.cpp Feel free to get rid of one of those! > 2) Does adding these lines leak any memory since 'forget()' might not be > called?: No. The point of nsRefPtr is to do reference-counting for you in cases like this. That code is correct. The patch looks good to me. Are you willing to add some tests too?

(In reply to comment #4) > > A comment: this doesn't really mesh with the way getCSSStyleRules works > > Why not? Right now, using getCSSStyleRules is saying "Give me the set of style rules you've got for this element". It just feels like the behavior should be like: > return all the pseudo element rules that apply to that element in the call to > getCSSStyleRules Maybe it's just because I can see how they'd be presented in the DOM Inspector, and my mental model doesn't include thinking of pseudo-elements as separate from the actual elements. I guess it makes sense if you think of them as being on the same level. Otherwise, I think it's just being me being nit-picky and hampering progress.

> my mental model doesn't include thinking of pseudo-elements as separate > from the actual elements Ah. That's what needs adjusting, then. ;)

>Maybe it's just because I can see how they'd be presented in the DOM Inspector, Ah, I didn't realize DOM Inspector didn't indicate overriding. It might be useful to expose a list of the pseudo-elements that the platform supports. This list is short right now, unless you include the -moz pseudo-elements. > Are you willing to add some tests too? Yeah, I can write some tests for this, I assume it would go in the 'tests' directory for the inspector, similar format to the other tests in that directory. Forgot to mention, calling getCSSStyleRules(element, "jcoiwjeiwao") will just return null, should it throw an exception? For reference, getCSSStyleRules(notAValidElement) will throw an exception.

same patch as above, with an added test

forgot to take out that extra #include I mentioned previously, this patch takes that out

let me know what I can do to possibly get this in the tree.

Find me more time to sort out the security issues, or find someone else who can do that instead.... This is a pretty low priority for the time being, sadly.

Boris, can you perhaps recommend anyone who could review the code? Firebug's functionality suffers with this functioanlity still not in place...

I can do that. I'm not sure what happened here back in May of last year... I'll look over the patch again just to make sure things are ok.

Comment on attachment 438321 [details] [diff] [review] patch with test and removal of extra #include Hmm. So this doesn't apply on tip. I'll get it merged...

Essentially same patch as before, main difference seems to be: nsComputedDOMStyle::GetStyleContextForElement(aContent->AsElement(), aPseudo, presShell); instead of: nsComputedDOMStyle::GetStyleContextForContent(aContent, aPseudo, presShell);

Sorry, forgot to hg add the test case.

nemo, thanks for updating that! http://hg.mozilla.org/projects/cedar/rev/5f243f6a0eb6

Fixed: http://hg.mozilla.org/mozilla-central/rev/5f243f6a0eb6 Dylan, thank you for the patch, and my apologies that this got lost last May. :(