Last Comment Bug 935803 - Display browser styles in the rule view (UA style sheet rules)
: Display browser styles in the rule view (UA style sheet rules)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
P2 normal with 1 vote (vote)
: Firefox 32
Assigned To: (Unavailable until Apr 3) [:bgrins]
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors:
: 940950 947877 1007288 (view as bug list)
Depends on: 1018162
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-06 17:53 PST by J. Ryan Stinnett [:jryans] (use ni?)
Modified: 2014-10-03 04:17 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
user-agent-styles.patch (1.05 KB, patch)
2014-05-23 05:59 PDT, (Unavailable until Apr 3) [:bgrins]
no flags Details | Diff | Splinter Review
user-agent-screenshot.png (146.08 KB, image/png)
2014-05-23 06:00 PDT, (Unavailable until Apr 3) [:bgrins]
no flags Details
useragent-WIP.patch (5.11 KB, patch)
2014-05-23 08:04 PDT, (Unavailable until Apr 3) [:bgrins]
no flags Details | Diff | Splinter Review
useragent-WIP.patch (26.75 KB, patch)
2014-05-23 13:01 PDT, (Unavailable until Apr 3) [:bgrins]
no flags Details | Diff | Splinter Review
cursor-ruleview-links.gif (76.08 KB, image/gif)
2014-05-26 02:32 PDT, Patrick Brosset <:pbro> (PTO, back on Feb 27th)
no flags Details
user-agent-styles.patch (37.47 KB, patch)
2014-05-27 11:25 PDT, (Unavailable until Apr 3) [:bgrins]
pbrosset: review+
Details | Diff | Splinter Review
about-preference-stylesheet.png (101.54 KB, image/png)
2014-05-28 10:44 PDT, (Unavailable until Apr 3) [:bgrins]
no flags Details
user-agent-styles-r=pbrosset.patch (36.28 KB, patch)
2014-05-28 10:45 PDT, (Unavailable until Apr 3) [:bgrins]
bgrinstead: review+
Details | Diff | Splinter Review
user-agent-styles-r=pbrosset.patch (37.65 KB, patch)
2014-05-28 12:50 PDT, (Unavailable until Apr 3) [:bgrins]
bgrinstead: review+
Details | Diff | Splinter Review
user-agent-styles-r=pbrosset.patch (37.76 KB, patch)
2014-05-28 12:53 PDT, (Unavailable until Apr 3) [:bgrins]
bgrinstead: review+
Details | Diff | Splinter Review

Description User image J. Ryan Stinnett [:jryans] (use ni?) 2013-11-06 17:53:02 PST
Currently we don't present any browser styles in the rule view.  This can be useful to understand how your own rules interact with and override those coming from the browser.

While we do have an option to show them in the computed view, this not a very good UX for understanding how your own rules interact with the browser, since you can't see both at the same time.

Perhaps we would add additional blocks to rule view's inheritance chain to display the browser-created entries.  We'd likely want to style these blocks differently, so they stand out as not being from the page's own rules.
Comment 1 User image Paul Rouget [:paul] 2013-11-07 00:48:45 PST
Yep, we should have the same checkbox-searchbox combo at the bottom of the rule view.

