add pseudo-element argument to getCSSStyleRules

RESOLVED FIXED in mozilla6

Status

RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: dylanhelling, Assigned: dylanhelling)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla6
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Created attachment 437495 [details] [diff] [review]
patch for inDOMUtils updated to comments

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.  ;)
(Assignee)

Comment 8

9 years ago
>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.
(Assignee)

Comment 9

9 years ago
Created attachment 438320 [details] [diff] [review]
patch with test

same patch as above, with an added test
Attachment #437495 - Attachment is obsolete: true
(Assignee)

Comment 10

9 years ago
Created attachment 438321 [details] [diff] [review]
patch with test and removal of extra #include

forgot to take out that extra #include I mentioned previously, this patch takes that out
Attachment #438320 - Attachment is obsolete: true
Blocks: 185431
(Assignee)

Comment 11

9 years ago
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.

Comment 13

8 years ago
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...

Comment 16

8 years ago
Created attachment 530405 [details] [diff] [review]
Same patch as before, updated due to bitrot

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

Comment 17

8 years ago
Created attachment 530406 [details] [diff] [review]
forgot the test

Sorry, forgot to hg add the test case.
Attachment #530405 - Attachment is obsolete: true
Assignee: nobody → dylanhelling
nemo, thanks for updating that!

http://hg.mozilla.org/projects/cedar/rev/5f243f6a0eb6
Whiteboard: [fixed-in-cedar]
Fixed: http://hg.mozilla.org/mozilla-central/rev/5f243f6a0eb6

Dylan, thank you for the patch, and my apologies that this got lost last May.  :(
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla6
Depends on: 836977
You need to log in before you can comment on or make changes to this bug.