we should be able to highlight and inspect a :pseudo element

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: paul, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 26
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
It could be useful to be able to inspect pseudo elements like :after and :before.

Comment 1

6 years ago
http://code.google.com/p/fbug/issues/detail?id=537 is some kind of related, because Firebug at least already displays pseudo elements.
As an intermediate step, can we just show the all the pseudo-element rules for an element in the rule view?
Assignee: nobody → fayearthur
also things like, :first-child, :first-line, :first-letter, ...
(Reporter)

Comment 4

6 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #3)
> also things like, :first-child

This is a pseudo class, and we already support that, because :first-child reference an existing DOM element.

> :first-line, :first-letter, ...

Yeah, we need to support these too.
(Reporter)

Updated

6 years ago
Duplicate of this bug: 712495
Created attachment 585888 [details]
Mockup of possible UI for Rules view

Here's a possible UI for showing pseudo-element rules in the Rules view. It just adds a section for each pseudo-element, separated by darker headers.
Attachment #585888 - Flags: feedback?(paul)
(Reporter)

Comment 7

6 years ago
(In reply to Heather Arthur [:harth] from comment #6)
> Created attachment 585888 [details]
> Mockup of possible UI for Rules view
> 
> Here's a possible UI for showing pseudo-element rules in the Rules view. It
> just adds a section for each pseudo-element, separated by darker headers.

(sorry, long comment, thinking out loud)

So you're suggesting that, for each element, we have an additional UI inside our Node Centric tools, to show the pseudo-elements.
That would mean extending the UI for the Rule View, for the Property View and for the other coming tools.

Can we imagine, instead, a way to highlight and inspect pseudo elements directly?

If highlighting a pseudo-element is too tricky (and I guess it is), we can imagine just extending the Rule View and the Property View then (other tools might not need to show the pseudo-elements).

To avoid confusion, I would hide (fold) the pseudo-element related UI in a single line at first:

             +------------------------------+
             |                              |
             ||inline:                      |
             |element {                     |
             |}                             |
             |                              |
             ||inline:10                    |
             |#main {                       |
             |...                           |
             |}                             |
             |                              |
             |------------------------------|
             | [+] :before, :first-line     |
             |------------------------------|
             |                              |
             |                              |
             |                              |
             +------------------------------+

Clicking on the [+] would unfold the pseudo-elements rules.

But, that would be too much visually-complex for the Property View (where a folding mechanism is already there, and that would mean scrolling very far to get the pseudo-elements properties).

And I don't think we should have 2 different UIs for Rules and Properties.

So here is another idea: for all of the tools, we could add, at the bottom, a dropdown menu that would let the user select a pseudo-element (if any):


             +------------------------------+
             |                              |
             ||inline:                      |
             |element {                     |
             |}                             |
             |                              |
             ||inline:10                    |
             |#main {                       |
             |...                           |
             |}                             |
             |                              |
             |                              |
             |                              |                                     |
             |                              |                                     |
             |                              |      |                              |
             |                              |      |                              |
             |                              |      +------------------------------+
             |                              |      |                              |
             |                              |      | x  Main element              |
             |                              |      |    :before                   |
             |                              |      |    :after                    |
             |                              |      |                              |
             +---+--------------------------+      |---+--------------------------|
             | ^ |available pseudo-elements | +--> | ^ |available pseudo-elements |
             +---+--------------------------+      +------------------------------+


That could directly be part of the sidebar (not the tools themselves).
The tools would need a way to be invoked for a pseudo-element.

Another option could be to add an arrow in the the breadcrumbs-current-node to let the user select the pseudo element (and, at the same time, let the user check the pseudo *classes*).

I will bring this problem to our UI-meeting next week. For now, I think the breadcrumbs idea is the best one.
I think Heather was proposing adding :pseudo element rules to the Rules view alone. I don't think we'd want to show those in the Property view, for example because they wouldn't make sense there. I think it's a fine suggestion.

Collapsing pseudo elements might be nice to have, though it requires extra clicks to make them visible. I think heather's suggestion is nice and simple.
(Reporter)

Comment 9

6 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> I think Heather was proposing adding :pseudo element rules to the Rules view
> alone. I don't think we'd want to show those in the Property view, for
> example because they wouldn't make sense there.

Why? What about getComputedStyle(element, "after")?

> Collapsing pseudo elements might be nice to have, though it requires extra
> clicks to make them visible.