That would make ua.css editable. I'm wondering if that would affect other web pages (we might have some sharing magic for ua stylesheets).
Comment 2 User image Paul Rouget [:paul] 2013-11-07 06:17:49 PST
Related: bug 931386
Comment 3 User image (Unavailable until Apr 3) [:bgrins] 2013-11-07 06:37:07 PST
(In reply to Paul Rouget [:paul] from comment #1)
> Yep, we should have the same checkbox-searchbox combo at the bottom of the
> rule view.
> 
> That would make ua.css editable. I'm wondering if that would affect other
> web pages (we might have some sharing magic for ua stylesheets).

In web inspector, these properties are not editable.  You can see them and they get crossed out if they are overridden, but you can't edit the text or disable them.  This seems reasonable, as there is no real value to changing these properties.

I think if we just showed them slightly greyed out at the bottom of the rule view it would be helpful.
Comment 4 User image Jonathan Watt [:jwatt] 2013-12-09 05:59:46 PST
*** Bug 947877 has been marked as a duplicate of this bug. ***
Comment 5 User image Jonathan Watt [:jwatt] 2013-12-09 06:03:03 PST
(In reply to Brian Grinstead [:bgrins] from comment #3)
> In web inspector, these properties are not editable.

I too don't care if these properties are editable, at least not for a first pass. I just really need to be able to _see_ them. Please go for non-editable if that means we get this back sooner.
Comment 6 User image (Unavailable until Apr 3) [:bgrins] 2014-05-09 13:25:20 PDT
I think this will be easy - we just need to add the `filter: "ua"` to the options when calling getApplied from the rule view (because of this check on the page style actor: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#390).

Then we need to just disable all controls on any UA styles
Comment 7 User image Jonathan Watt [:jwatt] 2014-05-23 04:36:07 PDT
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #1)
> Yep, we should have the same checkbox-searchbox combo at the bottom of the
> rule view.
> 
> That would make ua.css editable. I'm wondering if that would affect other
> web pages (we might have some sharing magic for ua stylesheets).

It should not. The sheets are shared, but each document has its own nsStyleSet containing its own mRuleTree generated from the shared sheets. Disabling rules in Inspector disables items in the document's rule tree, so we should be fine.
Comment 8 User image Jonathan Watt [:jwatt] 2014-05-23 04:39:18 PDT
(In reply to Brian Grinstead [:bgrins] from comment #6)
> I think this will be easy - we just need to add the `filter: "ua"` to the
> options when calling getApplied from the rule view (because of this check on
> the page style actor:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> styles.js#390).

It does not seem to be that simple. I tried doing that in a local build and also messing around with a bunch of other stuff like hardcoding |get sourceFilter()| to return "ua" and commenting out |contentSheet| checks, but all to no avail.

> Then we need to just disable all controls on any UA styles

As noted in the previous comment, disabling UA styles will only affect the current document, so I think it would be nice _not_ to disable the controls.
Comment 9 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 05:59:37 PDT
Created attachment 8427703 [details] [diff] [review]
user-agent-styles.patch

I can see user agent styles from html.css with this patch applied.


Note, trying to disable one of them throws this error:

TypeError: parentStyleSheet.ownerNode is null
Stack trace:
StyleRuleActor<.modifyProperties<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:682:9
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:943:13
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1151:9
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:540:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
Comment 10 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 06:00:36 PDT
Created attachment 8427705 [details]
user-agent-screenshot.png
Comment 11 User image Jonathan Watt [:jwatt] 2014-05-23 06:05:36 PDT
Ah, nice, thanks! Paul came up with a hack that worked on IRC, but this looks like it could be part of the actual fix.

Regarding that error, that seems expected. Document sheets will have their owner node be the <style> element that included them, for example. UA sheets have no such owner. I think the devtools code needs fixed up to handle that.
Comment 12 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 06:09:08 PDT
(In reply to Jonathan Watt [:jwatt] from comment #8) 
> > Then we need to just disable all controls on any UA styles
> 
> As noted in the previous comment, disabling UA styles will only affect the
> current document, so I think it would be nice _not_ to disable the controls.

What is the benefit to being able to disable / modify UA styles from the inspector?  It will never make sense to save back the changes you've made on UA styles, and I think it is likely to be confusing that you can change them.
Comment 13 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 06:09:46 PDT
(In reply to Brian Grinstead [:bgrins] from comment #10)
> Created attachment 8427705 [details]
> user-agent-screenshot.png

I would like to have these styles displayed somehow differently to prevent confusing them with page styles.  Instead of saying 'html.css' with a link to that, it should just say 'user agent styles' or similar with no link.  We could also consider having some opacity or alternate styling on them to further distinguish.
Comment 14 User image Jonathan Watt [:jwatt] 2014-05-23 06:15:17 PDT
(In reply to Brian Grinstead [:bgrins] from comment #12)
> What is the benefit to being able to disable / modify UA styles from the
> inspector?  It will never make sense to save back the changes you've made on
> UA styles, and I think it is likely to be confusing that you can change them.

If you have ticked the "Browser styles" checkbox then I think you are clearly opting in to seeing those styles. You can't save changes you make, but it could be interesting to see the effect of them being applied or not for any user.
Comment 15 User image Jonathan Watt [:jwatt] 2014-05-23 06:18:27 PDT
(In reply to Brian Grinstead [:bgrins] from comment #13)
> I would like to have these styles displayed somehow differently to prevent
> confusing them with page styles.  Instead of saying 'html.css' with a link
> to that, it should just say 'user agent styles' or similar with no link.  We
> could also consider having some opacity or alternate styling on them to
> further distinguish.

Ugh. If you tick "Browser styles" in the Computed tab it gives the name, and without the name this would have been pretty useless probably every one of the many times I as a gecko developer have wanted this feature. I think the link is interesting for non-gecko devs and may draw them into working on gecko, and after all if you click on "Browser styles" it isn't unreasonable that this is what you'd get.

I think some styling on them to distinguish from document styles would be a good idea though.
Comment 16 User image Jonathan Watt [:jwatt] 2014-05-23 06:20:03 PDT
If UX concerns mean that editing and linking to the UA sheets is going to be disabled, I think I'd rather this be a pref than a "Browser styles" checkbox so we can leave editing and linking on.
Comment 17 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 06:21:20 PDT
(In reply to Jonathan Watt [:jwatt] from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #12)
> > What is the benefit to being able to disable / modify UA styles from the
> > inspector?  It will never make sense to save back the changes you've made on
> > UA styles, and I think it is likely to be confusing that you can change them.
> 
> If you have ticked the "Browser styles" checkbox then I think you are
> clearly opting in to seeing those styles. You can't save changes you make,
> but it could be interesting to see the effect of them being applied or not
> for any user.

I hadn't considered that checkbox - I would think that these show up in the rule view regardless of this being checked.  I think that box has a different purpose in computed styles in that it can prevent spamming you with every style applied, since all styles are displayed equally.
Comment 18 User image Jonathan Watt [:jwatt] 2014-05-23 06:26:25 PDT
Hmm, I don't think UA rules should show up in the Rules tab unless the user wants them to (either by ticking a similar "Browser styles" checkbox to the one in the Computed tab, or by flipping a pref). I don't think this is just about spamming; I don't think most users would want to see the browser rules by default (rules that they can't change).
Comment 19 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 06:39:05 PDT
(In reply to Jonathan Watt [:jwatt] from comment #16)
> If UX concerns mean that editing and linking to the UA sheets is going to be
> disabled, I think I'd rather this be a pref than a "Browser styles" checkbox
> so we can leave editing and linking on.

> If you have ticked the "Browser styles" checkbox then I think you are
> clearly opting in to seeing those styles. You can't save changes you make,
> but it could be interesting to see the effect of them being applied or not
> for any user.

I also think that linking/editing could be interesting, but I think that brings up some UX questions we need to answer to make sure we don't confuse people.  My vote is to land this as non-editable/linkable and handle that in a follow up.

For reference, here are a couple of the UX questions I think it brings up:

1) If we link to them look they would look exactly like document styles.  This could be easy - there are other ways to handle this with the UI (a greyed out background, for instance).
2) The link will not open up in the style editor right now - it will only open a plain text popup window with the line highlighted.  If we *wanted* it to open up the style editor, then we get further into questions about saving changes back to disk, and confusion about why someone is seeing a stylesheet they haven't added.
3) With editing, how do we explain that the changes they are making don't belong to their CSS, and will not be able to be copied back into a style sheet?
4) With editing, how do we explain that they aren't permanently messing up their user agent styles by changing them?
Comment 20 User image Jonathan Watt [:jwatt] 2014-05-23 06:59:15 PDT
(In reply to Brian Grinstead [:bgrins] from comment #19)
> My vote is to land this as non-editable/linkable and handle that in
> a follow up.

I think the primary use case for this bug is gecko developers needing to see the UA rules. The rules being non-editable/linkable makes this pretty much useless to me and I assume others in this category.

Instead can we maybe just land your patch with a pref to toggle the filter, off by default? Then gecko devs have a useful tool and there's no UI needed, and thus no UX issues.

The follow-up could then be to create UI, where all the UX issues could be hashed out.
Comment 21 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 07:26:32 PDT
(In reply to Jonathan Watt [:jwatt] from comment #20)
> (In reply to Brian Grinstead [:bgrins] from comment #19)
> > My vote is to land this as non-editable/linkable and handle that in
> > a follow up.
> 
> I think the primary use case for this bug is gecko developers needing to see
> the UA rules. The rules being non-editable/linkable makes this pretty much
> useless to me and I assume others in this category.

It sounds like there are two different use cases here.  I disagree that this feature is not useful / less important for the non gecko dev.  As jryans pointed out in comment 1, seeing these can help debug a web page.  Also, this feature is on by default in Chrome.

> The follow-up could then be to create UI, where all the UX issues could be hashed out.

The UX issues can be avoided altogether if a user never sees editing controls or links for UA styles.

> Instead can we maybe just land your patch with a pref to toggle the filter,
> off by default? Then gecko devs have a useful tool and there's no UI needed,
> and thus no UX issues.

Maybe I'm missing something, but in Comment 5 you pointed out that you don't care if the properties are editable.  How frequently are you modifying the user agent stylesheets?
Comment 22 User image Jonathan Watt [:jwatt] 2014-05-23 07:30:02 PDT
Yeah, I don't care so much about the editability as I do about seeing the rule, and seeing which file and line it came from. The latter is far more important to me.
Comment 23 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 08:04:15 PDT
Created attachment 8427783 [details] [diff] [review]
useragent-WIP.patch

Patrick, do you think using pointer-events: none is too hacky of a workaround for making these not editable?   Otherwise we are going to have to propagate isSystem through ruleeditor/textpropertyeditor and all of the different ways/places that things can be edited.  This would require some refactoring, but it would be doable.

One thing I like about this solution is that we don't have to worry about the output parser (for instance, a color swatch shouldn't open a colorpicker tooltip if the rule is non editable, but it could theoretically open a font tooltip).  This just kills any interaction with the elements (besides text selection).
Comment 24 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-23 08:34:41 PDT
Comment on attachment 8427783 [details] [diff] [review]
useragent-WIP.patch

It's true that it simplifies the code quite a bit!
But I'm afraid it could lead to confusion when maintaining the code later on. People might wonder why things are editable, but in fact not really.
Also, will the pointer-event thing also block the tooltips from appearing? If yes, it would be a shame.

I haven't studied the code too long, but from what I saw, you have a 'isSystem' property per 'Rule' object. When building a 'RuleEditor', we get the rule as argument, and we later create 'TextPropertyEditor' for each property. That's where editable fields get created and that's where color swatches become editable (see 'this.ruleEditor.ruleView.colorPicker.addSwatch').
So it sounds relatively easy to add conditions in these places on 'isSystem'.

Of course, this will complexify the code further. Maybe another way (which will take more time) would be to create a 'TextProperty' class which the 'TextPropertyEditor' class would inherit from. 'TextProperty' would do everything 'TextPropertyEditor' does today except everything that is editing-related. 'TextPropertyEditor' would take care of this.

I hope this helps.
Comment 25 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 08:40:54 PDT
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> Comment on attachment 8427783 [details] [diff] [review]
> useragent-WIP.patch
> 
> It's true that it simplifies the code quite a bit!
> But I'm afraid it could lead to confusion when maintaining the code later
> on. People might wonder why things are editable, but in fact not really.
> Also, will the pointer-event thing also block the tooltips from appearing?
> If yes, it would be a shame.

Yes, it would block tooltips from appearing.

> I haven't studied the code too long, but from what I saw, you have a
> 'isSystem' property per 'Rule' object. When building a 'RuleEditor', we get
> the rule as argument, and we later create 'TextPropertyEditor' for each
> property. That's where editable fields get created and that's where color
> swatches become editable (see
> 'this.ruleEditor.ruleView.colorPicker.addSwatch').
> So it sounds relatively easy to add conditions in these places on 'isSystem'.
> 
> Of course, this will complexify the code further.

Yes, that is the solution I started with, but realized it gets complicated quickly.  For instance, in both the ruleeditor and the textpropertyeditor the non-editable UI is built directly alongside the editable fields.  I can follow up with making the required changes along with refactors.
Comment 26 User image (Unavailable until Apr 3) [:bgrins] 2014-05-23 13:01:48 PDT
Created attachment 8427933 [details] [diff] [review]
useragent-WIP.patch

Can you review the code changes?  I'm still working on the test, but I'm setting the pref to false by default so it shouldn't affect the existing test suite.
Comment 27 User image (Unavailable until Apr 3) [:bgrins] 2014-05-24 12:08:14 PDT
Oh, and an easy URL for testing is data:text/html,<blockquote style='color:red;' type=cite><pre _moz_quote=true>fun <em style='color:orange'>to</em> inspect</code></blockquote>
Comment 28 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-26 02:32:01 PDT
A couple of remarks before the code review:

- The cursor acts a little bit weirdly:
   - this isn't related to your patch, but when hovering over one of the stylesheets link, it alternates between pointer and default, depending on how you place your mouse
   - it should probably turn to default when hovering over the UA style rules, to indicate they aren't editable
   - the stylesheet links do not get underlined when hovered

- We now have a pref for browser-styles in the rule-view, and a checkbox for browser-styles in the computed-view. Probably not in this bug, but do you think we should merge both concepts and only have the pref?
Comment 29 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-26 02:32:40 PDT
Created attachment 8428592 [details]
cursor-ruleview-links.gif

Shows the cursor and missing underline on stylesheets links
Comment 30 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-26 02:36:57 PDT
2 more cursor-related comments:
- the cursor shouldn't be pointer when hovering over a colorswatch in browser-style
- the short-hand properties expander icon should either work when clicked or be hidden. Right now it is there, but nothing happens when you click it.
Comment 31 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-26 03:03:48 PDT
Comment on attachment 8427933 [details] [diff] [review]
useragent-WIP.patch

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

Not many comments about the code. Looks good to me.
Of course we need some tests.

::: browser/devtools/styleinspector/rule-view.js
@@ +1347,5 @@
> +    }
> +
> +    // Reselect the currently selected element
> +    if (data.pref === PREF_UA_STYLES ||
> +        data.pref === "devtools.defaultColorUnit") {

While you're at it, it would be good to reference this pref string as a const as you've done for PREF_UA_STYLES.

Also, there are only 2 pref we use to refresh the view here, so it's not a big deal if you don't do it, but I'd prefer something like this instead:

let refreshOnPrefs = [PREF_UA_STYLES, PREF_DEFAULT_COLOR_UNIT];
if (refreshOnPrefs.contains(data.pref)) {
  ...
}

Makes it easy to add new prefs to refresh later (I will add one when I resume working on the authored styles).

@@ +1442,5 @@
>        this._showEmpty();
>        return promise.resolve(undefined);
>      }
>  
> +    this._elementStyle = new ElementStyle(aElement, this.store, this.pageStyle, this.showUserAgentStyles);

nit: find a nice way to cut at around 80 chars.

@@ +1664,5 @@
>  function RuleEditor(aRuleView, aRule) {
>    this.ruleView = aRuleView;
>    this.doc = this.ruleView.doc;
>    this.rule = aRule;
> +  this.editable = !aRule.isSystem;

nit: I usually find boolean properties easier to understand when they have the "is" prefix, like this.isEditable, but I'm being really picky :)

@@ +1749,5 @@
>        }
>      }, false);
>  
> +    if (this.editable) {
> +      code.addEventListener("click", function() {

I know this code was here before, but the '.bind(this)' could be removed if we used a fat arrow function here.

@@ +1756,5 @@
> +          this.newProperty();
> +        }
> +      }.bind(this), false);
> +
> +      this.element.addEventListener("mousedown", function() {

ditto

@@ -2019,5 @@
>      // Click to expand the computed properties of the text property.
>      this.expander = createChild(this.element, "span", {
>        class: "ruleview-expander theme-twisty"
>      });
> -    this.expander.addEventListener("click", this._onExpandClicked, true);

As commented earlier, I think this should stay, even if the rule isn't editable.

@@ +2096,5 @@
>        class: "ruleview-computedlist",
>      });
>  
> +    // Only bind event handlers if the rule is editable.
> +    if (this.ruleEditor.editable) {

It's a good thing that we now have all these grouped in one place, it gives us an easier way to ultimately separate better a "normal" TextProperty from a TextPropertyEditor, if one day we want to do it.

::: browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js
@@ +7,5 @@
> +// Check that user agent styles are inspectable via rule view
> +
> +
> +  // XXX check matching styles from server with styles from frontend.
> +  // Probably can't hardcode the checks here since UA styles could be different across platforms

There probably are some UA styles that are the same across platforms (links colors?), maybe you could use those.

@@ +8,5 @@
> +
> +
> +  // XXX check matching styles from server with styles from frontend.
> +  // Probably can't hardcode the checks here since UA styles could be different across platforms
> +  // Could also run through after toggling pref to make sure there are less (and no isSystem styles)

Yes, that's a good idea. You can also test, when the pref is ON, that there is at least a [uneditable] section in the rule-view doc and that clicking doesn't focuses any editableField in the UI.

::: browser/themes/shared/devtools/ruleview.css
@@ +41,5 @@
>    padding: 2px 4px;
>  }
>  
> +/* User agent styles are not editable, display them differently */
> +.theme-light .ruleview-rule[uneditable=true] {

nit: all rules in this file are separated by an empty line, please separate all the "uneditable" rules.
Comment 32 User image (Unavailable until Apr 3) [:bgrins] 2014-05-27 06:54:15 PDT
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #28)
> A couple of remarks before the code review:
> 
> - The cursor acts a little bit weirdly:
>    - this isn't related to your patch, but when hovering over one of the
> stylesheets link, it alternates between pointer and default, depending on
> how you place your mouse

I've fixed this in the patch

>    - it should probably turn to default when hovering over the UA style
> rules, to indicate they aren't editable

I'm assuming you are referring to the color swatch here - I don't see any other pointer cursors on the UA parts.  I've fixed this.

>    - the stylesheet links do not get underlined when hovered

Fixed

> - We now have a pref for browser-styles in the rule-view, and a checkbox for
> browser-styles in the computed-view. Probably not in this bug, but do you
> think we should merge both concepts and only have the pref?

I was thinking about this before.  They seem a bit different to me - the extra styles would be much more in the way in computed styles than they are here.  That box is probably something you would only want to check temporarily when debugging an issue.  It's definitely something we should discuss further though - consolidating two options into one would of course be nice.
Comment 33 User image (Unavailable until Apr 3) [:bgrins] 2014-05-27 11:25:01 PDT
Created attachment 8429440 [details] [diff] [review]
user-agent-styles.patch

Addresses feedback, and adds two tests for this.  One to make sure the ua styles are visible and disappear / reappear on pref change, and one to make sure they are not editable.  I've used a timeout to make sure that the colorpicker tooltip and inplace editor doesn't open up when clicking on elements in the test, I'm not sure if there is a better way to handle that.
Comment 34 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2014-05-27 14:29:55 PDT
Comment on attachment 8429440 [details] [diff] [review]
user-agent-styles.patch

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

No complaints about the code. I see all the points I raised fixed.
I just have a couple of comments about one of the tests, to simplify it and avoid intermittents.

::: browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles-uneditable.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Check that user agent styles are inspectable via rule view if

Copy/pasted comment. Please change to detail what this test does.

@@ +32,5 @@
> +function* userAgentStylesUneditable(inspector, view) {
> +  info ("Making sure that UI is not editable for user agent styles");
> +
> +  yield selectNode("a", inspector);
> +  let userRules = view._elementStyle.rules.filter(rule=>rule.editor.isEditable);

This isn't used in the code

@@ +44,5 @@
> +      info ("Clicking color swatch and waiting to make sure popup doesn't show up");
> +      let onShown = view.colorPicker.tooltip.once("shown");
> +      let popupShownPromise = waitForPromise(onShown);
> +      colorswatch.click();
> +      let popupWasShown = yield popupShownPromise;

Another way to check this and avoid having to wait is to use the colorPicker's 'swatches' property.
If it contains colorswatch as one of its key, then it's editable.

@@ +49,5 @@
> +      ok (!popupWasShown, "Popup was not shown when clicking on color swatch")
> +    }
> +
> +    let editable = rule.editor.closeBrace;
> +    let onFocus = waitForPromise(once(editable.parentNode, "focus", true));

This could create intermittent failures, especially on slow platforms running debug builds. 200ms seems pretty low considering those cases.
I quickly looked at the inplace-editor code and it looks like the '_editable' is added to the element that is made editable. So perhaps you could just check if this property exists.

@@ +57,5 @@
> +
> +    ok (!rule.editor.newPropSpan, "There is no new prop span");
> +
> +    let nameSpan = rule.textProps[0].editor.nameSpan;
> +    let onNameFocus = waitForPromise(once(nameSpan.parentNode, "focus", true));

same here
Comment 35 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 10:44:21 PDT
Created attachment 8430184 [details]
about-preference-stylesheet.png

When inspecting a link, I see a sheet with an href of about:PreferenceStyleSheet defining the color.  Clicking on the link tries to open "about:PreferenceStyleSheet", and loads an error page.

The only place in the project I see path referenced is here: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1340.  Jonathan, any idea why this would be showing up this way, and can you think of a workaround?
Comment 36 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 10:45:47 PDT
Created attachment 8430188 [details] [diff] [review]
user-agent-styles-r=pbrosset.patch

Thanks for the tips on the tests Patrick - that is much better
Comment 37 User image Jonathan Watt [:jwatt] 2014-05-28 12:12:01 PDT
The preferences style sheet is created on the fly internally and only has an internal representation. Although it is given the URI "about:PreferenceStyleSheet" it is not registered with the "about:" handler (the name is purely for identification purposes, I'd assume).

Is it practical to special case this URI and avoid linking it?

Longer term, I think fixing bug 677302 should mean we could move the preference style sheet rules to other UA sheets that start life as a file that we load in rather than generate on the fly.
Comment 38 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 12:27:24 PDT
(In reply to Jonathan Watt [:jwatt] from comment #37)
> The preferences style sheet is created on the fly internally and only has an
> internal representation. Although it is given the URI
> "about:PreferenceStyleSheet" it is not registered with the "about:" handler
> (the name is purely for identification purposes, I'd assume).
> 
> Is it practical to special case this URI and avoid linking it? 

That would be an easy change - I'll make it before landing and leave a comment referring to 677302.
Comment 39 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 12:50:32 PDT
Created attachment 8430256 [details] [diff] [review]
user-agent-styles-r=pbrosset.patch

Special cases about:PreferenceStyleSheet to not be linkable - minor change.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cb0e04a23c19.
Comment 40 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 12:53:11 PDT
Created attachment 8430258 [details] [diff] [review]
user-agent-styles-r=pbrosset.patch

Updated commit message
Comment 41 User image (Unavailable until Apr 3) [:bgrins] 2014-05-28 19:04:34 PDT
*** Bug 1007288 has been marked as a duplicate of this bug. ***
Comment 43 User image Wes Kocher (:KWierso) 2014-05-29 17:59:57 PDT
https://hg.mozilla.org/mozilla-central/rev/40046039330c
Comment 44 User image Francesco Lodolo [:flod] 2014-05-30 00:19:07 PDT
Note for the next update: not sure why you landed the 2 new strings between a label and its accesskey (in the middle of the "color" group)
http://hg.mozilla.org/mozilla-central/diff/40046039330c/browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
Comment 45 User image (Unavailable until Apr 3) [:bgrins] 2014-06-04 14:03:35 PDT
Will, just a heads up - for FF32 user agent styles are inspectable in the rule view.  They are off by default, but can be enabled in the options panel under Inspector -> "Show Browser Styles".  Is this something you think we should add to the inspector docs?
Comment 46 User image Will Bamberg [:wbamberg] 2014-06-04 14:23:52 PDT
Thanks Brian, I'll make sure it's included.
Comment 47 User image Jonathan Watt [:jwatt] 2014-06-05 07:55:36 PDT
Thanks, this is awesome!
Comment 48 User image (Unavailable until Apr 3) [:bgrins] 2014-06-06 10:35:33 PDT
*** Bug 940950 has been marked as a duplicate of this bug. ***
Comment 49 User image Julien Wajsberg [:julienw] 2014-06-06 16:50:45 PDT
I don't understand why the "computed" tab has a "Browser styles" checkbox, while for the "Rules" tab we need to go to the options. This looks inconsistent at best.

Should I file another bug for this?

(but other than that, Yay :) )
Comment 50 User image PTO - Michael Ratcliffe [:miker] [:mratcliffe] 2014-06-07 06:00:23 PDT
(In reply to Julien Wajsberg [:julienw] from comment #49)
> I don't understand why the "computed" tab has a "Browser styles" checkbox,
> while for the "Rules" tab we need to go to the options. This looks
> inconsistent at best.
> 
> Should I file another bug for this?
> 
> (but other than that, Yay :) )

I partially agree, but having them on all of the time in the computed view would be confusing. Maybe we should have cogs in each tool that allow you to set settings just for that tool. If you could log a bug for that and CC me that would be great.
Comment 51 User image Julien Wajsberg [:julienw] 2014-06-07 10:45:35 PDT
Yeah, I'd prefer to have the checkbox in both views (possibly hidden in a "cog").

Filed bug 1022220

Note You need to log in before you can comment on or make changes to this bug.