1076788 - user-agent styles should be editable when debugging chrome

Status: RESOLVED DUPLICATE of bug 1230491

(DevTools :: Inspector, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: inspectingchrome.png

When working on devtools chrome styling, often, styles applied tagged as "user-agent" can be more specific than non user-agent styles, resulting in the user-agent styles appearing above non-user-agent styles. When working with non-chrome (normal content), this would never occur. This makes sense based off of the specificity order.

The problem is, it's difficult to experiment and tweak existing values tagged as user-agent, and it seems inconsistent what should be considered a user-agent within chrome context.

tagged as user-agent:
* dark-theme.css, light-theme.css
* data:text/css URIs
* minimal-xul.css

Not tagged as user-agent
* common.css
* global.css
* intl.css

As specificity order should be maintained, maybe it'd make sense if user-agent styles were editable when inspecting chrome, as a solution

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Editing existing styles/adding styles would be grand, but even just toggling off/on styles applied by user-agents would be huge as well!

Comment 2

[Patrick Brosset <:pbro>]

I don't see why theme files are tagged as user-agent stylesheets. They shouldn't!
Looks like CssLogic.isContentStylesheet is responsible for that, so I'm betting there's just a bug in there.

Comment 3

[Patrick Brosset <:pbro>]

:-moz-devtools-highlighted comes from the SimpleOutlineHighlighter which is what we use to highlight elements in the browser chrome (the red dotted outline).

In terms of rules specificity, I think rules are ordered properly.

Comment 4

[Brian Grinstead [:bgrins]]

They are marked as user agent styles because they are actually being loaded as agent styles: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#59.  I know there is a reason for this, but I can't remember exactly why.  It's using the addon SDK StylesheetUtils.loadSheet to load it.

Comment 5

[Brian Grinstead [:bgrins]]

As far as general editing of user agent styles, it was going to require some extra work and I didn't want to work around it since editing UA styles seemed like a pretty niche use case.  Of course here it isn't since many of the devtools styles are loaded as them.  I think a good fix would be to modify theme-switching.js to load the dark-theme and light-theme files as normal sheets above other sheets on the page

Comment 6

[Brian Grinstead [:bgrins]]

Patrick and I looked further into this and realized that the dark / light theme are meant to be loaded as "author" styles.  But any sheet added with the addon sdk (and therefore DOMUtils.loadSheet) is actually using the same function as the actual UA style loading [0] regardless of the type passed into loadSheet.  I'm not sure if we have a way from isContentStylesheet to determine the difference.  So unless there is a way from JS to tell the difference, we could either:

1) Whitelist the known UA stylesheets and only count those as system
2) Load stylesheets for dark / light theme using DOM methods instead of DOMUtils.loadSheet.  This could get a little tricky making sure it works with both XUL and HTML docs, and since loadSheet is sync while this would be async.  However, it could solve a few issues in that we could make sure it gets inserted before any other stylesheets, while it is currently just being appended after everything which is a bit unintuitive.

[0]: https://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp#249

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

That could work -- but wondering, beyond the dark/light themes, there are other user-agent styles that can be more specific than the light/dark themes and are uneditable (in the screenshot, there's a data:text/css and a minimal-xul.css) -- can we just make user-agent sheets editable all together in the inspector (when in the browser toolbox), even if its not a theme style? If I'm hacking on Firefox chrome, shouldn't I be able to edit those as well? In this case, I'm working on devtools, but if I'm actually working on chrome styling, or making a skin, etc., shouldn't I be able to edit these values to experiment with what I'm trying to make?

For example, if we just make *all* rules in RuleEditor[1] have `isEditable` true, is there any down side?

[1]https://github.com/mozilla/gecko-dev/blob/4c98b091378945f10dfa2e4bb115b4b13d335814/browser/devtools/styleinspector/rule-view.js#L1821

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> That could work -- but wondering, beyond the dark/light themes, there are
> other user-agent styles that can be more specific than the light/dark themes
> and are uneditable (in the screenshot, there's a data:text/css and a
> minimal-xul.css) -- can we just make user-agent sheets editable all together
> in the inspector (when in the browser toolbox), even if its not a theme
> style? If I'm hacking on Firefox chrome, shouldn't I be able to edit those
> as well? In this case, I'm working on devtools, but if I'm actually working
> on chrome styling, or making a skin, etc., shouldn't I be able to edit these
> values to experiment with what I'm trying to make?
> 
> For example, if we just make *all* rules in RuleEditor[1] have `isEditable`
> true, is there any down side?

We could try to do this for the case of the Browser Toolbox.  I'm not convinced that this would provide any value to most of our user base - and it may actually cause some confusion.  We could pref this off by default, then set a pref on load in toolbox-process-window.js to always allow editing of these in the Browser Toolbox.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Oh, I definitely think this should only be for the browser toolbox, as editing content pages wouldn't have a need for this, but only for browser toolbox inspecting chrome. Do you think having user-agent styles editable in browser toolbox would be confusing? The user base would only be people hacking on Firefox itself, right? (Maybe there are other uses of browser toolbox that I'm unaware of, assuming that remote debugging FxOS/Fennec does not inherit this option as well)

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #9)
> Oh, I definitely think this should only be for the browser toolbox, as
> editing content pages wouldn't have a need for this, but only for browser
> toolbox inspecting chrome. Do you think having user-agent styles editable in
> browser toolbox would be confusing? The user base would only be people
> hacking on Firefox itself, right? (Maybe there are other uses of browser
> toolbox that I'm unaware of, assuming that remote debugging FxOS/Fennec does
> not inherit this option as well)

Yes I think it will make sense and not be too confusing for anyone using the Browser Toolbox.  I will make a patch to see if things work as expected.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: edit-ua-styles.patch

needs test

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1076788 - Add preference to edit UA styles in rule view;r=tromey

Bug 1076788 - Add preference to edit UA styles in rule view;r=tromey

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8695994 [details]
MozReview Request: Bug 1076788 - Add preference to edit UA styles in rule view;r=tromey

https://reviewboard.mozilla.org/r/27241/#review24655

The patch itself seems fine, so r+.

However I was not really clear on the motivation for the patch.  My guess is that adding the pref lets "power users" turn this on, while not exposing the feature to most users.  If so, that seems reasonable to me.

That said, beware of bug 1205646 -- it seems likely to me (though I didn't try it) that the crashes there could be reproduced by editing UA style sheets as well.

Comment 15

[Brian Grinstead [:bgrins]]

Going to hold off on this until we can look into bug 1230491, which I think is a better fix

Comment 16

[Brian Grinstead [:bgrins]]

I think this will won't be needed as of Bug 1230491