Why would that be a problem? I prefer that rather than having the style of the pseudo elements popping up as soon as you highlight an element that has pseudo elements in it.

> I think heather's suggestion is nice and simple.

It is nice a simple for the Rule View if we consider that the ability to inspect a pseudo element is strictly restricted to the Rule View. But I don't see why we would do that.

Also, the first time I saw the mockup, it took me a while to understand that I was looking at the style of *several* elements. This is confusing. I think we want, at least, to let the user explicitly ask for the pseudo-element style. Then the user will understand that he will see the pseudo-elements style.
I'm not sure this bug is the best place for this discussion, but here we are.

(In reply to Paul Rouget [:paul] from comment #9)
> (In reply to Rob Campbell [:rc] (robcee) from comment #8)
> > I think Heather was proposing adding :pseudo element rules to the Rules view
> > alone. I don't think we'd want to show those in the Property view, for
> > example because they wouldn't make sense there.
> 
> Why? What about getComputedStyle(element, "after")?

What about it? How do you display those rules in the property view so that it doesn't confuse the developer? I guess you could put a new section under an :after heading with all the applied styles, but that feels funny.

Maybe we should tackle how to deal with the property view in a separate bug rather than try to do everything in here.

 > > Collapsing pseudo elements might be nice to have, though it requires extra
> > clicks to make them visible.
> 
> Why would that be a problem? I prefer that rather than having the style of
> the pseudo elements popping up as soon as you highlight an element that has
> pseudo elements in it.

People don't like clicking. They want to see everything immediately.

> > I think heather's suggestion is nice and simple.
> 
> It is nice a simple for the Rule View if we consider that the ability to
> inspect a pseudo element is strictly restricted to the Rule View. But I
> don't see why we would do that.

ok, I guess there's an argument to have things in other views as well, but we should probably deal with those in separate bugs.

> Also, the first time I saw the mockup, it took me a while to understand that
> I was looking at the style of *several* elements. This is confusing. I think
> we want, at least, to let the user explicitly ask for the pseudo-element
> style. Then the user will understand that he will see the pseudo-elements
> style.

Not really. As "Pseudo Elements" they don't really exist in the DOM per se. They are an artifact of styling applied to the current element.

I do think it'd be nice to be able to highlight pseudo elements in the inspector but have a feeling that'll require a bit of extra trickery. I think this sort of solution in this bug is a good stop-gap measure.
(Reporter)

Comment 11

6 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> I'm not sure this bug is the best place for this discussion, but here we are.
> 
> (In reply to Paul Rouget [:paul] from comment #9)
> > (In reply to Rob Campbell [:rc] (robcee) from comment #8)
> > > I think Heather was proposing adding :pseudo element rules to the Rules view
> > > alone. I don't think we'd want to show those in the Property view, for
> > > example because they wouldn't make sense there.
> > 
> > Why? What about getComputedStyle(element, "after")?
> 
> What about it? How do you display those rules in the property view so that
> it doesn't confuse the developer?

In this mockup, the Rule view is split in two: regular element rules + pseudo-elements rules.
For the Property view, we would do the same things: regular element properties + pseudo-elements properties.

> I guess you could put a new section under
> an :after heading with all the applied styles, but that feels funny.

But why wouldn't that feel funny with the Rule View? It is exactly what this mockup shows.

> Maybe we should tackle how to deal with the property view in a separate bug
> rather than try to do everything in here.

We want the user to be able to debug the style of the pseudo elements. There is no reason to let him use the Rule View but not the Property View, right? We need both.

So then, there are 2 approaches:
1) you show the regular element and the its pseudo-elements in the same (split) view; // current mockup
2) you give a way to the user to select a pseudo-element, so only the pseudo-element style is shown.

1) is easier to implement but comes with different UIs depending on the tool (at least property and rule). 2) comes with a consistent behavior.

>  > > Collapsing pseudo elements might be nice to have, though it requires
> extra
> > > clicks to make them visible.
> > 
> > Why would that be a problem? I prefer that rather than having the style of
> > the pseudo elements popping up as soon as you highlight an element that has
> > pseudo elements in it.
> 
> People don't like clicking. They want to see everything immediately.

Well, you can't show everything, it can get confusing.

> > > I think heather's suggestion is nice and simple.
> > 
> > It is nice a simple for the Rule View if we consider that the ability to
> > inspect a pseudo element is strictly restricted to the Rule View. But I
> > don't see why we would do that.
> 
> ok, I guess there's an argument to have things in other views as well, but
> we should probably deal with those in separate bugs.

I think we need a generic solution.

> > Also, the first time I saw the mockup, it took me a while to understand that
> > I was looking at the style of *several* elements. This is confusing. I think
> > we want, at least, to let the user explicitly ask for the pseudo-element
> > style. Then the user will understand that he will see the pseudo-elements
> > style.
> 
> Not really. As "Pseudo Elements" they don't really exist in the DOM per se.
> They are an artifact of styling applied to the current element.
> 
> I do think it'd be nice to be able to highlight pseudo elements in the
> inspector but have a feeling that'll require a bit of extra trickery.

I am not saying we should highlight the pseudo-element, but that we should treat them
like regular elements in the node-centric tools.

> I think this sort of solution in this bug is a good stop-gap measure.

Do we need stop-gap measure? Can't we get a clean and consistent solution?

How do you feel about being able to (sub)select a pseudo element from, either the sidebar, or the breadcrumbs?
Personally I think that the sidebar approach makes a lot of sense. Selecting pseudo element should be possible in all tools.

Just couldn't resist joining the chat.
(Reporter)

Comment 13

6 years ago
(In reply to Michael Ratcliffe from comment #12)
> Personally I think that the sidebar approach makes a lot of sense. Selecting
> pseudo element should be possible in all tools.

The question is more about if the selection should affect all the tools, or just the current tool.

If you select :after while you're looking at the Rule View, then switch to the Property View, what do you want to see? The regular node properties of the pseudo-element properties?
Personally I think that it would be information overload if you could always see pseudo elements in every tool ... just imaging how long the list could get in the property view.

Selecting pseudo elements is really just doing something that can't easily be done in the highlighter but it would be cool if they could be selected like any other node.

If selecting them in the highlighter is not possible I would say that selecting them should be like selecting any other element and affect all tools.
(Reporter)

Comment 15

6 years ago
(In reply to Michael Ratcliffe from comment #14)
> Personally I think that it would be information overload if you could always
> see pseudo elements in every tool ... just imaging how long the list could
> get in the property view.

Yeah. We should show them only if the user ask for it.

> Selecting pseudo elements is really just doing something that can't easily
> be done in the highlighter but it would be cool if they could be selected
> like any other node.
> 
> If selecting them in the highlighter is not possible I would say that
> selecting them should be like selecting any other element and affect all
> tools.

Exactly.
I see you all's point about the computed style and I just thought of something - what if you have rules like:

#main {
  font-family: Helvetica;
}

#main::first-line {
  font-family: Arial;
}

If you were inspecting the ::first-line pseudo-element you'd want to see the Helvetica crossed out because it'd be overridden. However if you were inspecting the main element it wouldn't be crossed out. To represent this nicely in the rules view we'd want to be able to switch completely between the main element and its pseudo-elements like Paul's second idea.

I say we go all the way, like Rob and Mike were saying, and allow selecting the pseudo-element in the highlighter: outlining the pseudo-element's bounding box, adding the pseudo-element to the infobar selector, and showing the rules and computed style for the pseudo-element in the sidebar.

If you right-click "Inspect Element" within the bounds of a pseudo-element, it'll inspect that pseudo-element. It could also be a sub-selection thing like we're doing with pseudo-classes, and we could allow sub-selection from the infobar and breadcrumb. Technically, pseudo-elements are rendered exactly like children, so we could represent that weirdness in the breadcrumbs and maybe HTML view eventually, though that is sketchy.

Doing it all the way would require some platform work and serious hashing out, all the "extra trickery", but I think anything less would be incorrect, inconsistent, or not apparent. One of those three, and if we had to do something in the meantime, I don't know about sub-selecting a pseudo-element without the highlighter bounding box changing. Fbug and Chrome seem to err on the side of apparentness and visual correlation by just showing the rules straight up, but there are definitely consistency issues there.
You describe the situation perfectly. Highlighting pseudo-elements using the highlighter just like other elements is certainly the best way to go. I would suggest calling the method getNodeForTool() and say that it should return the node at the cursor position (using screen co-ordinates) regardless of whether the node is in an iframe etc. In fact, if it could also be used to inspect chrome elements it would be even nicer.
(In reply to Heather Arthur [:harth] from comment #16)
> I say we go all the way, like Rob and Mike were saying, and allow selecting
> the pseudo-element in the highlighter: outlining the pseudo-element's
> bounding box, adding the pseudo-element to the infobar selector, and showing
> the rules and computed style for the pseudo-element in the sidebar.

Firebug just shows the rules for pseudo-children at the top of the list of rules for the selected element, but as a web dev being able to use the highlighter to pick the pseudo-element would be fantastic.

Comment 19

6 years ago
Triage, filter on pegasus.
Priority: -- → P2
(Reporter)

Comment 20

6 years ago
The "inspector pseudo elt" menu should live in the pseudo-bar. bug 717919.
Depends on: 717919
(Reporter)

Updated

5 years ago
Attachment #585888 - Flags: feedback?(paul)
(Reporter)

Updated

5 years ago
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
(Reporter)

Updated

5 years ago
Duplicate of this bug: 773130
Do we have any plan to fix this problem in recent versions?
This must need to inspect styles.
(Reporter)

Comment 23

5 years ago
Nothing planned yet.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 836520
At this time, we cannot get the style rules of ::(-moz-)selection with using inIDOMUtils.getCSSStyleRules() or other methods until fix bug 836977. But we can get pseudo elements. So we should defer only inspecting ::(-moz-)selection.
Depends on: 836977
Created attachment 708862 [details] [diff] [review]
work in progress

This is work in progress.
I think that we should implement inspecting ::(-moz-)selection after fixed bug 836977.

Paul, should we follow the mockup UI (attachment 585888 [details]) ?
And what else do we need to resolve this bug?
Attachment #708862 - Flags: feedback?(paul)
Created attachment 708960 [details] [diff] [review]
wip2

Support "::(-moz-)selection" pseudo-element.
It's to jump to conclusions that we cannot support "::(-moz-)selection" until fixed bug 836977.
Attachment #708862 - Attachment is obsolete: true
Attachment #708862 - Flags: feedback?(paul)
Attachment #708960 - Flags: feedback?(paul)
Created attachment 709095 [details] [diff] [review]
proposal-v3

Changed from wip-v2:
* Follow the mockup UI.
* Fix some bugs.
Attachment #708960 - Attachment is obsolete: true
Attachment #708960 - Flags: feedback?(paul)
Attachment #709095 - Flags: review?(paul)
No longer depends on: 836977
(Reporter)

Comment 29

4 years ago
Comment on attachment 709095 [details] [diff] [review]
proposal-v3

This approaches is functional. It works and brings some information we didn't have before. Is it possible to show pseudo-element in the computed view as well?

---

That said, could we, instead of showing pseudo elements in the RuleView/ComputedView, let the user select the pseudo-element from the context menu in the infobar (below the pseudo-classes)? Then it would be possible to only inspect a pseudo element (RuleView, ComputedView and Layout would be only be about this pseudo-element)? That would mean updating the Selection.jsm object to handle pseudo-element nodes.

The code might be a little more complex. Selection.jsm would need to expose 2 new functions: `isPseudo()` and `getPseudoName()`. And all the tools using `inspector.selection` would need to handle this special case.

I hope this is clear. Tetsuharu, what do you think?
Attachment #709095 - Flags: review?(paul)
(In reply to Paul Rouget [:paul] from comment #29)
> This approaches is functional. It works and brings some information we
> didn't have before. Is it possible to show pseudo-element in the computed
> view as well?

Thank you, Paul.
This is not possible to show pseudo-element in the computed view, because I thought that the current comuputed view is just only about the selected element. So it's not suitable that to show pseudo-element's styles.


> That said, could we, instead of showing pseudo elements in the
> RuleView/ComputedView, let the user select the pseudo-element from the
> context menu in the infobar (below the pseudo-classes)? Then it would be
> possible to only inspect a pseudo element (RuleView, ComputedView and Layout
> would be only be about this pseudo-element)? That would mean updating the
> Selection.jsm object to handle pseudo-element nodes.
> 
> The code might be a little more complex. Selection.jsm would need to expose
> 2 new functions: `isPseudo()` and `getPseudoName()`. And all the tools using
> `inspector.selection` would need to handle this special case.

Do you mean the inspector highlights the pseudo-element?
If it is so, I think it is good sounds. But it might be complex in some case which is like ::selection.

I don't think it's not good for display the infomation about pseudo-elements. Pseudo-elements is not state. It's different from pseudo-classes. Using same (or very similar) UI is not good in this case. If we'll add similar UI, we should highlight the pseudo-element (But it have above problem. :'-(

I thought we'll implement the mode fot DOM Tree view whch shows virtual pseudo-elements. But also I don't know well it's enable on our inspector design and whether or not its UI mode is really good for debugging.

The better (but not best) approach which I thought at this moment is that we add toggle switch which inspect only specified pseudo-elements to Computed view.

Paul, how do you think about these?
Created attachment 709775 [details]
breadcrumb mockup

(In reply to Paul Rouget [:paul] from comment #29)
> That said, could we, instead of showing pseudo elements in the
> RuleView/ComputedView, let the user select the pseudo-element from the
> context menu in the infobar (below the pseudo-classes)? Then it would be
> possible to only inspect a pseudo element (RuleView, ComputedView and Layout
> would be only be about this pseudo-element)? That would mean updating the
> Selection.jsm object to handle pseudo-element nodes.

(This is supplement of comment #30)
I flash the idea!
How about add showing pseudo-element to breadcrumb, not change the state. (Please see mockup)
I think this can resolve some problem which I commented!
(Reporter)

Comment 32

4 years ago
> But it might be complex in some case which is like ::selection.

We don't have to support them all. I'd focus on before/after (maybe first-line/letter too).

> It's different from pseudo-classes. Using same (or very similar) UI is not good in this case.

It's different, but part of the "logic" (pseudo-something).

Below the pseudo-classes, we could add some text like this:


          +--------------------+
          |  Copy Inner HTML   |
          |  Copy Outer HTML   |
          |  Delete Node       |
          +--------------------+
          |Toggle pseudo Class |
          |--------------------|
          |  :hover            |
          |  :focus            |
          |  :active           |
          +--------------------+
          |Select pseudo elt   |
          |--------------------|
          |  :before           |
          |  :after            |
          |  :first-line       |
          +--------------------+

This would be available from the breadcrumbs and the infobar.

And I like the breadcrumbs mockup.

What do you think?
(Reporter)

Updated

4 years ago
Blocks: 831711
I'm trying to implement the breadcrumbs mockup & the context menu to select pseudo element. I think Paul's suggestion is good. But Its works is proceeding with difficulty.

At the current stage, 

* We don't have a way to highlight the pseudo element (e.g. ::first-line). So we can highlight the element which is root of the pseudo element. Should we file a new bug which relate to bug 839458?

* Implement the breadcrumbs mockup is a little more complex (Paul's guess was correctly.)  I can't say with confidence it, but we may need some abstract representation Selection.jsm which represents DOM Node that we select now.

* I don't find a way to pass the information about that Inspector is selecting the pseudo element now. But this may be caused from my knowledge lacking to know about devtools design. It would be helpful if you answer more information when I get stuck.

And sadly, my main works is busy now...
It may need a times until I finish to make the patch.
(Reporter)

Comment 34

4 years ago
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #33)
> I'm trying to implement the breadcrumbs mockup & the context menu to select
> pseudo element. I think Paul's suggestion is good. But Its works is
> proceeding with difficulty.
> 
> At the current stage, 
> 
> * We don't have a way to highlight the pseudo element (e.g. ::first-line).
> So we can highlight the element which is root of the pseudo element. Should
> we file a new bug which relate to bug 839458?

We could just highlight the parent node for now.
But we'll need to get something like getBoundingClientRect for pseudo elements.

> * Implement the breadcrumbs mockup is a little more complex (Paul's guess
> was correctly.)  I can't say with confidence it, but we may need some
> abstract representation Selection.jsm which represents DOM Node that we
> select now.

Via Selection.jsm, we can define a new kind of "node", but thinking about it, we might prefer this approach: in Selection.jsm, we could add:
> isPseudoElementSelected: true/false
> getPseudoElementName(): ":before"
> setPseudoElementName()

Then anyone using Selection.jsm, can also support pseudoElements, but this would be optional. And that would me the implement simpler.

> * I don't find a way to pass the information about that Inspector is
> selecting the pseudo element now. But this may be caused from my knowledge
> lacking to know about devtools design. It would be helpful if you answer
> more information when I get stuck.
> 
> And sadly, my main works is busy now...
> It may need a times until I finish to make the patch.

I hope that answers most of your questions. If you need any more help, let me know. I'll try to answer sooner this time :)
About the issue on how to show pseudo elements, and/or allow user to select pseudo elements using the rules/computed view sidebar :

we can have a top toolbar for rules view (computed already has one) and add cog wheel buttons on it. This toolbar can then act like place for all the options (which will come in future) like choosing the color type (authored, rgb, hex or hsla), choosing the state of the element (hover, focus etc) and ofcourse, choosing the pseudo element (before, after etc.)

Chrome also does something similar, and I like only this thing in their sidebar :)
(Reporter)

Comment 36

4 years ago
I don't want this to be rule-view specific, but generic (inspector scope). What Chrome does is not optimal in this way.
I was saying in addition to the breadcrumbs context menu. Context menu can remain hidden from the user sometimes.
Assignee: fayearthur → nobody

Updated

4 years ago
Duplicate of this bug: 856576
Duplicate of this bug: 877312
Implementation should probably wait for remote inspector (bug 805526)
(Reporter)

Updated

4 years ago
Depends on: 805526

Updated

4 years ago
Duplicate of this bug: 882571

Updated

4 years ago
Blocks: 893669
(Assignee)

Updated

4 years ago
Assignee: nobody → bgrinstead
(Assignee)

Comment 42

4 years ago
Created attachment 798733 [details] [diff] [review]
694019-1.patch

Tests are still WIP
Attachment #709095 - Attachment is obsolete: true
Attachment #798733 - Flags: review?(paul)
(Reporter)

Comment 43

4 years ago
Comment on attachment 798733 [details] [diff] [review]
694019-1.patch

s/h2/div.
Attachment #798733 - Flags: review?(paul) → review+
(Assignee)

Comment 44

4 years ago
Created attachment 799311 [details] [diff] [review]
694019-2.patch

Move pseudo elements to front inside of collapsible group, and expand tests
Attachment #798733 - Attachment is obsolete: true
Attachment #799311 - Flags: feedback?(pbrosset)
Attachment #799311 - Flags: feedback?(pbrosset) → feedback+
(Assignee)

Comment 45

4 years ago
Created attachment 799353 [details] [diff] [review]
694019-3.patch

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=054cac889be8.  Added tests for expand/collapse functionality.
Attachment #799311 - Attachment is obsolete: true
Attachment #799353 - Flags: review?(paul)
(Reporter)

Comment 46

4 years ago
Comment on attachment 799353 [details] [diff] [review]
694019-3.patch

s/sortRules/sortRulesForPseudoElement/
s/markOverriddenPseudo/markOverridden/
s/layout.css.show_pseudo_elements/devtools.inspector.expand_pseudoelement_rules/

>+    return !!this._showPseudoElements;
>+ !!this.showPseudoElements;
Double bangs are useless here. And in some other places. Only use !! when you're not sure about the type.

> +      try {
> +        this._showPseudoElements = Services.prefs.getBoolPref("layout.css.show_pseudo_elements");
> +      }
> +      catch(e) { }

No need to try catch. Add a pref here:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1077

s/thisElement/selectedElement/ (in the property as well)

> +.ruleview .theme-gutter {

Do you need .ruleview?

> +.ruleview-rule-pseudo-element {

Add a \n before.


Please ask :miker for the next review.
Mike, if you can focus on devtools/server/actors/styles.js...
Attachment #799353 - Flags: review?(paul) → review-
(Assignee)

Comment 47

4 years ago
Created attachment 799388 [details] [diff] [review]
694019-4.patch

Updates from review by :paul.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3e320a08036a
Attachment #799353 - Attachment is obsolete: true
Attachment #799388 - Flags: review?(mratcliffe)
Comment on attachment 799388 [details] [diff] [review]
694019-4.patch

Review of attachment 799388 [details] [diff] [review]:
-----------------------------------------------------------------

Lookin good!
Attachment #799388 - Flags: review?(mratcliffe) → review+
(Assignee)

Comment 49

4 years ago
Can you please check this in?
Flags: needinfo?(mratcliffe)
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/4b65dc0a16cd
Flags: needinfo?(mratcliffe)
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4b65dc0a16cd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26

Updated

4 years ago
Depends on: 914079
(Reporter)

Comment 52

4 years ago
(wrong author in the commit)
You need to log in before you can comment on or make changes to this bug.