Firebug theme implemented in Firebug.next should be ported into devtools. Honza

So are you envisioning this as a third option in the settings panel? Also, will it need to control extra Firebug.next behavior (i.e. turn on certain tools / features when theme is enabled)?

(In reply to Brian Grinstead [:bgrins] from comment #1) > So are you envisioning this as a third option in the settings panel? Yes, exactly. > Also, will it need to control extra Firebug.next behavior (i.e. turn on certain > tools / features when theme is enabled)? No extra features, just colors, fonts and layout. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #2) > (In reply to Brian Grinstead [:bgrins] from comment #1) > > So are you envisioning this as a third option in the settings panel? > Yes, exactly. > > > Also, will it need to control extra Firebug.next behavior (i.e. turn on certain > > tools / features when theme is enabled)? > No extra features, just colors, fonts and layout. Does it default to 'light' theme with additions? I ask because I think we still have some hardcoded theme-light and theme-dark classes interspersed in CSS, and it'd be nice to not have to add a third special case in these places. Could be motivation to finally remove the last remaining places where that happens! But there are also cases where tool-specific variables are declared at the top of a tool's file and references light and dark theme directly, which probably won't go away anytime soon. Also we'd need to make sure it interacts properly with the DE theme (probably defaulting to light).

(In reply to Brian Grinstead [:bgrins] from comment #3) > (In reply to Jan Honza Odvarko [:Honza] from comment #2) > > (In reply to Brian Grinstead [:bgrins] from comment #1) > > > So are you envisioning this as a third option in the settings panel? > > Yes, exactly. > > > > > Also, will it need to control extra Firebug.next behavior (i.e. turn on certain > > > tools / features when theme is enabled)? > > No extra features, just colors, fonts and layout. > > Does it default to 'light' theme with additions? The current implementation is not based on light theme. > I ask because I think we > still have some hardcoded theme-light and theme-dark classes interspersed in > CSS, and it'd be nice to not have to add a third special case in these > places. Could be motivation to finally remove the last remaining places > where that happens! But there are also cases where tool-specific variables > are declared at the top of a tool's file and references light and dark theme > directly, which probably won't go away anytime soon. I have working prototype in Firebug.next so, we should test that. > Also we'd need to make sure it interacts properly with the DE theme > (probably defaulting to light). Yes There are other tough issues we need to solve. Here is a quick summary: 1) Firebug theme consists from many small *.css files. E.g. there is one file per panel, per component, etc. Not sure what will be the right location (directories) for them. 2) Should we already think about CSS Modules or any other new way we want to use when dealing with CSS? 3) Firebug theme is not only fonts and colors, but also layout changes: * Firebug theme has one search box UI to make the search UX consistent across panels. The way how the theme is overriding the default situation (every panel implements its own search experience) needs polishing and probably a platform support. * The theme unifies toolbars - Every panel in Firebug has a toolbar at the top. E.g. the Network panel has a new toolbar and filter buttons (originally located at the bottom). * The theme introduces a button that can be used to open/collapse side bars. * The theme supports srollbararrows for list of tabs * Unified UI for panel options I put together a doc with bunch of screenshots explaining basic aspects of the theme: https://docs.google.com/document/d/1t1_DMLl8U91mbcZnqjxryHp25EilC_y-KEsX6EsvtD0/edit# --- We might want to have some of the things supported by all themes (e.g. unified search UI), but keep in mind that the Firebug theme should be ready in DevTools when e10s is on in Firefox and Firebug 2 stops working. Honza

Can we keep the Firebug theme as simple as possible ? Ideally, it'd be nice if the Firebug theme *only* consisted of custom CSS variables. I don't see why the layout changes need to be done only for the Firebug theme, for example, moving the network monitor toolbar on top or adding a button to open/collapse sidebars can be done for all themes. If something very Firebug specific needs to be added, I think it belongs as an extension.

My main concern about this theme is the maintenance cost behind it, if the theme only consists of CSS vars, it should be easy to maintain, but if it has to inject CSS files in every tool, and do layout changes throughout the toolbox, that's a different story.

(In reply to Tim Nguyen [:ntim] from comment #5) > Can we keep the Firebug theme as simple as possible ? Ideally, it'd be nice > if the Firebug theme *only* consisted of custom CSS variables. I don't see > why the layout changes need to be done only for the Firebug theme, for > example, moving the network monitor toolbar on top or adding a button to > open/collapse sidebars can be done for all themes. > > If something very Firebug specific needs to be added, I think it belongs as > an extension. The layout changes are because we're migrating a large number of Firebug users into the Firefox devtools; we'd like the Firefox devtools to be as familiar as possible. This will be hugely beneficial to devtools, as we'll have a lot more people using and filing bugs and identifying UX issues within the tools. There will be a lot of maintenance overhead with the change, but everyone on the team is in agreement that it is worth doing.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #7) > (In reply to Tim Nguyen [:ntim] from comment #5) > > Can we keep the Firebug theme as simple as possible ? Ideally, it'd be nice > > if the Firebug theme *only* consisted of custom CSS variables. I don't see > > why the layout changes need to be done only for the Firebug theme, for > > example, moving the network monitor toolbar on top or adding a button to > > open/collapse sidebars can be done for all themes. > > > > If something very Firebug specific needs to be added, I think it belongs as > > an extension. > > The layout changes are because we're migrating a large number of Firebug > users into the Firefox devtools; we'd like the Firefox devtools to be as > familiar as possible. > > This will be hugely beneficial to devtools, as we'll have a lot more people > using and filing bugs and identifying UX issues within the tools. > > There will be a lot of maintenance overhead with the change, but everyone on > the team is in agreement that it is worth doing. I sensed the argument in Comment 5 / 6 is that many of the layout changes would make sense to just fix in the current themes (i.e. Bug 1247485). I'm also concerned about having a fork of our light and dark theme in place in terms of accidentally breaking the theme as markup / styles change. Like do the individual tool files @import the current theme's CSS or is it entirely copy/pasted styles? Honza, maybe it would help if you could upload a WIP so I can get a sense for the scope / type of the changes.

(In reply to (Unavailable until 3-7) Brian Grinstead [:bgrins] from comment #8) > I sensed the argument in Comment 5 / 6 is that many of the layout changes > would make sense to just fix in the current themes (i.e. Bug 1247485). Yes, if that's doable in the given time-frame (till e10s kills Firebug 2). > I'm also concerned about having a fork of our light and dark theme in place > in terms of accidentally breaking the theme as markup / styles change. Like > do the individual tool files @import the current theme's CSS or is it > entirely copy/pasted styles? Honza, maybe it would help if you could upload > a WIP so I can get a sense for the scope / type of the changes. There is no patch wip atm. All I have is the Firebug 3 itself. It doesn't fork the light theme, it's entirely copy/pasted. https://github.com/firebug/firebug.next/blob/master/chrome/skin/classic/shared/firebug-theme.css Honza

First rough copy-paste patch attached: Comments: 1) All related css/images are in devtools/client/themes/firebug. I am step by step moving images to devtools/client/themes/images/firebug and exposing through chrome:// protocol 2) All CSS files are @imported in devtools/client/themes/firebug-theme.css (located at the bottom of jar.mn file) 3) Arrow scrollbox (for panel tabs) is not included 4) Shared search box is not included 5) Panel options menu is not included (do we want it?) 6) Tested in Win only (a screenshot will follow) More cleanup (mainly removing obsolete CSS styles) coming! Honza

I've largely simplified your previous patch to this. The theme is basically a mod of the light theme now. There are a lot of image swaps to do, but this patch should give you a good idea on how to do it. Hopefully this helps.

I ran all the firebug theme SVGs through SVGO, so they're slimmer now.

This patch applies on top of bug 1255116 btw.

Comment on attachment 8735545 [details] [diff] [review] Very very simple implementation of the Firebug theme Review of attachment 8735545 [details] [diff] [review]: ----------------------------------------------------------------- Honza, would like your feedback on this approach. This is close to what I was envisioning for maintainability, but does it get 'close enough' to the current Firebug theme and/or could it be extended to be?

(In reply to Tim Nguyen [:ntim] from comment #12) > Created attachment 8735541 [details] [diff] [review] > Very very simple implementation of the firebug theme > > I've largely simplified your previous patch to this. The theme is basically > a mod of the light theme now. There are a lot of image swaps to do, but this > patch should give you a good idea on how to do it. Hopefully this helps. Nice! And yes, I agree with the way it's done. It's just question of how much we can do in the time-frame we have. Note that we have to land the theme in 48 i.e. in three weeks (including this one). (In reply to Brian Grinstead [:bgrins] from comment #17) > Comment on attachment 8735545 [details] [diff] [review] > Honza, would like your feedback on this approach. This is close to what I > was envisioning for maintainability, but does it get 'close enough' to the > current Firebug theme and/or could it be extended to be? Yes, this is the nice solution we mentioned at our meeting, but many many little things are broken since most of the Firebug theme CSS has been removed. I like this way and I am playing with that now, but I am skeptical we can do all nicely in time (without copying bunch of CSS back in and make it nice step by step). Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #18) > (In reply to Tim Nguyen [:ntim] from comment #12) > > Created attachment 8735541 [details] [diff] [review] > > Very very simple implementation of the firebug theme > > > > I've largely simplified your previous patch to this. The theme is basically > > a mod of the light theme now. There are a lot of image swaps to do, but this > > patch should give you a good idea on how to do it. Hopefully this helps. > Nice! And yes, I agree with the way it's done. It's just question of how much > we can do in the time-frame we have. Note that we have to land the theme in > 48 > i.e. in three weeks (including this one). > > (In reply to Brian Grinstead [:bgrins] from comment #17) > > Comment on attachment 8735545 [details] [diff] [review] > > Honza, would like your feedback on this approach. This is close to what I > > was envisioning for maintainability, but does it get 'close enough' to the > > current Firebug theme and/or could it be extended to be? > Yes, this is the nice solution we mentioned at our meeting, but many many > little things are broken since most of the Firebug theme CSS has been > removed. I like this way and I am playing with that now, but I am skeptical > we can do all nicely in time (without copying bunch of CSS back in and make > it nice step by step). The thing I like about it is that it's something we could land now (behind a pref), and then we can re-add in the missing things one by one. Some of which will require copy/pasting / duplicate code, but we can be explicit about what we are doing that with. It'll likely be easier to add in any extra code needed over the next 2 weeks than to remove a lot of code later on down the road. So the idea is: land this now, then go through each tool / component from the bigger patch and pull in whatever styles are necessary to reach compatibility with the current theme (possibly dumping into the bottom of firebug-theme.css depending on how much we end up needing to copy in). I don't have a great sense of how much is missing from it so I'd like your opinion on if that could work.

(In reply to Brian Grinstead [:bgrins] from comment #19) > The thing I like about it is that it's something we could land now (behind a > pref), and then we can re-add in the missing things one by one. Some of > which will require copy/pasting / duplicate code, but we can be explicit > about what we are doing that with. It'll likely be easier to add in any > extra code needed over the next 2 weeks than to remove a lot of code later > on down the road. > > So the idea is: land this now, then go through each tool / component from > the bigger patch and pull in whatever styles are necessary to reach > compatibility with the current theme (possibly dumping into the bottom of > firebug-theme.css depending on how much we end up needing to copy in). I > don't have a great sense of how much is missing from it so I'd like your > opinion on if that could work. So, we have two options: 1) Ship 'very very simple' Firebug theme Risks: The theme is missing many details and is far from the original. The first user-impression will be bad and we fail our goal to make Firebug users happy. Pros: We can land small piece of code that is easily maintainable. 2) Ship Firebug theme Risks: We have to land rather bigger piece of code (~70kB of CSS atm) that needs to be later adopted to match the existing CSS architecture. Pros: We have working Firebug theme ready for Firebug users migration. Note that neither of these solutions is doing any layout changes, it's just pure CSS. I think that's what we all want. --- I have been playing with #1 yesterday and it was quite slow (and also a bit frustrating) to start from nothing and bit by bit move code from the bigger patch and adopt it so, it matches the ideal solution (e.g. using variables whenever possible). It's hard to estimate how far I can get in given timeframe (not counting testing on Win/Mac/Linux, DOM panel, HTTPi, JSON Viewer and perhaps some extensions I am maintaining). This way is slow since it also includes analyses/changes of the existing CSS (e.g. the Options tool has hardcoded icon URL in definitions.js and so, having CSS variable for the image-url isn't possible atm, etc.) I switched to #2 today. Continue simplifying and clean up the bigger patch. And it's definitely easier to remove than add in any extra code. It's also safer since I am already working with working Firebug theme and I can also concentrate on fixing details. This way I can spend following 2 weeks + 2 days on clean up and fixing theme issues and adopt some parts of the code along the way so, it matches the architecture we want (nicely demonstrated by ntim). This is a lot less risky (in terms of product delivery) since we already have something and we can spend the rest of the time making it better. We are not even limited by the merge day. The new patch changes summary: - It adopts ntim patch (except of the toolbar variables) - Global theme variables updated - Some tool specific variables introduced. - A lot of CSS rules removed - All images are now in themes/images/firebug - All firebug specific CSS files are in themes/firebug/ (all CSS rules are nicely splitted into small files like: netmonitor.css, debugger.css, layout-view.css, etc.) - Firebug theme is based on Light theme - New images overwrites are done through CSS vars (e.g. command-pick.svg, docking buttons, etc.) --- So, I am thinking about the following: 1) Pick plan #2 2) Continue clean up (mainly remove obsolete CSS). 3) Adopting the code: the Toolbox-tabs and Toolbar-buttons CSS is the hardest. It would be great if someone could help to adopt it. I can provide screenshots, summary of features and location of the related code. 4) Concentrate on individual panels and testing on Win/Mac/Linux and extensions. What do you think? Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #20) > Created attachment 8736492 [details] [diff] [review] > bug1244054-theme.patch > > (In reply to Brian Grinstead [:bgrins] from comment #19) > > The thing I like about it is that it's something we could land now (behind a > > pref), and then we can re-add in the missing things one by one. Some of > > which will require copy/pasting / duplicate code, but we can be explicit > > about what we are doing that with. It'll likely be easier to add in any > > extra code needed over the next 2 weeks than to remove a lot of code later > > on down the road. > > > > So the idea is: land this now, then go through each tool / component from > > the bigger patch and pull in whatever styles are necessary to reach > > compatibility with the current theme (possibly dumping into the bottom of > > firebug-theme.css depending on how much we end up needing to copy in). I > > don't have a great sense of how much is missing from it so I'd like your > > opinion on if that could work. > > So, we have two options: > > 1) Ship 'very very simple' Firebug theme > Risks: The theme is missing many details and is far from the original. The > first user-impression will be bad and we fail our goal to make Firebug users > happy. > Pros: We can land small piece of code that is easily maintainable. > > 2) Ship Firebug theme > Risks: We have to land rather bigger piece of code (~70kB of CSS atm) that > needs to be later adopted to match the existing CSS architecture. > Pros: We have working Firebug theme ready for Firebug users migration. > > > Note that neither of these solutions is doing any layout changes, it's just > pure CSS. I think that's what we all want. > > --- > > I have been playing with #1 yesterday and it was quite slow (and also a bit > frustrating) to start from nothing and bit by bit move code from the bigger > patch and adopt it so, it matches the ideal solution (e.g. using variables > whenever possible). It's hard to estimate how far I can get in given > timeframe (not counting testing on Win/Mac/Linux, DOM panel, HTTPi, JSON > Viewer and perhaps some extensions I am maintaining). This way is slow since > it also includes analyses/changes of the existing CSS (e.g. the Options tool > has hardcoded icon URL in definitions.js and so, having CSS variable for the > image-url isn't possible atm, etc.) Note that I hacked up the "Very very simple" firebug theme patch in 2 hours (I wrote my own CSS, didn't copy paste anything). I'm pretty much sure copy-pasting things back in wouldn't require much more time. Here's a plan I'm suggesting: 1) Land my patch now (or with some minor tweaks if really needed) 2) Put the fb theme behind a pref 3) Add back more theme code in other bugs (it eases up review for us to have small chunks of code, which mesns you'll get faster review times) 4) When it happens to be ready, we can remove the pref If this happens not to be ready in time for 48 (which is unlikely), we can always uplift followup patches to DevEdition and simply flip on the pref in 48. This plan should benefit everyone, this way, users will get an awesome theme when the pref will be switched on, you will get faster review times (we won't have to look over and over again at a huge patch), we will have more maintainable code, which benefits users as well, as they will less suffer from future regressions. I'm very confident my patch is close from the wanted result, it simply needs more icon-swaps, more tweaks ith the toolbar/toolbar button code. And those things can happen in a followup (the icon-swaps can probably happen now though). You can request review from both bgrins or me (I promise quick review times as I don't have anything else to review :). > The new patch changes summary: > - It adopts ntim patch (except of the toolbar variables) > - Global theme variables updated > - Some tool specific variables introduced. > - A lot of CSS rules removed > - All images are now in themes/images/firebug Please make sure to match our image names, this way we get logical association between our images and firebug inages. > - All firebug specific CSS files are in themes/firebug/ (all CSS rules are > nicely splitted into small files like: netmonitor.css, debugger.css, > layout-view.css, etc.) This is probably my biggest concern about this patch. First, it introduces some platform specific files, which are hard to maintain (and we usually have a lot of doubt while cleaning theme code on whether these rules are really needed, I've experienced that while trying to clean up the light/dark theme code). Secondly, having seperate files for firebug makes it difficult to maintain, simply because those files are separate from our theme files, which means they can easily get missed by other developers, which can lower the Firebug theme experience in the future. If you have firebug specific CSS to add, paste into our current files (devtools/client/themes/tool.css) if it does very specific changes to a tool, if it does some major changes (toolbar buttons, tabs,...) you can just dump it into firebug-theme.css. Another thing with this patch: it seems to make some padding/margin changes in some very specific places (animation inspector for example), please avoid tHis kind of layout changes, since they are hard to maintain if we do theme changes on our side. If these layout changes are really needed, please add a comment and describe why, this way we can remove the CSS later while cleaning up if it reveals to no longer be relevant. > So, I am thinking about the following: > > 1) Pick plan #2 > 2) Continue clean up (mainly remove obsolete CSS). > 3) Adopting the code: the Toolbox-tabs and Toolbar-buttons CSS is the > hardest. It would be great if someone could help to adopt it. I can provide > screenshots, summary of features and location of the related code. > 4) Concentrate on individual panels and testing on Win/Mac/Linux and > extensions. My concern with plan 2 is landing lots of CSS. It means you'll get slower review times (lots of code for us to review at once, and lots of comments to address for you), and therefore, it would probably take as long as my plan. The other problem with reviewing big patches is that we can easily miss some review details at the first review round which we find only later at the second review round, which means you'll have to go through many rounds of review. If you give us small chunks to review, we'll probably catch all issues at once, and you'll have to address review comments only once.

Glad that the new patch is adopting theme variables and the light theme. I think the two approaches are getting closer to each other after those changes, especially if you can address the comments in the second half of Comment 21 starting with "This is probably my biggest concern about this patch". I agree that generally any firebug overrides should go into each tool's CSS file instead of a separate file so it's as close to the rest of the code as possible (easier to keep up to date and less likely to break). Maybe in the process of copying this code from external files into the theme files we could also evaluate if it's needed for a specific look in Firebug or if we could remove it. Does this make sense to you, or is there a reason to prefer separate files? I'm going to be unavailable until Wednesday, so please send feedback/review requests to Tim in the meantime.

And if possible to split into two parts (the 'base' part with variables, image files, shared theme css file) and then a second patch that does tool specific overrides that could make reviews easier

(In reply to Brian Grinstead [:bgrins] from comment #22) > Glad that the new patch is adopting theme variables and the light theme. I > think the two approaches are getting closer to each other after those > changes, especially if you can address the comments in the second half of > Comment 21 starting with "This is probably my biggest concern about this > patch". > > I agree that generally any firebug overrides should go into each tool's CSS > file instead of a separate file so it's as close to the rest of the code as > possible (easier to keep up to date and less likely to break). Maybe in the > process of copying this code from external files into the theme files we > could also evaluate if it's needed for a specific look in Firebug or if we > could remove it. Does this make sense to you, or is there a reason to > prefer separate files? Agree, putting new CSS into existing files is better. - platform specific files removed - separate files for firebug also removed (see attached patches) (In reply to Brian Grinstead [:bgrins] from comment #23) > And if possible to split into two parts (the 'base' part with variables, > image files, shared theme css file) and then a second patch that does tool > specific overrides that could make reviews easier Also done. There are three patches attached: 1) bug1244054-theme-firebug.patch: basic implementation of the Firebug theme, new option in the Options panel and basic CSS (toobars, toolbox) 2) bug1244054-theme-images.patch: all images 3) bug1244054-theme-tools.patch: tools specific CSS (in each tool's CSS file) (In reply to Tim Nguyen [:ntim] from comment #21) > I'm suggesting: > 1) Land my patch now (or with some minor tweaks if really needed) I was building my patches on top of yours with one exception, I couldn't use changes to the toolbox + toolbar since there were broken details. Anyway, if you know how to simplify the CSS for toolbox + toolbar (see my changes in toolbox.css and firebug-theme.css) than help-patch is welcome, I can also provide screenshots of the details that are broken. --- Tested on Win. Honza

Comment on attachment 8738591 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8738591 [details] [diff] [review]: ----------------------------------------------------------------- Tim, do you have a chance to take a look at this also? ::: devtools/client/themes/common.css @@ +9,5 @@ > } > > :root[platform="mac"] { > --monospace-font-family: Menlo, monospace; > + --proportional-font-family: Menlo, monospace; This is called --proportional-font-family but it's still monospace. I guess it depends on when the devtools-proportional class is going to be used, but I'd expect it to always actually be a proportional font. @@ +14,5 @@ > } > > :root[platform="win"] { > --monospace-font-family: Consolas, monospace; > + --proportional-font-family: Consolas, monospace; Ditto ::: devtools/client/themes/toolbars.css @@ +162,5 @@ > .devtools-toolbarbutton[label][standalone] { > min-height: 2em; > } > > .theme-dark .devtools-menulist, Should .theme-dark be removed from these selectors? This is changing style for light theme on the 'reload' button in Shader Editor, for example.

Comment on attachment 8738593 [details] [diff] [review] bug1244054-theme-images.patch Review of attachment 8738593 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding the svg reviews to Tim

Comment on attachment 8738595 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8738595 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to submit multiple reviews for these patches as I have comments in the hope that it'll be quicker to get feedback to you instead of doing one big review that might take a long time to finish. Let me know if it's easier for you to get them all in one comment. Tim, if you have time to help with these reviews it's appreciated but feel free to clear if you don't ::: devtools/client/themes/animationinspector.css @@ +175,5 @@ > + background-image: var(--rewind-image); > +} > + > +.theme-firebug #rewind-timeline::before { > + background-image: var(--play-image); I'd prefer to see a separate svg added for rewind (or a mode added inside the current play image). Would be easier to use elsewhere if needed, and less CSS to maintain ::: devtools/client/themes/layout.css @@ +144,5 @@ > #layout-content { > + background-color: var(--layout-content-color); > +} > + > +.theme-firebug #layout-margins { I'm not an expert on the firebug UI, but when I comment out all of the .theme-firebug bits in this file it looks alright. Keeping some font changes but discarding the outline / border changes and it feels firebug-like to me .theme-firebug #layout-main, .theme-firebug #layout-header { font-family: var(--proportional-font-family); color: black; } Are the outline / border changes important to keep?

Also, I hope you are aware of the devtools.loader.hotreload pref - it makes these CSS changes a lot easier to work with since you don't need to rebuild / reload.

Comment on attachment 8738591 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8738591 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/common.css @@ +52,5 @@ > +/* Firebug theme */ > + > +.theme-firebug .devtools-autocomplete-popup { > + background-color: var(--theme-body-background); > + border: 1px solid #BEBEBE; Where does this color come from ? Also, can't we just set the border-color ? Seems like the popup should already have a 1px border (I might be wrong though). @@ +54,5 @@ > +.theme-firebug .devtools-autocomplete-popup { > + background-color: var(--theme-body-background); > + border: 1px solid #BEBEBE; > + border-radius: 5px; > + font-size: 13px; Please use font-sizes relative to a CSS variable (you can use em units) ::: devtools/client/themes/firebug-theme.css @@ +16,5 @@ > + > +.theme-firebug .devtools-textinput, > +.theme-firebug .devtools-searchinput { > + -moz-appearance: none; > + margin: 0 3px; I see absolutely no difference when removing the 2 properties above locally. Can you remove them ? @@ +17,5 @@ > +.theme-firebug .devtools-textinput, > +.theme-firebug .devtools-searchinput { > + -moz-appearance: none; > + margin: 0 3px; > + border: 1px solid #aabccf; I wonder why you don't use #aabccf directly as splitter color, it looks better than #aaa (in my personal opinion). If you do that, you also won't need to set the border color on the toolbar and the text inputs, which saves you code (less code = more maintanable). @@ +18,5 @@ > +.theme-firebug .devtools-searchinput { > + -moz-appearance: none; > + margin: 0 3px; > + border: 1px solid #aabccf; > + border-radius: 2px; The border-radius is already 2px by default (except on OSX which has a bigger one, but that's also on the firebug theme) So this can be removed. @@ +24,5 @@ > +} > + > +.theme-firebug .devtools-searchinput { > + margin-top: 1px; > + margin-bottom: 1px; I see absolutely no change when removing this locally. @@ +25,5 @@ > + > +.theme-firebug .devtools-searchinput { > + margin-top: 1px; > + margin-bottom: 1px; > + padding: 0; Removing this locally has no effect. @@ +30,5 @@ > + -moz-padding-start: 22px; > + -moz-padding-end: 12px; > + background-position: 4px center; > + background-size: 14px 14px; > + background-repeat: no-repeat; I wish the image could simply be 16x16 pixels (or 14x14 but fitted inside a 16x16 SVG), so we can simply do an image swap, and avoid all this code duplication :/ @@ +32,5 @@ > + background-position: 4px center; > + background-size: 14px 14px; > + background-repeat: no-repeat; > + font-size: inherit; > + border-color: #aabccf; The border-color property has the same value as above, which is redundant. @@ +48,5 @@ > +.theme-firebug .devtools-no-search-result { > + border-color: #eb5368 !important; > +} > + > +/* CodeMirror Color Syntax */ Can you file a bug to make this more simple amd more maintainable? @@ +88,5 @@ > + > +/* Variables View */ > + > +.theme-firebug .variables-view-variable > .title > .value { > + color: #18191A; Where does this color come from, why does it need to be set ? @@ +92,5 @@ > + color: #18191A; > +} > + > +.theme-firebug .devtools-toolbar > .name { > + cursor: pointer; This kind of change (cursor change) doesn't seem appropriate for a theme to do, but I would be fine with making it globally. @@ +106,5 @@ > + > +.theme-firebug .devtools-tabbar, > +.theme-firebug .devtools-sidebar-tabs tabs { > + border-bottom-width: 0; > + box-shadow: none; There should be no box-shadow to remove. This isn't needed. @@ +111,2 @@ > font-family: Lucida Grande, Tahoma, sans-serif; > + -moz-appearance: none; -moz-appearance: none should already be set elsewhere, you don't need to set it here. @@ +111,4 @@ > font-family: Lucida Grande, Tahoma, sans-serif; > + -moz-appearance: none; > + background-color: rgb(219, 234, 249); > + background-image: linear-gradient(rgba(253, 253, 253, 0.2), rgba(253, 253, 253, 0)); Same comment about the toolbar background variable @@ +121,5 @@ > + Use !important to override even the rule in webconsole.css that uses > + ID in the selector. */ > +.theme-firebug .devtools-tabbar, > +.theme-firebug .devtools-sidebar-tabs tabs { > + border-bottom: 1px solid rgb(170, 188, 207) !important; This is confusing, you set border-bottom-width: 0; above on exactly the same selector, and you override it again here ? Also, can you merge this block with the block above since they share the same selector ? @@ +126,5 @@ > +} > + > +.theme-firebug .devtools-sidebar-tabs tabs { > + background-color: rgb(219, 234, 249) !important; > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); The background is already set above on the same selector, can you remove this block ? @@ +132,5 @@ > + > +.theme-firebug .devtools-tab, > +.theme-firebug .devtools-sidebar-tabs tab { > + margin: 3px 0 -1px 0; > + padding: 0 2px 1px 0; Please use an RTL aware value: padding: 0 0 1px 0; padding-inline-end: 2px; @@ +135,5 @@ > + margin: 3px 0 -1px 0; > + padding: 0 2px 1px 0; > + border: 1px solid transparent !important; > + border-radius: 4px 4px 0 0; > + font-size: 11px; nit: please use relative font sizes @@ +137,5 @@ > + border: 1px solid transparent !important; > + border-radius: 4px 4px 0 0; > + font-size: 11px; > + font-weight: bold; > + color: #565656; Please use variables @@ +138,5 @@ > + border-radius: 4px 4px 0 0; > + font-size: 11px; > + font-weight: bold; > + color: #565656; > + box-shadow: none; What box-shadow are you removing ? There is no box-shadow in light theme on tabs (even the selected state) @@ +155,5 @@ > + border-bottom: 1px solid transparent; > +} > + > +.theme-firebug .devtools-tab[selected], > +.theme-firebug #toolbox-sidetabs .devtools-tab[selected], #toolbox-sidetabs doesn't exist (according to dxr), what does this target ? @@ +162,5 @@ > + border: 1px solid rgb(170, 188, 207) !important; > + border-bottom-width: 0 !important; > + padding-bottom: 2px; > + color: inherit; > + box-shadow: none; box-shadow: none isn't needed. @@ +181,5 @@ > +} > + > +.theme-firebug #panelSideBox .devtools-tab:first-child, > +.theme-firebug .devtools-sidebar-tabs tab:first-child { > + margin-left: 5px; nit: this needs an RTL aware property @@ +189,5 @@ > + > +.theme-firebug .theme-toolbar, > +.theme-firebug toolbar, > +.theme-firebug .devtools-toolbar { > + -moz-appearance: none; -moz-appearance: none isn't needed. @@ +192,5 @@ > +.theme-firebug .devtools-toolbar { > + -moz-appearance: none; > + border-bottom: 1px solid rgb(170, 188, 207) !important; > + background-color: rgb(219, 234, 249) !important; > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); :( Can't we simply use the solution I've used in the Very simple theme patch? Setting the var(--theme-toolbar-background) variable to linear-gradient(#F7FBFE, #E3EEFA) is so much easier than all this, and you get the same result. @@ +193,5 @@ > + -moz-appearance: none; > + border-bottom: 1px solid rgb(170, 188, 207) !important; > + background-color: rgb(219, 234, 249) !important; > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); > + padding-right: 4px; nit: this needs an RTL aware replacement @@ +215,5 @@ > + height: 28px !important; > +} > + > +.theme-firebug .theme-toolbar .label { > + font-family: Lucida Grande,Tahoma,sans-serif; Please use inherit, or the proportional-font-family variable. Also where is this rule taking effect ? @@ +223,5 @@ > + > +.theme-firebug .theme-toolbar button, > +.theme-firebug .devtools-button, > +.theme-firebug toolbarbutton { > + background-color: transparent; This should already be set in toolbars.css @@ +224,5 @@ > +.theme-firebug .theme-toolbar button, > +.theme-firebug .devtools-button, > +.theme-firebug toolbarbutton { > + background-color: transparent; > + -moz-appearance: none; Same thing @@ +226,5 @@ > +.theme-firebug toolbarbutton { > + background-color: transparent; > + -moz-appearance: none; > + margin: 0; > + border: 0; Same thing for border: 0; @@ +228,5 @@ > + -moz-appearance: none; > + margin: 0; > + border: 0; > + border-radius: 2px; > + color: #141414; Can you use a variable for this color ? @@ +231,5 @@ > + border-radius: 2px; > + color: #141414; > + width: auto; > + line-height: 12px; > + font-size: 12px; nit: please use relative font sizes or a variable @@ +233,5 @@ > + width: auto; > + line-height: 12px; > + font-size: 12px; > + /* The icon is absolutely positioned in the button using ::before */ > + position: relative; This should already be set in toolbars.css @@ +238,5 @@ > +} > + > +.theme-firebug .theme-toolbar button, > +.theme-firebug .devtools-button { > + border-width: 1px !important; You're setting border: 0; above and then you're setting border-width: 1px here, this is confusing. @@ +246,5 @@ > + > +/* We might want to change this if toolbar buttons have icons, > +but for now do not allocate an extra space for images */ > +.theme-firebug toolbarbutton > image { > + -moz-margin-end: 0; I don't think .toolbarbutton-icon has a margin set anywhere ? ::: devtools/client/themes/light-theme.css @@ +173,5 @@ > .devtools-sidebar-tabs tabs, > .devtools-sidebar-alltabs, > .cm-s-mozilla .CodeMirror-dialog { /* General toolbar styling */ > color: var(--theme-body-color); > + background: var(--theme-toolbar-background); This change was done so you could use a linear-gradient as variable for theme-toolbar-background. ::: devtools/client/themes/splitters.css @@ +47,5 @@ > } > + > +/* Firebug theme */ > + > +.theme-firebug .devtools-side-splitter { This doesn't apply to horizontal splitters, is that wanted ? Also, this causes some inconsistencies, for example the perf tool only has a 1px splitter, not sure if that's wanted. ::: devtools/client/themes/toolbars.css @@ +43,5 @@ > + --magnifying-glass-image-2x: url(images/firebug/magnifying-glass.svg); > + --command-pick-image: url(images/firebug/command-pick.svg); > + --tool-options-image: url(images/firebug/tool-options.svg); > + --close-button-image: url(chrome://devtools/skin/images/firebug/close.svg); > + --icon-filter: none; Are you sure the fb theme should have no icon filter ? That means all icons that will be added in the future will be white, and therefore invisible. This doesn't seem like a maintainable solution. What you could do is maintain a blacklist of icons that don't need a filter inside firebug-theme.css (like I did in the simple patch). Ideally, I'd want the icons to be black, and apply `filter: invert(1)` on the dark theme. This solution should get rid of the blacklist I'm suggesting, but this is unlikely to happen the wanted timeframe, as not all our icons are SVG yet. @@ +186,5 @@ > } > > +/* Invert toolbox button icons in Firebug theme. */ > +.theme-firebug #toolbox-buttons toolbarbutton image { > + filter: invert(1); Using --icon-filter: invert(1) means you won't have to do this. @@ +998,5 @@ > + > +.theme-firebug #toolbox-tab-options::before { > + content: url(chrome://devtools/skin/images/firebug/tool-options.svg); > + display: block; > + margin: 4px 7px 0; This is a very hacky way to change the toolbox icons. Can you file a bug to investigate a better way to change it ? ::: devtools/client/themes/variables.css @@ +21,5 @@ > > --theme-tab-toolbar-background: #fcfcfc; > --theme-toolbar-background: #fcfcfc; > --theme-selection-background: #4c9ed9; > + --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.3); Why are you changing the light theme ? ::: devtools/client/themes/widgets.css @@ +336,5 @@ > > +/* Firebug theme support fro breadcrumbs widget*/ > + > +.theme-firebug .breadcrumbs-widget-item { > + -moz-appearance: none; I'm fairly sure -moz-appearance: none; isn't needed (we already do this above to reset the default styles) @@ +337,5 @@ > +/* Firebug theme support fro breadcrumbs widget*/ > + > +.theme-firebug .breadcrumbs-widget-item { > + -moz-appearance: none; > + margin: 0 1px 0 10px; Is this RTL aware ? If not, it needs to use margin-inline-start/end @@ +343,5 @@ > + color: #141414; > + border-radius: 2px; > + min-width: 0; > + min-height: 0; > + width: auto; Why is `width:auto` needed ? I don't think this overrides anything, just sets width to its default value. @@ +346,5 @@ > + min-height: 0; > + width: auto; > + padding: 0; > + line-height: 18px; > + font-size: 12px; Where do these sizes (18px and 12px) come from ? Please use em units or existing CSS variables, so we can easily change those sizes in the future. @@ +352,5 @@ > + > +.theme-firebug .breadcrumbs-widget-item:hover { > + border-color: rgba(0, 0, 0, 0.2); > + background: transparent linear-gradient(rgba(255, 255, 255, 0.4), > + rgba(255, 255, 255, 0.2)) no-repeat; nit: Setting the background-image should be enough, also, our code doesn't use spaces within color functions: background-image: linear-gradient(rgba(255,255,255,0.4), rgba(255,255,255,0.2)); @@ +354,5 @@ > + border-color: rgba(0, 0, 0, 0.2); > + background: transparent linear-gradient(rgba(255, 255, 255, 0.4), > + rgba(255, 255, 255, 0.2)) no-repeat; > + box-shadow: 1px 1px 1px rgba(255, 255, 255, 0.6) inset, 0 0 1px rgba(255, 255, 255, 0.6) inset, > + 0 0 2px rgba(0, 0, 0, 0.05); nit: please align these properly @@ +359,5 @@ > +} > + > +.theme-firebug .breadcrumbs-widget-item > .button-box { > + padding-left: 0px; > + padding-right: 0px; nit: you can omit the unit since the value is 0 @@ +384,5 @@ > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-classes { > + background: none; > + background-color: transparent; > + font-weight: bold; > + color: black; nit: color: inherit; (So the color actually gets changed when the variable does) @@ +427,5 @@ > + background: url(chrome://global/skin/arrow/arrow-lft-sharp.gif) no-repeat 0 center / 5px 9px; > +} > + > +.theme-firebug .breadcrumbs-widget-container .scrollbutton-down > .toolbarbutton-icon { > + background: url(chrome://global/skin/arrow/arrow-lft-sharp.gif) no-repeat 0 center / 5px 9px; So this is the kind of image usage I don't like (because they're harder to maintain). Please avoid setting background-position/size... It would be nice if image swaps are actually only image swaps @@ +858,5 @@ > +.theme-firebug .variables-view-delete:not([disabled="true"]), > +.theme-firebug .variables-view-delete:hover:not([disabled="true"]), > +.theme-firebug .variables-view-scope toolbarbutton:hover:active:not([disabled="true"]):not([type="menu-button"]) { > + width: 16px; > + margin-right: 3px !important; Is this rule *really* needed ? If so, please comment why. This whole block just seems mysterious to me. Also margin-right isn't RTL aware

Comment on attachment 8738593 [details] [diff] [review] bug1244054-theme-images.patch Review of attachment 8738593 [details] [diff] [review]: ----------------------------------------------------------------- These files should follow the SVG Guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines To do this quickly, what you can do is: - running them through SVGO `npm install -g svgo` and `svgo --pretty -f devtools/client/themes/firebug/` - Add a license header to those files and make sure they have 2 space indentation (a bash command can probably do those things) After those 2 steps, the SVGs should perfectly follow the guidelines.

Comment on attachment 8738593 [details] [diff] [review] bug1244054-theme-images.patch Review of attachment 8738593 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/images/firebug/filters.svg @@ +41,5 @@ > + <feFuncB type="table" tableValues=".1 0"/> > + </feComponentTransfer> > + </filter> > + > + <!-- Web Audio Gradients --> Are you using these ?

Comment on attachment 8738595 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8738595 [details] [diff] [review]: ----------------------------------------------------------------- I didn't take a deep look at rules.css. ::: devtools/client/themes/animationinspector.css @@ +177,5 @@ > + > +.theme-firebug #rewind-timeline::before { > + background-image: var(--play-image); > + transform: scaleX(-1); > + filter: FlipH; I agree with bgrins about adding a new SVG for this. Also `FlipH` isn't a valid filter value. ::: devtools/client/themes/computed.css @@ +97,5 @@ > } > > +.theme-firebug .property-view, > +.theme-firebug .property-content { > + background-color: inherit; This background-color property has no effect. @@ +99,5 @@ > +.theme-firebug .property-view, > +.theme-firebug .property-content { > + background-color: inherit; > + font-family: var(--proportional-font-family); > + font-size: 11px; Isn't this default for the firebug theme ? (it was set on :root) ::: devtools/client/themes/debugger.css @@ +71,5 @@ > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox:not([checked="true"]) ~ * { > + opacity: 0.5; > +} > + > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox[checked="true"] > .checkbox-check { This block seems useless, since it's just duplicating the rule above. ::: devtools/client/themes/layout.css @@ +19,5 @@ > +} > + > +.theme-firebug { > + --layout-margins-color: #edff64; > + --layout-borders-color: #D1C57A; This doesn't match the color used in the highlighter for borders, which is confusing. @@ +144,5 @@ > #layout-content { > + background-color: var(--layout-content-color); > +} > + > +.theme-firebug #layout-margins { I agree with Brian that there isn't a major difference when removing the .theme-firebug bits here. ::: devtools/client/themes/markup.css @@ +311,5 @@ > + color: red; > +} > + > +.theme-firebug .theme-comment { > + color: rgb(0, 100, 0); Can't you just set the --theme-comment variable to green ? @@ +315,5 @@ > + color: rgb(0, 100, 0); > +} > + > +.theme-firebug .theme-selected { > + background-color: #3399ff; This rule seems redundant. This already uses background-color: var(--theme-selection-background) by default, which corresponds to #3399ff for the firebug theme. @@ +328,5 @@ > +.theme-firebug .theme-selected ~ .editor .theme-fg-color6, > +.theme-firebug .theme-selected ~ .editor .theme-fg-color7, > +.theme-firebug .theme-selected ~ .editor .open, > +.theme-firebug .theme-selected ~ .editor .close { > + color: #fff; This rule seems redundant. This already uses color: var(--theme-selection-color) by default, which corresponds to white. ::: devtools/client/themes/netmonitor.css @@ +26,5 @@ > + --timing-wait-color: rgba(95, 136, 176, 0.8); /* blue grey */ > + --timing-receive-color: rgba(44, 187, 15, 0.8); /* green */ > +} > + > +:root.theme-firebug { I've unapplied this block with the devtools, and the difference is almost unnoticeable. Are you sure this is needed ? @@ +144,5 @@ > +/* Firebug theme support for Network panel header */ > + > +.theme-firebug .requests-menu-header { > + cursor: pointer; > + -moz-user-select: none; These kind of changes shouldn't be firebug-theme specific, they should be removed or applied globally @@ +145,5 @@ > + > +.theme-firebug .requests-menu-header { > + cursor: pointer; > + -moz-user-select: none; > + /*border-bottom: 1px solid rgba(0, 0, 0, 0.2);*/ nit: please remove this comment @@ +153,5 @@ > + rgba(255, 255, 255, 0.05), > + rgba(0, 0, 0, 0.05)), > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%) repeat-x #C8D2DC; nit: please align this properly. Also, repeat-x isn't needed. @@ +154,5 @@ > + rgba(0, 0, 0, 0.05)), > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%) repeat-x #C8D2DC; > + color: #000000; color: #000000 doesn't seem needed. @@ +155,5 @@ > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%) repeat-x #C8D2DC; > + color: #000000; > + white-space: nowrap; This isn't needed. @@ +159,5 @@ > + white-space: nowrap; > +} > + > +.theme-firebug .requests-menu-header-button { > + transition: none; Why transition: none ? As far as I know there is no transition being set. @@ +172,5 @@ > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); > +} > + > +.theme-firebug .requests-menu-header-button:not(:active)[sorted=ascending] { > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); We can see both the default theme image and the firebug image at the same time. See how the image is set earlier in this file to override it properly. @@ +181,5 @@ > + rgba(0, 0, 0, 0.1), > + transparent), > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%); nit: please fix the alignment @@ +319,5 @@ > transform: rotate(45deg); > } > > +/* Firebug theme support */ > + All the rules below with .theme-firebug box.requests-menu-status have no effect. The correct selector is .requests-menu-status-icon. Also, I don't think this needs specific tweaks on the color code, it feels alright to me (and firebug-y enough). @@ +512,5 @@ > + line-height: 15px; > + border-bottom: 1px solid #EFEFEF; > + margin: 0; > + padding: 0; > +} I honestly don't see a difference when commenting out the rule above. @@ +519,5 @@ > +.theme-firebug .side-menu-widget-item.selected .requests-menu-icon-and-file, > +.theme-firebug .side-menu-widget-item.selected .requests-menu-status-and-method, > +.theme-firebug .side-menu-widget-item.selected { > + background: #3399ff; > + color: white; This block isn't needed. The existing code already sets this. @@ +537,5 @@ > +} > + > +/* Method Column */ > +.theme-firebug .requests-menu-subitem.requests-menu-status-and-method { > + color: rgb(128, 128, 128); This isn't applying, because the selector is obsolete @@ +566,5 @@ > } > > +/* Headers separators are toolbars, but they have different height > + in Firebug theme */ > +.theme-firebug #headers-tabpanel.tabpanel-content .title.devtools-toolbar { It would be nice if the selector had `variables-view` in it, so it's more explicit. Also, why is it only for the Headers tab ? It feels odd that the other netmonitor sidetabs have bigger "Header separators". @@ +567,5 @@ > > +/* Headers separators are toolbars, but they have different height > + in Firebug theme */ > +.theme-firebug #headers-tabpanel.tabpanel-content .title.devtools-toolbar { > + display: -moz-box; display: -moz-box; has no effect, please remove it. @@ +666,5 @@ > } > > +.theme-firebug #timings-tabpanel .requests-menu-timings-total { > + color: #808080; > + min-width: 16px; I don't see a visible change when removing min-width: 16px; ::: devtools/client/themes/rules.css @@ +520,5 @@ > + background-position:0 0; > +} > + > +.theme-firebug .ruleview-rule .theme-checkbox:not([checked]){ > + filter: url(chrome://devtools/skin/images/firebug/filters.svg#grayscale); filter: grayscale(1) should just work as well.

Comment on attachment 8738593 [details] [diff] [review] bug1244054-theme-images.patch Review of attachment 8738593 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/images/firebug/filters.svg @@ +1,4 @@ > +<?xml version="1.0" encoding="UTF-8"?> > +<!-- See license.txt for terms of usage --> > +<svg xmlns="http://www.w3.org/2000/svg"> > + <filter id="darken"> Taking a look at these filters again, it seems like the filters you're adding can be achieved using CSS filters. You can for example use filter: brightness(90%) to darken an image, or brightness(110%) to brighten it.

Comment on attachment 8738591 [details] [diff] [review] bug1244054-theme-firebug.patch Clearing out reviews - I'll take another look after this round of feedback is addressed

Comment on attachment 8738595 [details] [diff] [review] bug1244054-theme-tools.patch Clearing out reviews - I'll take another look after this round of feedback is addressed

(In reply to Brian Grinstead [:bgrins] from comment #28) > This is called --proportional-font-family but it's still monospace. I guess > it depends on when the devtools-proportional class is going to be used, but > I'd expect it to always actually be a proportional font. OKay, I changed to var to proprtional font (it's only used in firebug-theme atm) --proportional-font-family: Lucida Grande, Tahoma, sans-serif; (for both mentioned cases) > ::: devtools/client/themes/toolbars.css > @@ +162,5 @@ > > .devtools-toolbarbutton[label][standalone] { > > min-height: 2em; > > } > > > > .theme-dark .devtools-menulist, > > Should .theme-dark be removed from these selectors? This is changing style > for light theme on the 'reload' button in Shader Editor, for example. --toolbar-button-border-color is set to rgba(0, 0, 0, .4); for dark-theme and rgba(170, 170, 170, .5); for the light-theme so, there is no difference from how it was before. (In reply to Brian Grinstead [:bgrins] from comment #30) > Comment on attachment 8738595 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8738595 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm going to submit multiple reviews for these patches as I have comments in > the hope that it'll be quicker to get feedback to you instead of doing one > big review that might take a long time to finish. Let me know if it's > easier for you to get them all in one comment. This way is fine. > Tim, if you have time to help with these reviews it's appreciated but feel > free to clear if you don't > > ::: devtools/client/themes/animationinspector.css > @@ +175,5 @@ > > + background-image: var(--rewind-image); > > +} > > + > > +.theme-firebug #rewind-timeline::before { > > + background-image: var(--play-image); > > I'd prefer to see a separate svg added for rewind (or a mode added inside > the current play image). Would be easier to use elsewhere if needed, and > less CSS to maintain Done > > ::: devtools/client/themes/layout.css > @@ +144,5 @@ > > #layout-content { > > + background-color: var(--layout-content-color); > > +} > > + > > +.theme-firebug #layout-margins { > > I'm not an expert on the firebug UI, but when I comment out all of the > .theme-firebug bits in this file it looks alright. > > Keeping some font changes but discarding the outline / border changes and it > feels firebug-like to me > > .theme-firebug #layout-main, > .theme-firebug #layout-header { > font-family: var(--proportional-font-family); > color: black; > } > > Are the outline / border changes important to keep? OK, removed the outlines, but kept the borders (not big deal). (In reply to Brian Grinstead [:bgrins] from comment #31) > Also, I hope you are aware of the devtools.loader.hotreload pref - it makes > these CSS changes a lot easier to work with since you don't need to rebuild > / reload. Yes, I am using it on Mac, but it doesn't work on Windows (no symlinks) :( This would be so great to have it fixed! Honza

(In reply to Tim Nguyen [:ntim] from comment #32) > Comment on attachment 8738591 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8738591 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/common.css > @@ +52,5 @@ > > +/* Firebug theme */ > > + > > +.theme-firebug .devtools-autocomplete-popup { > > + background-color: var(--theme-body-background); > > + border: 1px solid #BEBEBE; > > Where does this color come from ? Also, can't we just set the border-color ? > Seems like the popup should already have a 1px border (I might be wrong > though). Fixed, I used a var for the color and used only border color. > > @@ +54,5 @@ > > +.theme-firebug .devtools-autocomplete-popup { > > + background-color: var(--theme-body-background); > > + border: 1px solid #BEBEBE; > > + border-radius: 5px; > > + font-size: 13px; > > Please use font-sizes relative to a CSS variable (you can use em units) Done > > ::: devtools/client/themes/firebug-theme.css > @@ +16,5 @@ > > + > > +.theme-firebug .devtools-textinput, > > +.theme-firebug .devtools-searchinput { > > + -moz-appearance: none; > > + margin: 0 3px; > > I see absolutely no difference when removing the 2 properties above locally. > Can you remove them ? Done > > @@ +17,5 @@ > > +.theme-firebug .devtools-textinput, > > +.theme-firebug .devtools-searchinput { > > + -moz-appearance: none; > > + margin: 0 3px; > > + border: 1px solid #aabccf; > > I wonder why you don't use #aabccf directly as splitter color, it looks > better than #aaa (in my personal opinion). If you do that, you also won't > need to set the border color on the toolbar and the text inputs, which saves > you code (less code = more maintanable). Good idea, done. > > @@ +18,5 @@ > > +.theme-firebug .devtools-searchinput { > > + -moz-appearance: none; > > + margin: 0 3px; > > + border: 1px solid #aabccf; > > + border-radius: 2px; > > The border-radius is already 2px by default (except on OSX which has a > bigger one, but that's also on the firebug theme) > So this can be removed. Done > > @@ +24,5 @@ > > +} > > + > > +.theme-firebug .devtools-searchinput { > > + margin-top: 1px; > > + margin-bottom: 1px; > > I see absolutely no change when removing this locally. Removed > > @@ +25,5 @@ > > + > > +.theme-firebug .devtools-searchinput { > > + margin-top: 1px; > > + margin-bottom: 1px; > > + padding: 0; > > Removing this locally has no effect. Removed > > @@ +30,5 @@ > > + -moz-padding-start: 22px; > > + -moz-padding-end: 12px; > > + background-position: 4px center; > > + background-size: 14px 14px; > > + background-repeat: no-repeat; > > I wish the image could simply be 16x16 pixels (or 14x14 but fitted inside a > 16x16 SVG), so we can simply do an image swap, and avoid all this code > duplication :/ I used only image swap and using the default positioning and size looks good. (the rest of the rules removed) > > @@ +32,5 @@ > > + background-position: 4px center; > > + background-size: 14px 14px; > > + background-repeat: no-repeat; > > + font-size: inherit; > > + border-color: #aabccf; > > The border-color property has the same value as above, which is redundant. > > @@ +48,5 @@ > > +.theme-firebug .devtools-no-search-result { > > + border-color: #eb5368 !important; > > +} > > + > > +/* CodeMirror Color Syntax */ > > Can you file a bug to make this more simple amd more maintainable? As noted above, the addition search input customization is removed. > > @@ +88,5 @@ > > + > > +/* Variables View */ > > + > > +.theme-firebug .variables-view-variable > .title > .value { > > + color: #18191A; > > Where does this color come from, why does it need to be set ? It's body color, I am using var(--theme-body-color) now. It's for variables view values. E.g. Headers side panel (in Net panel) is displaying HTTP headers values (.value) and the values should be black not red. And actually even the names (.name) should be black, fixed. Also there shouldn't be border at the bottom of .variables-view-variable, also fixed (in widgets.css) Also bold for .name is not necessary and removed (already set in widgets.css) > > @@ +92,5 @@ > > + color: #18191A; > > +} > > + > > +.theme-firebug .devtools-toolbar > .name { > > + cursor: pointer; > > This kind of change (cursor change) doesn't seem appropriate for a theme to > do, but I would be fine with making it globally. Removed > > @@ +106,5 @@ > > + > > +.theme-firebug .devtools-tabbar, > > +.theme-firebug .devtools-sidebar-tabs tabs { > > + border-bottom-width: 0; > > + box-shadow: none; > > There should be no box-shadow to remove. This isn't needed. Removed > > @@ +111,2 @@ > > font-family: Lucida Grande, Tahoma, sans-serif; > > + -moz-appearance: none; > > -moz-appearance: none should already be set elsewhere, you don't need to set > it here. Removed > > @@ +111,4 @@ > > font-family: Lucida Grande, Tahoma, sans-serif; > > + -moz-appearance: none; > > + background-color: rgb(219, 234, 249); > > + background-image: linear-gradient(rgba(253, 253, 253, 0.2), rgba(253, 253, 253, 0)); > > Same comment about the toolbar background variable Done > > @@ +121,5 @@ > > + Use !important to override even the rule in webconsole.css that uses > > + ID in the selector. */ > > +.theme-firebug .devtools-tabbar, > > +.theme-firebug .devtools-sidebar-tabs tabs { > > + border-bottom: 1px solid rgb(170, 188, 207) !important; > > This is confusing, you set border-bottom-width: 0; above on exactly the same > selector, and you override it again here ? > Also, can you merge this block with the block above since they share the > same selector ? Both done > > @@ +126,5 @@ > > +} > > + > > +.theme-firebug .devtools-sidebar-tabs tabs { > > + background-color: rgb(219, 234, 249) !important; > > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); > > The background is already set above on the same selector, can you remove > this block ? No, the background for side panel tabs is different (it matches the background of panel's toolbar since so, it's visually one strip). > > @@ +132,5 @@ > > + > > +.theme-firebug .devtools-tab, > > +.theme-firebug .devtools-sidebar-tabs tab { > > + margin: 3px 0 -1px 0; > > + padding: 0 2px 1px 0; > > Please use an RTL aware value: > padding: 0 0 1px 0; > padding-inline-end: 2px; > > @@ +135,5 @@ > > + margin: 3px 0 -1px 0; > > + padding: 0 2px 1px 0; > > + border: 1px solid transparent !important; > > + border-radius: 4px 4px 0 0; > > + font-size: 11px; > > nit: please use relative font sizes Removed instead. > > @@ +137,5 @@ > > + border: 1px solid transparent !important; > > + border-radius: 4px 4px 0 0; > > + font-size: 11px; > > + font-weight: bold; > > + color: #565656; > > Please use variables Done > > @@ +138,5 @@ > > + border-radius: 4px 4px 0 0; > > + font-size: 11px; > > + font-weight: bold; > > + color: #565656; > > + box-shadow: none; > > What box-shadow are you removing ? There is no box-shadow in light theme on > tabs (even the selected state) Done > > @@ +155,5 @@ > > + border-bottom: 1px solid transparent; > > +} > > + > > +.theme-firebug .devtools-tab[selected], > > +.theme-firebug #toolbox-sidetabs .devtools-tab[selected], > > #toolbox-sidetabs doesn't exist (according to dxr), what does this target ? Removed > > @@ +162,5 @@ > > + border: 1px solid rgb(170, 188, 207) !important; > > + border-bottom-width: 0 !important; > > + padding-bottom: 2px; > > + color: inherit; > > + box-shadow: none; > > box-shadow: none isn't needed. Removed > > @@ +181,5 @@ > > +} > > + > > +.theme-firebug #panelSideBox .devtools-tab:first-child, > > +.theme-firebug .devtools-sidebar-tabs tab:first-child { > > + margin-left: 5px; > > nit: this needs an RTL aware property Replaced by -moz-margin-start > > @@ +189,5 @@ > > + > > +.theme-firebug .theme-toolbar, > > +.theme-firebug toolbar, > > +.theme-firebug .devtools-toolbar { > > + -moz-appearance: none; > > -moz-appearance: none isn't needed. Removed > > @@ +192,5 @@ > > +.theme-firebug .devtools-toolbar { > > + -moz-appearance: none; > > + border-bottom: 1px solid rgb(170, 188, 207) !important; > > + background-color: rgb(219, 234, 249) !important; > > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); > > :( Can't we simply use the solution I've used in the Very simple theme > patch? Setting the var(--theme-toolbar-background) variable to > linear-gradient(#F7FBFE, #E3EEFA) is so much easier than all this, and you > get the same result. Agree, but let's do it in a follow up (also note that there are two different backgrounds in Firebug theme and other themes have just one). > > @@ +193,5 @@ > > + -moz-appearance: none; > > + border-bottom: 1px solid rgb(170, 188, 207) !important; > > + background-color: rgb(219, 234, 249) !important; > > + background-image: linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2)); > > + padding-right: 4px; > > nit: this needs an RTL aware replacement Replaced by -moz-padding-end > > @@ +215,5 @@ > > + height: 28px !important; > > +} > > + > > +.theme-firebug .theme-toolbar .label { > > + font-family: Lucida Grande,Tahoma,sans-serif; > > Please use inherit, or the proportional-font-family variable. Also where is > this rule taking effect ? The variable used. E.g. in the Inspector side panels toolbars (btw. they are using both theme-toolbar as well as devtools-toolbar, which doesn't sounds right). > > @@ +223,5 @@ > > + > > +.theme-firebug .theme-toolbar button, > > +.theme-firebug .devtools-button, > > +.theme-firebug toolbarbutton { > > + background-color: transparent; > > This should already be set in toolbars.css Removed > > @@ +224,5 @@ > > +.theme-firebug .theme-toolbar button, > > +.theme-firebug .devtools-button, > > +.theme-firebug toolbarbutton { > > + background-color: transparent; > > + -moz-appearance: none; > > Same thing Removed > > @@ +226,5 @@ > > +.theme-firebug toolbarbutton { > > + background-color: transparent; > > + -moz-appearance: none; > > + margin: 0; > > + border: 0; > > Same thing for border: 0; Removed > > @@ +228,5 @@ > > + -moz-appearance: none; > > + margin: 0; > > + border: 0; > > + border-radius: 2px; > > + color: #141414; > > Can you use a variable for this color ? Done var(--theme-body-color) > > @@ +231,5 @@ > > + border-radius: 2px; > > + color: #141414; > > + width: auto; > > + line-height: 12px; > > + font-size: 12px; > > nit: please use relative font sizes or a variable Done var(--theme-toolbar-font-size) > > @@ +233,5 @@ > > + width: auto; > > + line-height: 12px; > > + font-size: 12px; > > + /* The icon is absolutely positioned in the button using ::before */ > > + position: relative; > > This should already be set in toolbars.css Removed > > @@ +238,5 @@ > > +} > > + > > +.theme-firebug .theme-toolbar button, > > +.theme-firebug .devtools-button { > > + border-width: 1px !important; > > You're setting border: 0; above and then you're setting border-width: 1px > here, this is confusing. Fixed > > @@ +246,5 @@ > > + > > +/* We might want to change this if toolbar buttons have icons, > > +but for now do not allocate an extra space for images */ > > +.theme-firebug toolbarbutton > image { > > + -moz-margin-end: 0; > > I don't think .toolbarbutton-icon has a margin set anywhere ? Removed > > ::: devtools/client/themes/light-theme.css > @@ +173,5 @@ > > .devtools-sidebar-tabs tabs, > > .devtools-sidebar-alltabs, > > .cm-s-mozilla .CodeMirror-dialog { /* General toolbar styling */ > > color: var(--theme-body-color); > > + background: var(--theme-toolbar-background); > > This change was done so you could use a linear-gradient as variable for > theme-toolbar-background. Yes, I'll adopt the firebug-theme in a follow up (as mentioned above) > > ::: devtools/client/themes/splitters.css > @@ +47,5 @@ > > } > > + > > +/* Firebug theme */ > > + > > +.theme-firebug .devtools-side-splitter { > > This doesn't apply to horizontal splitters, is that wanted ? Yes, this is what we have now. There are more troubles we should solve, like e.g. the main Toolbox splitter lives outside of toolbox.xul and can't be themed since there is not theme class name. I am putting this on list of follow ups. > Also, this causes some inconsistencies, for example the perf tool only has a > 1px splitter, not sure if that's wanted. The Performance panel is not even using a splitter it's just two vboxes next to each other. > > ::: devtools/client/themes/toolbars.css > @@ +43,5 @@ > > + --magnifying-glass-image-2x: url(images/firebug/magnifying-glass.svg); > > + --command-pick-image: url(images/firebug/command-pick.svg); > > + --tool-options-image: url(images/firebug/tool-options.svg); > > + --close-button-image: url(chrome://devtools/skin/images/firebug/close.svg); > > + --icon-filter: none; > > Are you sure the fb theme should have no icon filter ? That means all icons > that will be added in the future will be white, and therefore invisible. > This doesn't seem like a maintainable solution. > What you could do is maintain a blacklist of icons that don't need a filter > inside firebug-theme.css (like I did in the simple patch). Sounds good to me, I am putting this on the list of follow ups. What exactly do you mean by a blacklist? > > Ideally, I'd want the icons to be black, and apply `filter: invert(1)` on > the dark theme. This solution should get rid of the blacklist I'm > suggesting, but this is unlikely to happen the wanted timeframe, as not all > our icons are SVG yet. Yes > > @@ +186,5 @@ > > } > > > > +/* Invert toolbox button icons in Firebug theme. */ > > +.theme-firebug #toolbox-buttons toolbarbutton image { > > + filter: invert(1); > > Using --icon-filter: invert(1) means you won't have to do this. > > @@ +998,5 @@ > > + > > +.theme-firebug #toolbox-tab-options::before { > > + content: url(chrome://devtools/skin/images/firebug/tool-options.svg); > > + display: block; > > + margin: 4px 7px 0; > > This is a very hacky way to change the toolbox icons. Can you file a bug to > investigate a better way to change it ? Yes, I'll do it. > > ::: devtools/client/themes/variables.css > @@ +21,5 @@ > > > > --theme-tab-toolbar-background: #fcfcfc; > > --theme-toolbar-background: #fcfcfc; > > --theme-selection-background: #4c9ed9; > > + --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.3); > > Why are you changing the light theme ? Reverted > > ::: devtools/client/themes/widgets.css > @@ +336,5 @@ > > > > +/* Firebug theme support fro breadcrumbs widget*/ > > + > > +.theme-firebug .breadcrumbs-widget-item { > > + -moz-appearance: none; > > I'm fairly sure -moz-appearance: none; isn't needed (we already do this > above to reset the default styles) Removed > > @@ +337,5 @@ > > +/* Firebug theme support fro breadcrumbs widget*/ > > + > > +.theme-firebug .breadcrumbs-widget-item { > > + -moz-appearance: none; > > + margin: 0 1px 0 10px; > > Is this RTL aware ? If not, it needs to use margin-inline-start/end Done (but RTL needs testing, I guess another follow up). > > @@ +343,5 @@ > > + color: #141414; > > + border-radius: 2px; > > + min-width: 0; > > + min-height: 0; > > + width: auto; > > Why is `width:auto` needed ? I don't think this overrides anything, just > sets width to its default value. Removed > > @@ +346,5 @@ > > + min-height: 0; > > + width: auto; > > + padding: 0; > > + line-height: 18px; > > + font-size: 12px; > > Where do these sizes (18px and 12px) come from ? Please use em units or > existing CSS variables, so we can easily change those sizes in the future. Fixed > > @@ +352,5 @@ > > + > > +.theme-firebug .breadcrumbs-widget-item:hover { > > + border-color: rgba(0, 0, 0, 0.2); > > + background: transparent linear-gradient(rgba(255, 255, 255, 0.4), > > + rgba(255, 255, 255, 0.2)) no-repeat; > > nit: Setting the background-image should be enough, Done > also, our code doesn't > use spaces within color functions: Can we changed that? It would be consistent with similar rule in JS (which is there for a reason, better readability) and as far as I can see CSS isn't even consistent atm. > > background-image: linear-gradient(rgba(255,255,255,0.4), > rgba(255,255,255,0.2)); > > @@ +354,5 @@ > > + border-color: rgba(0, 0, 0, 0.2); > > + background: transparent linear-gradient(rgba(255, 255, 255, 0.4), > > + rgba(255, 255, 255, 0.2)) no-repeat; > > + box-shadow: 1px 1px 1px rgba(255, 255, 255, 0.6) inset, 0 0 1px rgba(255, 255, 255, 0.6) inset, > > + 0 0 2px rgba(0, 0, 0, 0.05); > > nit: please align these properly Done (I think) > > @@ +359,5 @@ > > +} > > + > > +.theme-firebug .breadcrumbs-widget-item > .button-box { > > + padding-left: 0px; > > + padding-right: 0px; > > nit: you can omit the unit since the value is 0 Done > > @@ +384,5 @@ > > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-classes { > > + background: none; > > + background-color: transparent; > > + font-weight: bold; > > + color: black; > > nit: color: inherit; (So the color actually gets changed when the variable > does) Done > > @@ +427,5 @@ > > + background: url(chrome://global/skin/arrow/arrow-lft-sharp.gif) no-repeat 0 center / 5px 9px; > > +} > > + > > +.theme-firebug .breadcrumbs-widget-container .scrollbutton-down > .toolbarbutton-icon { > > + background: url(chrome://global/skin/arrow/arrow-lft-sharp.gif) no-repeat 0 center / 5px 9px; > > So this is the kind of image usage I don't like (because they're harder to > maintain). Please avoid setting background-position/size... It would be nice > if image swaps are actually only image swaps Done > > @@ +858,5 @@ > > +.theme-firebug .variables-view-delete:not([disabled="true"]), > > +.theme-firebug .variables-view-delete:hover:not([disabled="true"]), > > +.theme-firebug .variables-view-scope toolbarbutton:hover:active:not([disabled="true"]):not([type="menu-button"]) { > > + width: 16px; > > + margin-right: 3px !important; > > Is this rule *really* needed ? If so, please comment why. > This whole block just seems mysterious to me. > > Also margin-right isn't RTL aware This is broken, removed and on my TODO list. Thanks Tim! Honza

(In reply to Tim Nguyen [:ntim] from comment #33) > Comment on attachment 8738593 [details] [diff] [review] > bug1244054-theme-images.patch > > Review of attachment 8738593 [details] [diff] [review]: > ----------------------------------------------------------------- > > These files should follow the SVG Guidelines: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > SVG_Guidelines > > To do this quickly, what you can do is: > - running them through SVGO `npm install -g svgo` and `svgo --pretty -f > devtools/client/themes/firebug/` > - Add a license header to those files and make sure they have 2 space > indentation (a bash command can probably do those things) > > After those 2 steps, the SVGs should perfectly follow the guidelines. Done

(In reply to Tim Nguyen [:ntim] from comment #34) > Comment on attachment 8738593 [details] [diff] [review] > bug1244054-theme-images.patch > > Review of attachment 8738593 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/images/firebug/filters.svg > @@ +41,5 @@ > > + <feFuncB type="table" tableValues=".1 0"/> > > + </feComponentTransfer> > > + </filter> > > + > > + <!-- Web Audio Gradients --> > > Are you using these ? Removed and used alternatives (see below) Honza

(In reply to Tim Nguyen [:ntim] from comment #35) > Comment on attachment 8738595 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8738595 [details] [diff] [review]: > ----------------------------------------------------------------- > > I didn't take a deep look at rules.css. > > ::: devtools/client/themes/animationinspector.css > @@ +177,5 @@ > > + > > +.theme-firebug #rewind-timeline::before { > > + background-image: var(--play-image); > > + transform: scaleX(-1); > > + filter: FlipH; > > I agree with bgrins about adding a new SVG for this. > Also `FlipH` isn't a valid filter value. Yes, already done. > > ::: devtools/client/themes/computed.css > @@ +97,5 @@ > > } > > > > +.theme-firebug .property-view, > > +.theme-firebug .property-content { > > + background-color: inherit; > > This background-color property has no effect. Removed > > @@ +99,5 @@ > > +.theme-firebug .property-view, > > +.theme-firebug .property-content { > > + background-color: inherit; > > + font-family: var(--proportional-font-family); > > + font-size: 11px; > > Isn't this default for the firebug theme ? (it was set on :root) Yes, removed > > ::: devtools/client/themes/debugger.css > @@ +71,5 @@ > > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox:not([checked="true"]) ~ * { > > + opacity: 0.5; > > +} > > + > > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox[checked="true"] > .checkbox-check { > > This block seems useless, since it's just duplicating the rule above. True, removed. > > ::: devtools/client/themes/layout.css > @@ +19,5 @@ > > +} > > + > > +.theme-firebug { > > + --layout-margins-color: #edff64; > > + --layout-borders-color: #D1C57A; > > This doesn't match the color used in the highlighter for borders, which is > confusing. Yes, I remember that I was unable to customize the highlighter colors? Is that possible? > > @@ +144,5 @@ > > #layout-content { > > + background-color: var(--layout-content-color); > > +} > > + > > +.theme-firebug #layout-margins { > > I agree with Brian that there isn't a major difference when removing the > .theme-firebug bits here. Yes, I removed some of them already. > > ::: devtools/client/themes/markup.css > @@ +311,5 @@ > > + color: red; > > +} > > + > > +.theme-firebug .theme-comment { > > + color: rgb(0, 100, 0); > > Can't you just set the --theme-comment variable to green ? Yes, nice. > > @@ +315,5 @@ > > + color: rgb(0, 100, 0); > > +} > > + > > +.theme-firebug .theme-selected { > > + background-color: #3399ff; > > This rule seems redundant. > > This already uses background-color: var(--theme-selection-background) by > default, which corresponds to #3399ff for the firebug theme. Removed > > @@ +328,5 @@ > > +.theme-firebug .theme-selected ~ .editor .theme-fg-color6, > > +.theme-firebug .theme-selected ~ .editor .theme-fg-color7, > > +.theme-firebug .theme-selected ~ .editor .open, > > +.theme-firebug .theme-selected ~ .editor .close { > > + color: #fff; > > This rule seems redundant. > > This already uses color: var(--theme-selection-color) by default, which > corresponds to white. Removed > > ::: devtools/client/themes/netmonitor.css > @@ +26,5 @@ > > + --timing-wait-color: rgba(95, 136, 176, 0.8); /* blue grey */ > > + --timing-receive-color: rgba(44, 187, 15, 0.8); /* green */ > > +} > > + > > +:root.theme-firebug { > > I've unapplied this block with the devtools, and the difference is almost > unnoticeable. Are you sure this is needed ? Yes agree maybe a detail but, I would like to keep it (there was a lot of discussions to make the waterfall colors the same across browser tools in the past, it's not like they are all the same now, but changing it feels like step back) > > @@ +144,5 @@ > > +/* Firebug theme support for Network panel header */ > > + > > +.theme-firebug .requests-menu-header { > > + cursor: pointer; > > + -moz-user-select: none; > > These kind of changes shouldn't be firebug-theme specific, they should be > removed or applied globally Global now > > @@ +145,5 @@ > > + > > +.theme-firebug .requests-menu-header { > > + cursor: pointer; > > + -moz-user-select: none; > > + /*border-bottom: 1px solid rgba(0, 0, 0, 0.2);*/ > > nit: please remove this comment Removed > > @@ +153,5 @@ > > + rgba(255, 255, 255, 0.05), > > + rgba(0, 0, 0, 0.05)), > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%) repeat-x #C8D2DC; > > nit: please align this properly. Also, repeat-x isn't needed. Done > > @@ +154,5 @@ > > + rgba(0, 0, 0, 0.05)), > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%) repeat-x #C8D2DC; > > + color: #000000; > > color: #000000 doesn't seem needed. Removed > > @@ +155,5 @@ > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%) repeat-x #C8D2DC; > > + color: #000000; > > + white-space: nowrap; > > This isn't needed. Removed > > @@ +159,5 @@ > > + white-space: nowrap; > > +} > > + > > +.theme-firebug .requests-menu-header-button { > > + transition: none; > > Why transition: none ? As far as I know there is no transition being set. Removed > > @@ +172,5 @@ > > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); > > +} > > + > > +.theme-firebug .requests-menu-header-button:not(:active)[sorted=ascending] { > > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); > > We can see both the default theme image and the firebug image at the same > time. See how the image is set earlier in this file to override it properly. Done > > @@ +181,5 @@ > > + rgba(0, 0, 0, 0.1), > > + transparent), > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%); > > nit: please fix the alignment Done > > @@ +319,5 @@ > > transform: rotate(45deg); > > } > > > > +/* Firebug theme support */ > > + > > All the rules below with .theme-firebug box.requests-menu-status have no > effect. The correct selector is .requests-menu-status-icon. > > Also, I don't think this needs specific tweaks on the color code, it feels > alright to me (and firebug-y enough). Agree, removed > > @@ +512,5 @@ > > + line-height: 15px; > > + border-bottom: 1px solid #EFEFEF; > > + margin: 0; > > + padding: 0; > > +} > > I honestly don't see a difference when commenting out the rule above. Agree removed. > > @@ +519,5 @@ > > +.theme-firebug .side-menu-widget-item.selected .requests-menu-icon-and-file, > > +.theme-firebug .side-menu-widget-item.selected .requests-menu-status-and-method, > > +.theme-firebug .side-menu-widget-item.selected { > > + background: #3399ff; > > + color: white; > > This block isn't needed. The existing code already sets this. Removed > > @@ +537,5 @@ > > +} > > + > > +/* Method Column */ > > +.theme-firebug .requests-menu-subitem.requests-menu-status-and-method { > > + color: rgb(128, 128, 128); > > This isn't applying, because the selector is obsolete Fixed > > @@ +566,5 @@ > > } > > > > +/* Headers separators are toolbars, but they have different height > > + in Firebug theme */ > > +.theme-firebug #headers-tabpanel.tabpanel-content .title.devtools-toolbar { > > It would be nice if the selector had `variables-view` in it, so it's more > explicit. There is no variables-view, I used variables-view-container > Also, why is it only for the Headers tab ? It feels odd that the other > netmonitor sidetabs have bigger "Header separators". True, fixed. > > @@ +567,5 @@ > > > > +/* Headers separators are toolbars, but they have different height > > + in Firebug theme */ > > +.theme-firebug #headers-tabpanel.tabpanel-content .title.devtools-toolbar { > > + display: -moz-box; > > display: -moz-box; has no effect, please remove it. Removed > > @@ +666,5 @@ > > } > > > > +.theme-firebug #timings-tabpanel .requests-menu-timings-total { > > + color: #808080; > > + min-width: 16px; > > I don't see a visible change when removing min-width: 16px; Removed > > ::: devtools/client/themes/rules.css > @@ +520,5 @@ > > + background-position:0 0; > > +} > > + > > +.theme-firebug .ruleview-rule .theme-checkbox:not([checked]){ > > + filter: url(chrome://devtools/skin/images/firebug/filters.svg#grayscale); > > filter: grayscale(1) should just work as well. Yes, nice, fixed. Honza

(In reply to Tim Nguyen [:ntim] from comment #36) > Comment on attachment 8738593 [details] [diff] [review] > bug1244054-theme-images.patch > > Review of attachment 8738593 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/images/firebug/filters.svg > @@ +1,4 @@ > > +<?xml version="1.0" encoding="UTF-8"?> > > +<!-- See license.txt for terms of usage --> > > +<svg xmlns="http://www.w3.org/2000/svg"> > > + <filter id="darken"> > > Taking a look at these filters again, it seems like the filters you're > adding can be achieved using CSS filters. You can for example use filter: > brightness(90%) to darken an image, or brightness(110%) to brighten it. Done Patches will follow ... Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #39) > > ::: devtools/client/themes/toolbars.css > > @@ +162,5 @@ > > > .devtools-toolbarbutton[label][standalone] { > > > min-height: 2em; > > > } > > > > > > .theme-dark .devtools-menulist, > > > > Should .theme-dark be removed from these selectors? This is changing style > > for light theme on the 'reload' button in Shader Editor, for example. > --toolbar-button-border-color is set to rgba(0, 0, 0, .4); for dark-theme > and > rgba(170, 170, 170, .5); for the light-theme so, there is no difference > from how it was before. Maybe it's not that rule then, but there's definitely a difference with the patches applied and not in the light theme for the 'reload' button in the shader editor (I wish I had taken screenshots but you should be able to see it with the patches applied).

(In reply to Brian Grinstead [:bgrins] from comment #45) > (In reply to Jan Honza Odvarko [:Honza] from comment #39) > > > ::: devtools/client/themes/toolbars.css > > > @@ +162,5 @@ > > > > .devtools-toolbarbutton[label][standalone] { > > > > min-height: 2em; > > > > } > > > > > > > > .theme-dark .devtools-menulist, > > > > > > Should .theme-dark be removed from these selectors? This is changing style > > > for light theme on the 'reload' button in Shader Editor, for example. > > --toolbar-button-border-color is set to rgba(0, 0, 0, .4); for dark-theme > > and > > rgba(170, 170, 170, .5); for the light-theme so, there is no difference > > from how it was before. > > Maybe it's not that rule then, but there's definitely a difference with the > patches applied and not in the light theme for the 'reload' button in the > shader editor (I wish I had taken screenshots but you should be able to see > it with the patches applied). Ah yes, you are right, I see it now. Fixed Honza

Comment on attachment 8739449 [details] [diff] [review] bug1244054-theme-images.patch Forwarding again to Tim

Comment on attachment 8739450 [details] [diff] [review] bug1244054-theme-tools.patch Forwarding again to Tim

Comment on attachment 8739462 [details] [diff] [review] bug1244054-theme-firebug.patch Forwarding again to Tim

Comment on attachment 8739449 [details] [diff] [review] bug1244054-theme-images.patch Review of attachment 8739449 [details] [diff] [review]: ----------------------------------------------------------------- The images have 4 spaces indentation instead of 2 spaces, but I'd be happy to postpone until SVGO has an indent option. I'll likely review the other patches tomorrow.

Comment on attachment 8739462 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8739462 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/common.css @@ +14,1 @@ > } The light and dark themes use font: message-box; to get the system font. Please don't change our font usage. @@ +14,5 @@ > } > > :root[platform="win"] { > --monospace-font-family: Consolas, monospace; > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; Same comment for this. @@ +23,2 @@ > --monospace-font-family: monospace; > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; Same here. @@ +27,5 @@ > .devtools-monospace { > font-family: var(--monospace-font-family); > } > > +.devtools-proportional { I'm not a big fan of introducing this new class. The proportional font should simply be the default font (set on the root), whereas the monospace font should be set on the relevant elements. @@ +120,5 @@ > + padding: 1px; > +} > + > +.devtools-autocomplete-popup.firebug-theme { > + background: var(--theme-body-background); Is this selector even correct ? `.devtools-autocomplete-popup.firebug-theme` Should be .theme-firebug right ? ::: devtools/client/themes/firebug-theme.css @@ +19,5 @@ > +/* CodeMirror Color Syntax */ > + > +.theme-firebug .cm-keyword {color: BlueViolet; font-weight: bold;} > +.theme-firebug .cm-atom {color: #219;} > +.theme-firebug .cm-number {color: #164;} We should find a better way to set these colors. Can you file a follow up bug ? @@ +82,5 @@ > +} > + > +.theme-firebug .devtools-tab, > +.theme-firebug .devtools-sidebar-tabs tab { > + margin: 3px 0 -1px 0; You should make it clear why there is a negative margin. /* Add a negative bottom margin to overlap the border */ @@ +128,5 @@ > +} > + > +.theme-firebug #panelSideBox .devtools-tab:first-child, > +.theme-firebug .devtools-sidebar-tabs tab:first-child { > + -moz-margin-start: 5px; nit: we prefer the standard form better: margin-inline-start (same for other -moz-*-end/start properties) @@ +145,5 @@ > + > +/* The vbox for panel content also uses theme-toolbar class from some reason > + but it shouldn't have the padding as defined above, so fix it here */ > +.theme-firebug #toolbox-deck > .toolbox-panel.theme-toolbar { > + padding-right: 0; nit: padding-inline-end @@ +178,5 @@ > +.theme-firebug toolbarbutton { > + margin: 0; > + border-radius: 2px; > + color: var(--theme-body-color); > + width: auto; width: auto isn't needed. ::: devtools/client/themes/splitters.css @@ +11,5 @@ > :root[devtoolstheme="dark"] { > --devtools-splitter-color: #42484f; > } > > +:root[devtoolstheme="firebug"] { Can you combine this with the rule above ? @@ +50,5 @@ > + > +.theme-firebug .devtools-side-splitter { > + background-color: rgb(231, 241, 251) !important; > + border: none; > + min-width: 5px; This splitter causes problems because it's bigger than the default one. You can now resize hidden sidebars (collapsed inspector sidebar for example), which is definitively not wanted. ::: devtools/client/themes/toolbars.css @@ +43,5 @@ > + --magnifying-glass-image-2x: url(images/firebug/filter.svg); > + --command-pick-image: url(images/firebug/command-pick.svg); > + --tool-options-image: url(images/firebug/tool-options.svg); > + --close-button-image: url(chrome://devtools/skin/images/firebug/close.svg); > + --icon-filter: none; I know you want to do this change in a followup, but this is causing some usability issues in the Firebug theme itself. This causes most icons to have very low contrast (especially the take snapshot icon in the memory tool). Feel free to fix this in a follow up though. @@ +900,5 @@ > } > > +/* Display execution pointer in the Debugger tab to indicate > + that the debugger is paused. */ > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { I would like a global solution for all tools that have a highlighted icon state (memory, performance and debugger). Maybe you could unhide the icon if the tab is highlighted. @@ +902,5 @@ > +/* Display execution pointer in the Debugger tab to indicate > + that the debugger is paused. */ > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { > + background-color: rgba(89, 178, 234, .2); > + background-image: url(chrome://devtools/skin/images/firebug/debugger-execution-pointer.svg); Can you rename this image to tool-debugger-paused.svg ? This way, we can have logical association between our default images and the firebug images. @@ +904,5 @@ > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { > + background-color: rgba(89, 178, 234, .2); > + background-image: url(chrome://devtools/skin/images/firebug/debugger-execution-pointer.svg); > + background-repeat: no-repeat; > + padding-left: 13px !important; This is not RTL aware. @@ +988,5 @@ > #toolbox-tab-options > image { > margin: 0 8px; > } > > +/* Firebug theme support for the Option (panel) tab */ Can you move this code below the firebug theme tab code in firebug-theme.css ? @@ +991,5 @@ > > +/* Firebug theme support for the Option (panel) tab */ > + > +.theme-firebug #toolbox-tab-options { > + margin-right: 4px; nit: margin-inline-end. @@ +1005,5 @@ > +.theme-firebug #toolbox-tab-options:not([selected]):hover::before { > + filter: brightness(80%); > +} > + > +.theme-firebug #toolbox-tab-options[selected] image { This block has no effect since the image is hidden ::: devtools/client/themes/widgets.css @@ +367,5 @@ > + margin: 0; > +} > + > +.theme-firebug .breadcrumbs-widget-item:not(:first-child)::before { > + content: url(chrome://devtools/skin/images/firebug/statusPathSeparator.svg); Can you name this image to breadcrumbs-divider.svg ? It's for easier logical association with our default theme images. @@ +382,5 @@ > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-tag, > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-pseudo-classes, > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-classes { > + background: none; > + background-color: transparent; background-color: transparent; is redundant after background: none; @@ +400,5 @@ > +} > + > +.theme-firebug .breadcrumbs-widget-container .breadcrumbs-widget-item, > +.theme-firebug .breadcrumbs-widget-container .breadcrumbs-widget-item-tag { > + cursor: pointer; Can you remove this cursor change ?

Comment on attachment 8739450 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8739450 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/debugger.css @@ +73,5 @@ > +} > + > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox { > + padding-right: 0; > + margin-right: 0; nit: padding-inline-end: 0; margin-inline-end: 0; @@ +96,5 @@ > > +.theme-firebug #debugger-controls > .devtools-toolbarbutton, > +.theme-firebug #sources-controls > .devtools-toolbarbutton { > + min-width: 24px; > +} I don't see why this is specific to the debugger buttons @@ +693,5 @@ > + > +/* Firebug theme customization of source group title */ > +.theme-firebug #sources-pane .side-menu-widget-group-title { > + border-bottom: none; > + margin: 0; What margin are you overriding ? @@ +698,5 @@ > + padding: 2px 4px; > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; > + font-size: inherit; > + font-weight: bold; > + color: black; nit : use inherit or CSS var @@ +699,5 @@ > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; > + font-size: inherit; > + font-weight: bold; > + color: black; > + cursor: pointer; cursor: pointer; doesn't make much sense as clicking on this element does nothing @@ +711,5 @@ > + > +/* Firebug theme support for the Callstack Panel */ > + > +.theme-firebug #callstack-list { > + font-family: Lucida Grande, Tahoma, sans-serif; Use CSS variable please @@ +716,5 @@ > + font-size: 11px; > +} > + > +.theme-firebug #callstack-list .dbg-classic-stackframe-title { > + color: blue; I don't like the use of hardcoded colors here and below. Can you use variable equivalents if they exist ? @@ +727,5 @@ > + padding-bottom: 1px; > +} > + > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > + content: "()"; Please don't add content like this, it may cause bugs if we change the dom structure later. @@ +744,5 @@ > + margin: 0 4px 0 4px; > +} > + > +.theme-firebug #callstack-list .side-menu-widget-item.selected { > + background-color: lightgoldenrodyellow; Didn't know that color existed :) Anyway, please use a variable here if possible @@ +753,5 @@ > + border-top: none; > +} > + > +.theme-firebug #callstack-list .side-menu-widget-item :-moz-any(label, hbox) { > + cursor: pointer; Can you remove this cursor change ? ::: devtools/client/themes/layout.css @@ +19,5 @@ > +} > + > +.theme-firebug { > + --layout-margins-color: #edff64; > + --layout-borders-color: #D1C57A; As mentioned in the previous review, this doesn't match the highlighter color. Please undo this change. To answer your question, to change it, you would need to fiddle with the highlighter actor JS file to add a firebug-theme class when appropriate, but I don't think actors have access to the theme code. Patrick knows better than me about this, but this should be done in another bug anyway. Also, I'm surprised you need to introduce new colors for this theme, since we actually changed our colors to match the firebug theme a year ago. @@ +158,5 @@ > + > +.theme-firebug #layout-main { > + border-color: #929292; > + color: black; > + font-size: 12px; Please avoid hardcoded colors and font sizes ::: devtools/client/themes/markup.css @@ +297,5 @@ > + > +/* Firebug Theme */ > + > +.theme-firebug .theme-fg-color3 { > + color: blue; Please avoid hardcoded colors ::: devtools/client/themes/netmonitor.css @@ +163,5 @@ > + background: linear-gradient(rgba(255, 255, 255, 0.05), > + rgba(0, 0, 0, 0.05)), > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%) The radial gradient acts as border, so it should be used on the border-image-source property. (This is how the image border is defined for the default themes, so please be consistent) @@ +180,5 @@ > + background-image: linear-gradient(rgba(0, 0, 0, 0.1), > + transparent), > + radial-gradient(1px 60% at right, > + rgba(0, 0, 0, 0.8) 0%, > + transparent 80%); Same comment here @@ +324,5 @@ > padding-inline-start: 0; > } > > +.theme-firebug .requests-menu-waterfall { > + background-image: none; What does this do ? @@ +485,5 @@ > +} > + > +/* Size Column */ > +.theme-firebug .requests-menu-subitem.requests-menu-size { > + text-align: right; text-align: end; @@ +486,5 @@ > + > +/* Size Column */ > +.theme-firebug .requests-menu-subitem.requests-menu-size { > + text-align: right; > + padding-right: 4px; padding-inline-end @@ +613,5 @@ > transition: transform 0.2s ease-out; > } > > +.theme-firebug #timings-tabpanel .requests-menu-timings-total { > + color: #808080; Variable ? ::: devtools/client/themes/rules.css @@ +68,5 @@ > height: 0px; > } > > +.theme-firebug #pseudo-class-panel[hidden] { > + padding: 0px; Why is this needed ? @@ +174,5 @@ > margin-top: -1px; > } > > +.theme-firebug .theme-gutter.ruleview-header { > + font-family: Lucida Grande, sans-serif; nit: please use the font variable @@ +176,5 @@ > > +.theme-firebug .theme-gutter.ruleview-header { > + font-family: Lucida Grande, sans-serif; > + font-weight: bold; > + color: black; nit: please use color: inherit @@ +178,5 @@ > + font-family: Lucida Grande, sans-serif; > + font-weight: bold; > + color: black; > + border: none; > + margin: 4px 0 4px 0; margin: 4px 0; @@ +182,5 @@ > + margin: 4px 0 4px 0; > + padding: 3px 4px 2px 4px; > + line-height: inherit; > + min-height: 0; > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; I noticed this gradient is used in a lot of places in the FB theme, can you add a firebug-theme specific variable for it ? @@ +245,5 @@ > + color: darkgreen; > +} > + > +.theme-firebug .ruleview-propertycontainer > .ruleview-propertyvalue { > + color: darkblue; Hardcoded colors :( @@ +250,5 @@ > +} > + > +.theme-firebug .ruleview-overridden .ruleview-propertyname, > +.theme-firebug .ruleview-overridden .ruleview-propertyvalue { > + text-decoration: line-through; This is already done by default. @@ +413,5 @@ > margin: -1px -3px -1px -1px; > } > > +.theme-firebug .styleinspector-propertyeditor { > + border: 1px solid #a0a0a0; hardcoded color @@ +469,5 @@ > > +.theme-firebug .ruleview-selector > .ruleview-selector-matched, > +.theme-firebug .ruleview-selector > .ruleview-selector-separator, > +.theme-firebug .ruleview-selector > .ruleview-selector-unmatched { > + color: black; Use a variable or inherit here @@ +516,5 @@ > +.theme-firebug .ruleview-rule .theme-checkbox { > + background-repeat: no-repeat; > + background-size: 12px 12px; > + background-image: url(chrome://devtools/skin/images/firebug/disable.svg); > + background-position:0 0; nit: space after : ::: devtools/client/themes/webconsole.css @@ +59,5 @@ > align-self: flex-start; > } > > +.theme-firebug .message > .icon { > + margin:0 6px 0 0; nit: spaces after : @@ +64,5 @@ > +} > + > +.theme-firebug .message[severity="error"] { > + color:#FF0000; > + background-color:#FFEBEB; same @@ +68,5 @@ > + background-color:#FFEBEB; > +} > + > +.theme-firebug .message[severity="warn"] { > + background-color:#FFFFC8; same here @@ +126,5 @@ > text-decoration: none; > white-space: nowrap; > } > > +/* More space for the location data in Firebug theme */ Why ? Can you elaborate this comment ? @@ +397,5 @@ > } > > +.theme-firebug .jsterm-input-container { > + border-top: 1px solid #CCCCCC; > + border-top-style: solid; Setting border-top-color should be enough. @@ +622,5 @@ > :root[platform="win"] .hud-filter-box { > width: 200px; > } > + > +/* Firebug theme support for console.table() */ All this stuff should be done in widgets.css so it affects the storage inspector table as well (and therefor, shouldn't be specific to .consoletable)

Issues I've found: - Pseudo class panel doesn't work in firebug theme (you can't toggle it) - The pane-expand icon is invisible in dark and light themes (probably a variable you forgot to set)

> Can we changed that? It would be consistent with similar rule in JS (which is there for a reason, better readability) and as far as I can see CSS isn't even consistent atm. Sure. > Agree, but let's do it in a follow up (also note that there are two different backgrounds in Firebug theme and other themes have just one). Ok, I'm fine with a followup. > Sounds good to me, I am putting this on the list of follow ups. What exactly do you mean by a blacklist? Blacklist means a list of selectors that should not be affected by the invert filter.

(In reply to Tim Nguyen [:ntim] from comment #54) > Comment on attachment 8739462 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8739462 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/common.css > @@ +14,1 @@ > > } > > The light and dark themes use font: message-box; to get the system font. > Please don't change our font usage. > > @@ +14,5 @@ > > } > > > > :root[platform="win"] { > > --monospace-font-family: Consolas, monospace; > > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; > > Same comment for this. Ok, removed > > @@ +23,2 @@ > > --monospace-font-family: monospace; > > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; > > Same here. Removed > > @@ +27,5 @@ > > .devtools-monospace { > > font-family: var(--monospace-font-family); > > } > > > > +.devtools-proportional { > > I'm not a big fan of introducing this new class. The proportional font > should simply be the default font (set on the root), whereas the monospace > font should be set on the relevant elements. Removed I only kept the var --monospace-font-family for firebug theme. I agree that having proportional font the default and use monospace when appropriate sounds good, but firebug sometimes wants to use proportional even if the markup attributes say devtools-monospace and is using the var (above) for that. But perhaps we can improve this (making a note on the todo list) > > @@ +120,5 @@ > > + padding: 1px; > > +} > > + > > +.devtools-autocomplete-popup.firebug-theme { > > + background: var(--theme-body-background); > > Is this selector even correct ? `.devtools-autocomplete-popup.firebug-theme` > > Should be .theme-firebug right ? No, the selector is correct. The autocomplete popup is using a classname that is generated explicitly in autocomplete-popup.js (ctor). > > ::: devtools/client/themes/firebug-theme.css > @@ +19,5 @@ > > +/* CodeMirror Color Syntax */ > > + > > +.theme-firebug .cm-keyword {color: BlueViolet; font-weight: bold;} > > +.theme-firebug .cm-atom {color: #219;} > > +.theme-firebug .cm-number {color: #164;} > > We should find a better way to set these colors. Can you file a follow up > bug ? Yes, I am putting this on the todo list. I am think about filing a meta bug and listing all the things we want to improve. Consequently we can file bugs for any of these things. > > @@ +82,5 @@ > > +} > > + > > +.theme-firebug .devtools-tab, > > +.theme-firebug .devtools-sidebar-tabs tab { > > + margin: 3px 0 -1px 0; > > You should make it clear why there is a negative margin. > /* Add a negative bottom margin to overlap the border */ Done > > @@ +128,5 @@ > > +} > > + > > +.theme-firebug #panelSideBox .devtools-tab:first-child, > > +.theme-firebug .devtools-sidebar-tabs tab:first-child { > > + -moz-margin-start: 5px; > > nit: we prefer the standard form better: margin-inline-start (same for other > -moz-*-end/start properties) Fixed > > @@ +145,5 @@ > > + > > +/* The vbox for panel content also uses theme-toolbar class from some reason > > + but it shouldn't have the padding as defined above, so fix it here */ > > +.theme-firebug #toolbox-deck > .toolbox-panel.theme-toolbar { > > + padding-right: 0; > > nit: padding-inline-end Fixed > > @@ +178,5 @@ > > +.theme-firebug toolbarbutton { > > + margin: 0; > > + border-radius: 2px; > > + color: var(--theme-body-color); > > + width: auto; > > width: auto isn't needed. Removed > > ::: devtools/client/themes/splitters.css > @@ +11,5 @@ > > :root[devtoolstheme="dark"] { > > --devtools-splitter-color: #42484f; > > } > > > > +:root[devtoolstheme="firebug"] { > > Can you combine this with the rule above ? Done > > @@ +50,5 @@ > > + > > +.theme-firebug .devtools-side-splitter { > > + background-color: rgb(231, 241, 251) !important; > > + border: none; > > + min-width: 5px; > > This splitter causes problems because it's bigger than the default one. > > You can now resize hidden sidebars (collapsed inspector sidebar for > example), which is definitively not wanted. I removed it and put an item on the todo list. > ::: devtools/client/themes/toolbars.css > @@ +43,5 @@ > > + --magnifying-glass-image-2x: url(images/firebug/filter.svg); > > + --command-pick-image: url(images/firebug/command-pick.svg); > > + --tool-options-image: url(images/firebug/tool-options.svg); > > + --close-button-image: url(chrome://devtools/skin/images/firebug/close.svg); > > + --icon-filter: none; > > I know you want to do this change in a followup, but this is causing some > usability issues in the Firebug theme itself. This causes most icons to have > very low contrast (especially the take snapshot icon in the memory tool). > Feel free to fix this in a follow up though. Yeah, already on the list > > @@ +900,5 @@ > > } > > > > +/* Display execution pointer in the Debugger tab to indicate > > + that the debugger is paused. */ > > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { > > I would like a global solution for all tools that have a highlighted icon > state (memory, performance and debugger). > > Maybe you could unhide the icon if the tab is highlighted. Another todo created > > @@ +902,5 @@ > > +/* Display execution pointer in the Debugger tab to indicate > > + that the debugger is paused. */ > > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { > > + background-color: rgba(89, 178, 234, .2); > > + background-image: url(chrome://devtools/skin/images/firebug/debugger-execution-pointer.svg); > > Can you rename this image to tool-debugger-paused.svg ? This way, we can > have logical association between our default images and the firebug images. Done > > @@ +904,5 @@ > > +.theme-firebug #toolbox-tab-jsdebugger.devtools-tab:not([selected])[highlighted] { > > + background-color: rgba(89, 178, 234, .2); > > + background-image: url(chrome://devtools/skin/images/firebug/debugger-execution-pointer.svg); > > + background-repeat: no-repeat; > > + padding-left: 13px !important; > > This is not RTL aware. Yeah, the icon is always on the left side of the label. I have this on the list. > > @@ +988,5 @@ > > #toolbox-tab-options > image { > > margin: 0 8px; > > } > > > > +/* Firebug theme support for the Option (panel) tab */ > > Can you move this code below the firebug theme tab code in firebug-theme.css > ? Done > > @@ +991,5 @@ > > > > +/* Firebug theme support for the Option (panel) tab */ > > + > > +.theme-firebug #toolbox-tab-options { > > + margin-right: 4px; > > nit: margin-inline-end. Fixed > > @@ +1005,5 @@ > > +.theme-firebug #toolbox-tab-options:not([selected]):hover::before { > > + filter: brightness(80%); > > +} > > + > > +.theme-firebug #toolbox-tab-options[selected] image { > > This block has no effect since the image is hidden Ah, removed > > ::: devtools/client/themes/widgets.css > @@ +367,5 @@ > > + margin: 0; > > +} > > + > > +.theme-firebug .breadcrumbs-widget-item:not(:first-child)::before { > > + content: url(chrome://devtools/skin/images/firebug/statusPathSeparator.svg); > > Can you name this image to breadcrumbs-divider.svg ? It's for easier logical > association with our default theme images. Done > > @@ +382,5 @@ > > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-tag, > > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-pseudo-classes, > > +.theme-firebug .breadcrumbs-widget-item[checked] .breadcrumbs-widget-item-classes { > > + background: none; > > + background-color: transparent; > > background-color: transparent; is redundant after background: none; Removed > > @@ +400,5 @@ > > +} > > + > > +.theme-firebug .breadcrumbs-widget-container .breadcrumbs-widget-item, > > +.theme-firebug .breadcrumbs-widget-container .breadcrumbs-widget-item-tag { > > + cursor: pointer; > > Can you remove this cursor change ? Alright, removed. Honza

(In reply to Tim Nguyen [:ntim] from comment #55) > Comment on attachment 8739450 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8739450 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/debugger.css > @@ +73,5 @@ > > +} > > + > > +.theme-firebug #sources-pane .dbg-breakpoint-checkbox { > > + padding-right: 0; > > + margin-right: 0; > > nit: > padding-inline-end: 0; > margin-inline-end: 0; Done > > @@ +96,5 @@ > > > > +.theme-firebug #debugger-controls > .devtools-toolbarbutton, > > +.theme-firebug #sources-controls > .devtools-toolbarbutton { > > + min-width: 24px; > > +} > > I don't see why this is specific to the debugger buttons True, changed to .theme-firebug .devtools-toolbarbutton and moved into firebug-theme.css > > @@ +693,5 @@ > > + > > +/* Firebug theme customization of source group title */ > > +.theme-firebug #sources-pane .side-menu-widget-group-title { > > + border-bottom: none; > > + margin: 0; > > What margin are you overriding ? Removed > > @@ +698,5 @@ > > + padding: 2px 4px; > > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; > > + font-size: inherit; > > + font-weight: bold; > > + color: black; > > nit : use inherit or CSS var Removed > > @@ +699,5 @@ > > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; > > + font-size: inherit; > > + font-weight: bold; > > + color: black; > > + cursor: pointer; > > cursor: pointer; doesn't make much sense as clicking on this element does > nothing Removed > > @@ +711,5 @@ > > + > > +/* Firebug theme support for the Callstack Panel */ > > + > > +.theme-firebug #callstack-list { > > + font-family: Lucida Grande, Tahoma, sans-serif; > > Use CSS variable please Done > > @@ +716,5 @@ > > + font-size: 11px; > > +} > > + > > +.theme-firebug #callstack-list .dbg-classic-stackframe-title { > > + color: blue; > > I don't like the use of hardcoded colors here and below. Can you use > variable equivalents if they exist ? Done > > @@ +727,5 @@ > > + padding-bottom: 1px; > > +} > > + > > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > > + content: "()"; > > Please don't add content like this, it may cause bugs if we change the dom > structure later. Any other alternatives how to append the brackets? > > @@ +744,5 @@ > > + margin: 0 4px 0 4px; > > +} > > + > > +.theme-firebug #callstack-list .side-menu-widget-item.selected { > > + background-color: lightgoldenrodyellow; > > Didn't know that color existed :) > Anyway, please use a variable here if possible Done > > @@ +753,5 @@ > > + border-top: none; > > +} > > + > > +.theme-firebug #callstack-list .side-menu-widget-item :-moz-any(label, hbox) { > > + cursor: pointer; > > Can you remove this cursor change ? Done > > ::: devtools/client/themes/layout.css > @@ +19,5 @@ > > +} > > + > > +.theme-firebug { > > + --layout-margins-color: #edff64; > > + --layout-borders-color: #D1C57A; > > As mentioned in the previous review, this doesn't match the highlighter > color. Please undo this change. > > To answer your question, to change it, you would need to fiddle with the > highlighter actor JS file to add a firebug-theme class when appropriate, but > I don't think actors have access to the theme code. Patrick knows better > than me about this, but this should be done in another bug anyway. > > Also, I'm surprised you need to introduce new colors for this theme, since > we actually changed our colors to match the firebug theme a year ago. Alright, removed, I can solve this in bug 1128209 > > @@ +158,5 @@ > > + > > +.theme-firebug #layout-main { > > + border-color: #929292; > > + color: black; > > + font-size: 12px; > > Please avoid hardcoded colors and font sizes Fixed > > ::: devtools/client/themes/markup.css > @@ +297,5 @@ > > + > > +/* Firebug Theme */ > > + > > +.theme-firebug .theme-fg-color3 { > > + color: blue; > > Please avoid hardcoded colors Done > > ::: devtools/client/themes/netmonitor.css > @@ +163,5 @@ > > + background: linear-gradient(rgba(255, 255, 255, 0.05), > > + rgba(0, 0, 0, 0.05)), > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%) > > The radial gradient acts as border, so it should be used on the > border-image-source property. (This is how the image border is defined for > the default themes, so please be consistent) I kept the linear gradient and used original border > > @@ +180,5 @@ > > + background-image: linear-gradient(rgba(0, 0, 0, 0.1), > > + transparent), > > + radial-gradient(1px 60% at right, > > + rgba(0, 0, 0, 0.8) 0%, > > + transparent 80%); > > Same comment here The same > > @@ +324,5 @@ > > padding-inline-start: 0; > > } > > > > +.theme-firebug .requests-menu-waterfall { > > + background-image: none; > > What does this do ? Removed > > @@ +485,5 @@ > > +} > > + > > +/* Size Column */ > > +.theme-firebug .requests-menu-subitem.requests-menu-size { > > + text-align: right; > > text-align: end; Done > > @@ +486,5 @@ > > + > > +/* Size Column */ > > +.theme-firebug .requests-menu-subitem.requests-menu-size { > > + text-align: right; > > + padding-right: 4px; > > padding-inline-end Done > > @@ +613,5 @@ > > transition: transform 0.2s ease-out; > > } > > > > +.theme-firebug #timings-tabpanel .requests-menu-timings-total { > > + color: #808080; > > Variable ? Done > > ::: devtools/client/themes/rules.css > @@ +68,5 @@ > > height: 0px; > > } > > > > +.theme-firebug #pseudo-class-panel[hidden] { > > + padding: 0px; > > Why is this needed ? Removed > > @@ +174,5 @@ > > margin-top: -1px; > > } > > > > +.theme-firebug .theme-gutter.ruleview-header { > > + font-family: Lucida Grande, sans-serif; > > nit: please use the font variable Done > > @@ +176,5 @@ > > > > +.theme-firebug .theme-gutter.ruleview-header { > > + font-family: Lucida Grande, sans-serif; > > + font-weight: bold; > > + color: black; > > nit: please use color: inherit Done > > @@ +178,5 @@ > > + font-family: Lucida Grande, sans-serif; > > + font-weight: bold; > > + color: black; > > + border: none; > > + margin: 4px 0 4px 0; > > margin: 4px 0; done > > @@ +182,5 @@ > > + margin: 4px 0 4px 0; > > + padding: 3px 4px 2px 4px; > > + line-height: inherit; > > + min-height: 0; > > + background: #F0F0F0 linear-gradient(to top, rgba(0, 0, 0, 0.1), transparent) repeat-x; > > I noticed this gradient is used in a lot of places in the FB theme, can you > add a firebug-theme specific variable for it ? Done (debugger panel updated) > > @@ +245,5 @@ > > + color: darkgreen; > > +} > > + > > +.theme-firebug .ruleview-propertycontainer > .ruleview-propertyvalue { > > + color: darkblue; > > Hardcoded colors :( Vars created > > @@ +250,5 @@ > > +} > > + > > +.theme-firebug .ruleview-overridden .ruleview-propertyname, > > +.theme-firebug .ruleview-overridden .ruleview-propertyvalue { > > + text-decoration: line-through; > > This is already done by default. Hm... interesting, if I remove this the line-throug disappears on overridden props. > > @@ +413,5 @@ > > margin: -1px -3px -1px -1px; > > } > > > > +.theme-firebug .styleinspector-propertyeditor { > > + border: 1px solid #a0a0a0; > > hardcoded color Fixed > > @@ +469,5 @@ > > > > +.theme-firebug .ruleview-selector > .ruleview-selector-matched, > > +.theme-firebug .ruleview-selector > .ruleview-selector-separator, > > +.theme-firebug .ruleview-selector > .ruleview-selector-unmatched { > > + color: black; > > Use a variable or inherit here Done > > @@ +516,5 @@ > > +.theme-firebug .ruleview-rule .theme-checkbox { > > + background-repeat: no-repeat; > > + background-size: 12px 12px; > > + background-image: url(chrome://devtools/skin/images/firebug/disable.svg); > > + background-position:0 0; > > nit: space after : Fixed > > ::: devtools/client/themes/webconsole.css > @@ +59,5 @@ > > align-self: flex-start; > > } > > > > +.theme-firebug .message > .icon { > > + margin:0 6px 0 0; > > nit: spaces after : Fixed > > @@ +64,5 @@ > > +} > > + > > +.theme-firebug .message[severity="error"] { > > + color:#FF0000; > > + background-color:#FFEBEB; > > same Fixed > > @@ +68,5 @@ > > + background-color:#FFEBEB; > > +} > > + > > +.theme-firebug .message[severity="warn"] { > > + background-color:#FFFFC8; > > same here Fixed > > @@ +126,5 @@ > > text-decoration: none; > > white-space: nowrap; > > } > > > > +/* More space for the location data in Firebug theme */ > > Why ? Can you elaborate this comment ? /* More space for the location data for location URL */ > > @@ +397,5 @@ > > } > > > > +.theme-firebug .jsterm-input-container { > > + border-top: 1px solid #CCCCCC; > > + border-top-style: solid; > > Setting border-top-color should be enough. Done > > @@ +622,5 @@ > > :root[platform="win"] .hud-filter-box { > > width: 200px; > > } > > + > > +/* Firebug theme support for console.table() */ > > All this stuff should be done in widgets.css so it affects the storage > inspector table as well (and therefor, shouldn't be specific to > .consoletable) Putting this on the todo list (also it might be related to Bug 1263122) Thanks! Honza

(In reply to Tim Nguyen [:ntim] from comment #56) > Issues I've found: > - Pseudo class panel doesn't work in firebug theme (you can't toggle it) Fixed > - The pane-expand icon is invisible in dark and light themes (probably a > variable you forgot to set) I can't see this, can you attach a screenshot/more info? Honza

Comment on attachment 8740384 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8740384 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/definitions.js @@ +473,4 @@ > exports.defaultThemes = [ > Tools.darkTheme, > Tools.lightTheme, > + Tools.firebugTheme, We should pref off adding the theme to the list until the follow ups are done and/or we hit our deadline, right?

Comment on attachment 8740384 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8740384 [details] [diff] [review]: ----------------------------------------------------------------- This is getting better! ::: devtools/client/themes/common.css @@ +16,5 @@ > --monospace-font-family: Consolas, monospace; > } > > +:root[platform="linux"], > +:root.theme-firebug { This is setting the proportional font-family for linux default themes too, please make sure that doesn't get changed @@ +110,5 @@ > +.theme-firebug .devtools-autocomplete-popup { > + border-color: var(--theme-splitter-color); > + border-radius: 5px; > + font-size: var(--theme-autompletion-font-size); > + padding: 1px; Does the padding make such a difference ? ::: devtools/client/themes/firebug-theme.css @@ +12,5 @@ > + font-family: var(--proportional-font-family); > +} > + > +.theme-firebug .devtools-searchinput { > + background-image: url(chrome://devtools/skin/images/firebug/filter.svg); The fact that --magnifying-glass-image is set to this in toolbars.css makes this rule redundant @@ +74,1 @@ > font-family: Lucida Grande, Tahoma, sans-serif; Is font-family needed ? If so, please use var(--proportional-font-family), but it should normally simply inherit the font @@ +216,5 @@ > + > +.theme-firebug .theme-toolbar button, > +.theme-firebug .devtools-button, > +.theme-firebug toolbarbutton { > + margin: 1; This isn't a valid margin value ::: devtools/client/themes/toolbars.css @@ +647,5 @@ > +} > + > +.theme-firebug #toolbox-controls toolbarbutton:hover { > + background: none; > + box-shadow: none; Is this needed ? We don't set a box-shadow in the default theme. ::: devtools/client/themes/variables.css @@ +60,1 @@ > } In light and dark themes, --theme-pane-expand-image is missing, which is causing the icon to disappear when you click on it to collapse the sidebar. ::: devtools/client/themes/widgets.css @@ +371,5 @@ > +.theme-firebug .breadcrumbs-widget-item:not(:first-child)::before { > + content: url(chrome://devtools/skin/images/firebug/breadcrumbs-divider.svg); > + position: relative; > + left: -3px; > + margin: 0 0 0 -5px; If this doesn't work well in RTL, I would recommend: offset-inline-start: -3px; margin: 0; margin-inline-start: -5px;

Comment on attachment 8740385 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8740385 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/debugger.css @@ +717,5 @@ > + padding-bottom: 2px; > +} > + > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > + content: "()"; If you would like to add (), please do this within JS (I would recommend opening a new bug for that so we can figure out whether we want to do this everywhere or not). @@ +719,5 @@ > + > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > + content: "()"; > + color: var(--theme-content-color2); > + font-family: monospace; nit: font-family: var(--monospace-font-family) @@ +730,5 @@ > + font-weight: bold; > +} > + > +.theme-firebug #callstack-list .side-menu-widget-item { > + margin: 0 4px 0 4px; nit: margin: 0 4px; ::: devtools/client/themes/layout.css @@ +7,5 @@ > +.theme-dark, > +.theme-light, > +.theme-firebug { > + --layout-margins-color: #edff64; > + --layout-borders-color: #D1C57A; Please restore the older black colour for the borders. If you want to change this, please file a new bug, so we can change the border colours on the page highlighter as well. ::: devtools/client/themes/webconsole.css @@ +63,5 @@ > align-self: flex-start; > } > > +.theme-firebug .message > .icon { > + margin: 0 6px 0 0; nit: margin: 0; margin-inline-end: 6px; @@ +636,5 @@ > + border-right: 1px solid #D7D7D7 !important; > +} > + > +.theme-firebug .consoletable .table-widget-column-header { > + padding: 3px 13px 2px 4px !important; padding-top: 3px; padding-inline-end: 13px; padding-bottom: 2px; padding-inline-start: 4px; @@ +641,5 @@ > + font-size: 11px !important; > + font-family: sans-serif; > + height: 20px !important; > + min-height: 20px !important; > + font-weight: bold !important; I see you're sprinkling important all over the place, can you check whether you can remove them ? @@ +646,5 @@ > + color: #000000; > +} > + > +.theme-firebug .consoletable .table-widget-cell > .console-string { > + color: #FF0000 !important; Making a color change for a specific object for only console.table doesn't seem right to me. Can you remove this change ? @@ +650,5 @@ > + color: #FF0000 !important; > +} > + > +.theme-firebug .consoletable .table-widget-column-header[sorted="descending"]:not(:active) { > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); A simple background-image swap should work. @@ +655,5 @@ > + background-color: #C5CBD9; > +} > + > +.theme-firebug .consoletable .table-widget-column-header[sorted="ascending"]:not(:active) { > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); Same here. @@ +670,5 @@ > + position: absolute; > +} > + > +.theme-firebug .consoletable .table-widget-body .devtools-side-splitter:last-of-type { > + display: none; I would love this fix to be done across all themes :)

Thanks Tim and Honza for the great progress here. I'll take another look at the updated patches, but won't get a start on that until at least later in the day tomorrow so feel free to upload new patches based on the latest comments, it won't interrupt me.

(In reply to Tim Nguyen [:ntim] from comment #65) > Comment on attachment 8740384 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8740384 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is getting better! > > ::: devtools/client/themes/common.css > @@ +16,5 @@ > > --monospace-font-family: Consolas, monospace; > > } > > > > +:root[platform="linux"], > > +:root.theme-firebug { > > This is setting the proportional font-family for linux default themes too, > please make sure that doesn't get changed Done > > @@ +110,5 @@ > > +.theme-firebug .devtools-autocomplete-popup { > > + border-color: var(--theme-splitter-color); > > + border-radius: 5px; > > + font-size: var(--theme-autompletion-font-size); > > + padding: 1px; > > Does the padding make such a difference ? True, removed I also fixed selection in the autocompletion popup. Btw. the autompletion-popup.js code is wrong and needs to be fixed, a comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1263889#c1 > ::: devtools/client/themes/firebug-theme.css > @@ +12,5 @@ > > + font-family: var(--proportional-font-family); > > +} > > + > > +.theme-firebug .devtools-searchinput { > > + background-image: url(chrome://devtools/skin/images/firebug/filter.svg); > > The fact that --magnifying-glass-image is set to this in toolbars.css makes > this rule redundant Nice, removed > > @@ +74,1 @@ > > font-family: Lucida Grande, Tahoma, sans-serif; > > Is font-family needed ? If so, please use var(--proportional-font-family), > but it should normally simply inherit the font Removed > > @@ +216,5 @@ > > + > > +.theme-firebug .theme-toolbar button, > > +.theme-firebug .devtools-button, > > +.theme-firebug toolbarbutton { > > + margin: 1; > > This isn't a valid margin value Fixed > > ::: devtools/client/themes/toolbars.css > @@ +647,5 @@ > > +} > > + > > +.theme-firebug #toolbox-controls toolbarbutton:hover { > > + background: none; > > + box-shadow: none; > > Is this needed ? We don't set a box-shadow in the default theme. Removed > > ::: devtools/client/themes/variables.css > @@ +60,1 @@ > > } > > In light and dark themes, --theme-pane-expand-image is missing, which is > causing the icon to disappear when you click on it to collapse the sidebar. Cool, fixed > > ::: devtools/client/themes/widgets.css > @@ +371,5 @@ > > +.theme-firebug .breadcrumbs-widget-item:not(:first-child)::before { > > + content: url(chrome://devtools/skin/images/firebug/breadcrumbs-divider.svg); > > + position: relative; > > + left: -3px; > > + margin: 0 0 0 -5px; > > If this doesn't work well in RTL, I would recommend: > > offset-inline-start: -3px; > margin: 0; > margin-inline-start: -5px; This is not enough for RTL (the arrow image should be on the right side pointing to the other direction). I made a note https://bugzilla.mozilla.org/show_bug.cgi?id=1263889#c2 Honza

(In reply to Tim Nguyen [:ntim] from comment #66) > Comment on attachment 8740385 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8740385 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/debugger.css > @@ +717,5 @@ > > + padding-bottom: 2px; > > +} > > + > > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > > + content: "()"; > > If you would like to add (), please do this within JS (I would recommend > opening a new bug for that so we can figure out whether we want to do this > everywhere or not). Made a comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1263889#c3 > > @@ +719,5 @@ > > + > > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > > + content: "()"; > > + color: var(--theme-content-color2); > > + font-family: monospace; > > nit: font-family: var(--monospace-font-family) Fixed > > @@ +730,5 @@ > > + font-weight: bold; > > +} > > + > > +.theme-firebug #callstack-list .side-menu-widget-item { > > + margin: 0 4px 0 4px; > > nit: margin: 0 4px; Fixed > > ::: devtools/client/themes/layout.css > @@ +7,5 @@ > > +.theme-dark, > > +.theme-light, > > +.theme-firebug { > > + --layout-margins-color: #edff64; > > + --layout-borders-color: #D1C57A; > > Please restore the older black colour for the borders. If you want to change > this, please file a new bug, so we can change the border colours on the page > highlighter as well. Done > > ::: devtools/client/themes/webconsole.css > @@ +63,5 @@ > > align-self: flex-start; > > } > > > > +.theme-firebug .message > .icon { > > + margin: 0 6px 0 0; > > nit: > margin: 0; > margin-inline-end: 6px; Done > > @@ +636,5 @@ > > + border-right: 1px solid #D7D7D7 !important; > > +} > > + > > +.theme-firebug .consoletable .table-widget-column-header { > > + padding: 3px 13px 2px 4px !important; > > padding-top: 3px; > padding-inline-end: 13px; > padding-bottom: 2px; > padding-inline-start: 4px; > > @@ +641,5 @@ > > + font-size: 11px !important; > > + font-family: sans-serif; > > + height: 20px !important; > > + min-height: 20px !important; > > + font-weight: bold !important; > > I see you're sprinkling important all over the place, can you check whether > you can remove them ? The entire rule simplified + comments for !important > > @@ +646,5 @@ > > + color: #000000; > > +} > > + > > +.theme-firebug .consoletable .table-widget-cell > .console-string { > > + color: #FF0000 !important; > > Making a color change for a specific object for only console.table doesn't > seem right to me. Can you remove this change ? Removed > > @@ +650,5 @@ > > + color: #FF0000 !important; > > +} > > + > > +.theme-firebug .consoletable .table-widget-column-header[sorted="descending"]:not(:active) { > > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); > > A simple background-image swap should work. I couldn't make that work (the position and repeat is wrong if I use just background-image) Anyway, I moved all into widgets.css (removed .consoletable) so, the Storage table is also themed. > > @@ +655,5 @@ > > + background-color: #C5CBD9; > > +} > > + > > +.theme-firebug .consoletable .table-widget-column-header[sorted="ascending"]:not(:active) { > > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); > > Same here. > > @@ +670,5 @@ > > + position: absolute; > > +} > > + > > +.theme-firebug .consoletable .table-widget-body .devtools-side-splitter:last-of-type { > > + display: none; > > I would love this fix to be done across all themes :) .theme-firebug removed from the selector. Honza

(In reply to Brian Grinstead [:bgrins] from comment #64) > Comment on attachment 8740384 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8740384 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/definitions.js > @@ +473,4 @@ > > exports.defaultThemes = [ > > Tools.darkTheme, > > Tools.lightTheme, > > + Tools.firebugTheme, > > We should pref off adding the theme to the list until the follow ups are > done and/or we hit our deadline, right? The dead line is this Friday so, lets see how many of the follow-ups we can fix till the end of the week. We'll need to ask for uplift for the rest (if urgent). In any case I filled (meta) bug 1263889 to keep track of those follow ups. Honza

Btw. I am usually applying patches in this order: 1) bug1244054-theme-images.patch 2) bug1244054-theme-firebug.patch 3) bug1244054-theme-tools.patch Honza

Comment on attachment 8740657 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8740657 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/debugger.css @@ +717,5 @@ > + padding-bottom: 2px; > +} > + > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > + content: "()"; You forgot to remove this change ::: devtools/client/themes/netmonitor.css @@ +154,5 @@ > } > > +.requests-menu-header { > + cursor: pointer; > + -moz-user-select: none; Please remove the cursor change. -moz-user-select: none; seems useless in this case (the header is already unselectable). ::: devtools/client/themes/webconsole.css @@ +399,5 @@ > border-top-color: #e0e0e0; > } > > +.theme-firebug .jsterm-input-container { > + border-top: 1px solid #CCCCCC; nit: border-top-color: #ccc; ::: devtools/client/themes/widgets.css @@ +1335,5 @@ > + font-weight: bold !important; > +} > + > +.theme-firebug .table-widget-column-header[sorted=descending]:not(:active) { > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); The reason why a simple image-swap probably didn't work for you is because you used the shorthand instead of the longhand. Longhand (only changes background-image): background-image: url(chrome://devtools/skin/images/firebug/arrow-down.svg); Shorthand (resets all other longhand properties): background: url(chrome://devtools/skin/images/firebug/arrow-down.svg); @@ +1340,5 @@ > + background-color: #C5CBD9; > +} > + > +.theme-firebug .table-widget-column-header[sorted=ascending]:not(:active) { > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); Same comment as above. @@ +1348,5 @@ > +.theme-firebug .table-widget-body .devtools-side-splitter { > + background: linear-gradient(#CBD4DE,#656A6E,#CBD4DE 18px, transparent 18px), > + linear-gradient(rgba(255, 255, 255, 0.05), rgba(0, 0, 0, 0.05) 10px, transparent 18px), > + linear-gradient(transparent, rgba(0, 0, 0, 0.5) 10px, transparent 18px) > + repeat-x transparent !important; nit: please align this properly

(In reply to Tim Nguyen [:ntim] from comment #72) > Comment on attachment 8740657 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8740657 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/debugger.css > @@ +717,5 @@ > > + padding-bottom: 2px; > > +} > > + > > +.theme-firebug #callstack-list .dbg-classic-stackframe::after { > > + content: "()"; > > You forgot to remove this change Sorry, removed now. > > ::: devtools/client/themes/netmonitor.css > @@ +154,5 @@ > > } > > > > +.requests-menu-header { > > + cursor: pointer; > > + -moz-user-select: none; > > Please remove the cursor change. Done > -moz-user-select: none; seems useless in this case (the header is already > unselectable). True, done. > > ::: devtools/client/themes/webconsole.css > @@ +399,5 @@ > > border-top-color: #e0e0e0; > > } > > > > +.theme-firebug .jsterm-input-container { > > + border-top: 1px solid #CCCCCC; > > nit: border-top-color: #ccc; Done > > ::: devtools/client/themes/widgets.css > @@ +1335,5 @@ > > + font-weight: bold !important; > > +} > > + > > +.theme-firebug .table-widget-column-header[sorted=descending]:not(:active) { > > + background: url(chrome://devtools/skin/images/firebug/arrow-down.svg) no-repeat calc(100% - 4px); > > The reason why a simple image-swap probably didn't work for you is because > you used the shorthand instead of the longhand. > > Longhand (only changes background-image): > background-image: url(chrome://devtools/skin/images/firebug/arrow-down.svg); > > Shorthand (resets all other longhand properties): > background: url(chrome://devtools/skin/images/firebug/arrow-down.svg); Yes, I tried it. The thing is that I have to specify all three props in order to get the desired look (image, repeat, position) > > @@ +1340,5 @@ > > + background-color: #C5CBD9; > > +} > > + > > +.theme-firebug .table-widget-column-header[sorted=ascending]:not(:active) { > > + background: url(chrome://devtools/skin/images/firebug/arrow-up.svg) no-repeat calc(100% - 4px); > > Same comment as above. > > @@ +1348,5 @@ > > +.theme-firebug .table-widget-body .devtools-side-splitter { > > + background: linear-gradient(#CBD4DE,#656A6E,#CBD4DE 18px, transparent 18px), > > + linear-gradient(rgba(255, 255, 255, 0.05), rgba(0, 0, 0, 0.05) 10px, transparent 18px), > > + linear-gradient(transparent, rgba(0, 0, 0, 0.5) 10px, transparent 18px) > > + repeat-x transparent !important; > > nit: please align this properly Done Thanks! Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #70) > (In reply to Brian Grinstead [:bgrins] from comment #64) > > Comment on attachment 8740384 [details] [diff] [review] > > bug1244054-theme-firebug.patch > > > > Review of attachment 8740384 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: devtools/client/definitions.js > > @@ +473,4 @@ > > > exports.defaultThemes = [ > > > Tools.darkTheme, > > > Tools.lightTheme, > > > + Tools.firebugTheme, > > > > We should pref off adding the theme to the list until the follow ups are > > done and/or we hit our deadline, right? > The dead line is this Friday so, lets see how many of the follow-ups we can > fix till the end of the week. We'll need to ask for uplift for the rest (if > urgent). > > In any case I filled (meta) bug 1263889 to keep track of those follow ups. 1) The merge day is now 04-25 2) We still might want to pref it off though, right? A pref change would be an easy uplift to go along with any follow ups (if we decide they are needed)

(In reply to Brian Grinstead [:bgrins] from comment #74) > (In reply to Jan Honza Odvarko [:Honza] from comment #70) > > (In reply to Brian Grinstead [:bgrins] from comment #64) > > > Comment on attachment 8740384 [details] [diff] [review] > > > bug1244054-theme-firebug.patch > > > > > > Review of attachment 8740384 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: devtools/client/definitions.js > > > @@ +473,4 @@ > > > > exports.defaultThemes = [ > > > > Tools.darkTheme, > > > > Tools.lightTheme, > > > > + Tools.firebugTheme, > > > > > > We should pref off adding the theme to the list until the follow ups are > > > done and/or we hit our deadline, right? > > The dead line is this Friday so, lets see how many of the follow-ups we can > > fix till the end of the week. We'll need to ask for uplift for the rest (if > > urgent). > > > > In any case I filled (meta) bug 1263889 to keep track of those follow ups. > > 1) The merge day is now 04-25 > 2) We still might want to pref it off though, right? A pref change would be > an easy uplift to go along with any follow ups (if we decide they are needed) If we pref it off nobody will use it and we get no feedback in early stages when it's easy to uplift fixes. Why do you want to actually do it? Honza

Comment on attachment 8740656 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8740656 [details] [diff] [review]: ----------------------------------------------------------------- This all looks pretty good, nice work so far. I'll take one more quick look at this once a new version is uploaded ::: devtools/client/definitions.js @@ +442,4 @@ > exports.defaultThemes = [ > Tools.darkTheme, > Tools.lightTheme, > + Tools.firebugTheme, I guess no need to pref it off if you think it's ready for broader testing ::: devtools/client/themes/common.css @@ +21,5 @@ > --monospace-font-family: monospace; > } > > +:root.theme-firebug { > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; Seems like the firebug overrides for --monospace-font-family and --proportional-font-family belong in variables.css, but I don't feel too strongly about it so do what you think is best ::: devtools/client/themes/toolbars.css @@ +640,5 @@ > } > > +/* Save space in Firebug theme */ > +.theme-firebug #toolbox-controls toolbarbutton { > + margin-left: 0 !important; Do you want 'left' and not 'inline-start' here? @@ +843,5 @@ > padding: 0; > border-style: solid; > border-width: 0; > -moz-border-start-width: 1px; > -moz-box-align: center; These lines need rebasing since https://hg.mozilla.org/mozilla-central/diff/61e35a492b0d/devtools/client/themes/toolbars.css ::: devtools/client/themes/variables.css @@ +150,5 @@ > + --theme-pane-collapse-image: url(chrome://devtools/skin/images/firebug/pane-collapse.svg); > + --theme-pane-expand-image: url(chrome://devtools/skin/images/firebug/pane-expand.svg); > + > + /* CodeMirror */ > + --theme-firebug-object: #006400; /*Green*/ Why are --theme-firebug-object and --theme-firebug-string assigned as variables here vs the large number of hardcoded colors in .theme-firebug.css (like .theme-firebug .cm-link). I'd say remove these vars and put them as hardcoded colors alongside all of those ::: devtools/client/themes/widgets.css @@ +336,5 @@ > } > > +/* Firebug theme support for breadcrumbs widget. */ > + > +.theme-firebug .breadcrumbs-widget-item { Just a heads up that the breadcrumbs widget is changing a bit in Bug 1259812 and this CSS may need some maintenance. I'd like if we could make the normal light theme breadcrumbs more closely match these though (let's talk with Helen about that as a follow up).

(In reply to Jan Honza Odvarko [:Honza] from comment #70) > (In reply to Brian Grinstead [:bgrins] from comment #64) > > Comment on attachment 8740384 [details] [diff] [review] > > bug1244054-theme-firebug.patch > > > > Review of attachment 8740384 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: devtools/client/definitions.js > > @@ +473,4 @@ > > > exports.defaultThemes = [ > > > Tools.darkTheme, > > > Tools.lightTheme, > > > + Tools.firebugTheme, > > > > We should pref off adding the theme to the list until the follow ups are > > done and/or we hit our deadline, right? > The dead line is this Friday so, lets see how many of the follow-ups we can > fix till the end of the week. We'll need to ask for uplift for the rest (if > urgent). > > In any case I filled (meta) bug 1263889 to keep track of those follow ups. According to https://wiki.mozilla.org/Electrolysis#Schedule_and_Status, 48 is planning to roll out e10s only for users without addons. So just verifying: do we not have to target this for Firebug users on release 48? Because that might change our strategy of when to surface this in the UI for our dev edition audience.

Comment on attachment 8741241 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8741241 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/layout.css @@ +112,5 @@ > border-color: #edff64; > } > > #layout-borders { > + border-color: #D1C57A; I believe the colors for layout-borders and layout-content are expected to match various highlighter colors on the server: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters.css#49. How important is this change? We should also be updating the colors on the server to match if these need to change, but I'd suggest that both this and that done in a follow-up ::: devtools/client/themes/markup.css @@ +297,5 @@ > + > +/* Firebug Theme */ > + > +.theme-firebug .theme-fg-color3 { > + color: blue; Could this use an existing theme variable? @@ +307,5 @@ > + color: rgb(0, 0, 136); > +} > + > +.theme-firebug .attr-value.theme-fg-color6 { > + color: red; Ditto ::: devtools/client/themes/webconsole.css @@ +69,5 @@ > +} > + > +.theme-firebug .message[severity="error"] { > + color: var(--error-color); > + background-color: var(--error-background-color); There is an existing: .theme-light .message[severity=error] { background-color: rgba(255, 150, 150, 0.3); } .theme-dark .message[severity=error] { background-color: rgba(235, 83, 104, 0.17); } Could you please extract the light and dark color into vars and unprefix these rules? @@ +132,5 @@ > white-space: nowrap; > } > > +/* More space for the location data for location URL */ > +.theme-firebug .message-location { What is an example log that hits this?

Comment on attachment 8741241 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8741241 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/widgets.css @@ +1328,5 @@ > + font-family: var(--proportional-font-family); > + color: var(--theme-body-color); > + > + /* Make sure to override the default Firebug devtools-toolbar height */ > + height: 24px !important; This fails to override the default Firebug devtools-toolbar height (At least locally on OSX). If you look in the devtools, height is computed to 28px; The storage inspector header looks quite big because of this. @@ +1354,5 @@ > + width: 1px; > + position: absolute; > +} > + > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(odd) { We use the .even class to target even cells. Use `.theme-firebug .table-widget-column .table-widget-cell:not(.even)` instead. Using :nth-child(even) looks funky when you filter the Storage Inspector table. @@ +1358,5 @@ > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(odd) { > + background: #EFEFEF; > +} > + > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(even) { Please use `.theme-firebug .table-widget-column .table-widget-cell.even` instead. @@ +1363,5 @@ > + background: #FFFFFF; > +} > + > +.theme-firebug .devtools-toolbar { > + background: linear-gradient(#E7E7E7, #DADADA); If you change background to background-image here: background-image: url(chrome://devtools/skin/images/firebug/arrow-down.svg); should just work to change the arrow. (tested myself). Also, this looks different compared to the netmonitor table, not sure this is wanted.

(In reply to Brian Grinstead [:bgrins] from comment #77) > According to https://wiki.mozilla.org/Electrolysis#Schedule_and_Status, 48 > is planning to roll out e10s only for users without addons. So just > verifying: do we not have to target this for Firebug users on release 48? > Because that might change our strategy of when to surface this in the UI for > our dev edition audience. CC Jeff. It was a while ago that we thought that 48 was the release that we needed to hit, but things could have changed since then.

(In reply to Brian Grinstead [:bgrins] from comment #76) > Comment on attachment 8740656 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8740656 [details] [diff] [review]: > ----------------------------------------------------------------- > > This all looks pretty good, nice work so far. I'll take one more quick look > at this once a new version is uploaded > > ::: devtools/client/definitions.js > @@ +442,4 @@ > > exports.defaultThemes = [ > > Tools.darkTheme, > > Tools.lightTheme, > > + Tools.firebugTheme, > > I guess no need to pref it off if you think it's ready for broader testing Yes Btw. there are already users using Firebug on Nightly (with e10s on), switching to DevTools. Those users can provide valuable early feedback. > ::: devtools/client/themes/common.css > @@ +21,5 @@ > > --monospace-font-family: monospace; > > } > > > > +:root.theme-firebug { > > + --proportional-font-family: Lucida Grande, Tahoma, sans-serif; > > Seems like the firebug overrides for --monospace-font-family and > --proportional-font-family belong in variables.css, but I don't feel too > strongly about it so do what you think is best I also don't have strong opinion, but it's nice to have all font definitions at one place. > ::: devtools/client/themes/toolbars.css > @@ +640,5 @@ > > } > > > > +/* Save space in Firebug theme */ > > +.theme-firebug #toolbox-controls toolbarbutton { > > + margin-left: 0 !important; > > Do you want 'left' and not 'inline-start' here? Yes, inline-start is a bit better. > > @@ +843,5 @@ > > padding: 0; > > border-style: solid; > > border-width: 0; > > -moz-border-start-width: 1px; > > -moz-box-align: center; > > These lines need rebasing since > https://hg.mozilla.org/mozilla-central/diff/61e35a492b0d/devtools/client/ > themes/toolbars.css Done > > ::: devtools/client/themes/variables.css > @@ +150,5 @@ > > + --theme-pane-collapse-image: url(chrome://devtools/skin/images/firebug/pane-collapse.svg); > > + --theme-pane-expand-image: url(chrome://devtools/skin/images/firebug/pane-expand.svg); > > + > > + /* CodeMirror */ > > + --theme-firebug-object: #006400; /*Green*/ > > Why are --theme-firebug-object and --theme-firebug-string assigned as > variables here vs the large number of hardcoded colors in .theme-firebug.css > (like .theme-firebug .cm-link). I'd say remove these vars and put them as > hardcoded colors alongside all of those Make sense, done > > ::: devtools/client/themes/widgets.css > @@ +336,5 @@ > > } > > > > +/* Firebug theme support for breadcrumbs widget. */ > > + > > +.theme-firebug .breadcrumbs-widget-item { > > Just a heads up that the breadcrumbs widget is changing a bit in Bug 1259812 > and this CSS may need some maintenance. I'd like if we could make the > normal light theme breadcrumbs more closely match these though (let's talk > with Helen about that as a follow up). Ok (updated patch will follow) Thanks! Honza

(In reply to Tim Nguyen [:ntim] from comment #79) > Comment on attachment 8741241 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8741241 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/widgets.css > @@ +1328,5 @@ > > + font-family: var(--proportional-font-family); > > + color: var(--theme-body-color); > > + > > + /* Make sure to override the default Firebug devtools-toolbar height */ > > + height: 24px !important; > > This fails to override the default Firebug devtools-toolbar height (At least > locally on OSX). If you look in the devtools, height is computed to 28px; > > The storage inspector header looks quite big because of this. Fixed (the height of the console.table & storage header is now 20px) Btw. The table-widget-column-header is applied to <label> and I couldn't make the label text vertically centered. Any tips? > > @@ +1354,5 @@ > > + width: 1px; > > + position: absolute; > > +} > > + > > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(odd) { > > We use the .even class to target even cells. Use `.theme-firebug > .table-widget-column .table-widget-cell:not(.even)` instead. > > Using :nth-child(even) looks funky when you filter the Storage Inspector > table. Done > > @@ +1358,5 @@ > > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(odd) { > > + background: #EFEFEF; > > +} > > + > > +.theme-firebug .table-widget-column .table-widget-cell:nth-child(even) { > > Please use `.theme-firebug .table-widget-column .table-widget-cell.even` > instead. Done > > @@ +1363,5 @@ > > + background: #FFFFFF; > > +} > > + > > +.theme-firebug .devtools-toolbar { > > + background: linear-gradient(#E7E7E7, #DADADA); > > If you change background to background-image here: > > background-image: url(chrome://devtools/skin/images/firebug/arrow-down.svg); > should just work to change the arrow. (tested myself). Nice! Fixed > > Also, this looks different compared to the netmonitor table, not sure this > is wanted. Good point, it's better now. Just the labels are not v-centered (see my comment above) (updated patch will follow) Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #81) > > I guess no need to pref it off if you think it's ready for broader testing > Yes > > Btw. there are already users using Firebug on Nightly (with e10s on), > switching to DevTools. Those users can provide valuable early feedback. Is there a flow in place to get these users transitioned to DevTools?

Comment on attachment 8741784 [details] [diff] [review] bug1244054-theme-firebug.patch Review of attachment 8741784 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/locales/en-US/toolbox.properties @@ +109,5 @@ > options.lightTheme.label=Light theme > > +# LOCALIZATION NOTE (options.firebugTheme.label) > +# Used as a label for Firebug theme > +options.firebugTheme.label=Firebug theme Running out of space here (probably more so in other locales). What about "Firebug", "Light" and "Dark" since they are just below the "Themes" header? If you'd like to do this in a follow up, please file a bug that's tracking 48 and ni? Helen so we can get the string changes in this release

Comment on attachment 8741787 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8741787 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/layout.css @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/ */ > > +/* CSS Variables specific to this panel that aren't defined by the themes */ This comment is unneeded @@ +112,5 @@ > border-color: #edff64; > } > > #layout-borders { > + border-color: #D1C57A; These color changes slipped back in even after https://bugzilla.mozilla.org/show_bug.cgi?id=1244054#c59. Please revert this. @@ +120,5 @@ > border-color: #6a5acd; > } > > #layout-content { > + background-color: #9BBFD1; and this ::: devtools/client/themes/markup.css @@ +297,5 @@ > + > +/* Firebug Theme */ > + > +.theme-firebug .theme-fg-color3 { > + color: blue; Could this use an existing theme variable? @@ +303,5 @@ > + > +.theme-firebug .open, > +.theme-firebug .close, > +.theme-firebug .attr-name.theme-fg-color2 { > + color: rgb(0, 0, 136); ditto @@ +307,5 @@ > + color: rgb(0, 0, 136); > +} > + > +.theme-firebug .attr-value.theme-fg-color6 { > + color: red; ditto

Hi Helen, have a few requests for you :) 1) I'd like your feedback on https://bugzilla.mozilla.org/show_bug.cgi?id=1244054#c86. What do you think about changing "Light theme" and "Dark theme" to "Light" and "Dark" in the options panel? 2) Can you have a look at the latest progress and do a UI review: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8e058951cd8. Some context, this shouldn't be making any significant changes to light and dark theme, just the new one 3) I'd value your opinion on our rollout plan based on the UI review. What do you think about shipping this to our Dev Edition audience in a week or so? See Comments 74, 75, and 77. Pro to riding train now is that we can get more initial feedback, con is that there are still a number of 'polish' follow ups to do (and we may have the time depending on the answer to Comment 77).

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #80) > (In reply to Brian Grinstead [:bgrins] from comment #77) > > According to https://wiki.mozilla.org/Electrolysis#Schedule_and_Status, 48 > > is planning to roll out e10s only for users without addons. So just > > verifying: do we not have to target this for Firebug users on release 48? > > Because that might change our strategy of when to surface this in the UI for > > our dev edition audience. > > CC Jeff. It was a while ago that we thought that 48 was the release that we > needed to hit, but things could have changed since then. 48 is the release we need to hit.

(In reply to Brian Grinstead [:bgrins] from comment #87) > Comment on attachment 8741787 [details] [diff] [review] > bug1244054-theme-tools.patch > > Review of attachment 8741787 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/layout.css > @@ +1,5 @@ > > /* This Source Code Form is subject to the terms of the Mozilla Public > > * License, v. 2.0. If a copy of the MPL was not distributed with this > > * file, You can obtain one at http://mozilla.org/MPL/2.0/ */ > > > > +/* CSS Variables specific to this panel that aren't defined by the themes */ > > This comment is unneeded Removed > > @@ +112,5 @@ > > border-color: #edff64; > > } > > > > #layout-borders { > > + border-color: #D1C57A; > > These color changes slipped back in even after > https://bugzilla.mozilla.org/show_bug.cgi?id=1244054#c59. Please revert > this. > > @@ +120,5 @@ > > border-color: #6a5acd; > > } > > > > #layout-content { > > + background-color: #9BBFD1; > > and this Both reverted > > ::: devtools/client/themes/markup.css > @@ +297,5 @@ > > + > > +/* Firebug Theme */ > > + > > +.theme-firebug .theme-fg-color3 { > > + color: blue; > > Could this use an existing theme variable? > > @@ +303,5 @@ > > + > > +.theme-firebug .open, > > +.theme-firebug .close, > > +.theme-firebug .attr-name.theme-fg-color2 { > > + color: rgb(0, 0, 136); > > ditto > > @@ +307,5 @@ > > + color: rgb(0, 0, 136); > > +} > > + > > +.theme-firebug .attr-value.theme-fg-color6 { > > + color: red; > > ditto Variables used (updated patches will follow) Honza

Rebasing

(In reply to Brian Grinstead [:bgrins] from comment #86) > Comment on attachment 8741784 [details] [diff] [review] > bug1244054-theme-firebug.patch > > Review of attachment 8741784 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/locales/en-US/toolbox.properties > @@ +109,5 @@ > > options.lightTheme.label=Light theme > > > > +# LOCALIZATION NOTE (options.firebugTheme.label) > > +# Used as a label for Firebug theme > > +options.firebugTheme.label=Firebug theme > > Running out of space here (probably more so in other locales). What about > "Firebug", "Light" and "Dark" since they are just below the "Themes" header? I like the idea. Honza

(In reply to Brian Grinstead [:bgrins] from comment #85) > (In reply to Jan Honza Odvarko [:Honza] from comment #81) > > > I guess no need to pref it off if you think it's ready for broader testing > > Yes > > > > Btw. there are already users using Firebug on Nightly (with e10s on), > > switching to DevTools. Those users can provide valuable early feedback. > > Is there a flow in place to get these users transitioned to DevTools? Yes, Firebug 2 implements a popup window that appears when running in e10s enabled browser. The popup notifies the user about built in DevTools. The user have two options (a) the default - go ahead and open built-in tools or (b) switch off e10s and continue using Firebug. Firebug also needs to activate the Firebug theme. It should happen just once for all users that have Firebug 2 enabled (this step needs this bug landed). Honza

Fixing one CSS parser error on try. This try push looks good! (just unrelated debugger orange fail) https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeee5b39c97e&selectedJob=19653178 Honza

Comment on attachment 8742749 [details] [diff] [review] bug1244054-theme-tools.patch Review of attachment 8742749 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine. I still have a pending ni? for Helen for a UI review and feedback on the theme strings, so please check with her about whether to land this now and handle these things in follow ups or what. We definitely need to get string changes in for 48 if we want to go with those

(In reply to (Unavailable until April 25) Brian Grinstead [:bgrins] from comment #88) > Hi Helen, have a few requests for you :) > > 1) I'd like your feedback on > https://bugzilla.mozilla.org/show_bug.cgi?id=1244054#c86. What do you think > about changing "Light theme" and "Dark theme" to "Light" and "Dark" in the > options panel? > 2) Can you have a look at the latest progress and do a UI review: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8e058951cd8. Some > context, this shouldn't be making any significant changes to light and dark > theme, just the new one > 3) I'd value your opinion on our rollout plan based on the UI review. What > do you think about shipping this to our Dev Edition audience in a week or > so? See Comments 74, 75, and 77. Pro to riding train now is that we can > get more initial feedback, con is that there are still a number of 'polish' > follow ups to do (and we may have the time depending on the answer to > Comment 77). 1. 100% fine with the theme string changes, makes sense to me. 2. I do a few ui nits that I think can be addressed with follow-up polish bugs: - Sometimes the tab looks odd with the background because of the contents of the pane. (See screenshot: http://cl.ly/3c2c1B1A2G2W) Tools with a secondary toolbar don’t seem to have this problem. Temporary fix below: - Man, the command buttons look strange with Firebug. (http://cl.ly/310Z211R1I12) unsure what to do about that right now. - I also notice that the Settings cog is sitting a little low: http://cl.ly/310Z211R1I12 That might be regular theme CSS messing with Firebug CSS. - The command-pick, on the other hand, looks a little high: http://cl.ly/261I1c3j411X 3. I’m fine with it going out early; I think the one thing I would like to see happen (since this is such a huge chunk of work) is for us to broadcast this more widely than our Hacks post. (CC’ing Bryan about that.) That way we can alert more Firebug users to the change and also reference that this is a work-in-progress, expect these things to be wonky as they’re known issues, pls file bugs kthnx, etc.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #97) > - Man, the command buttons look strange with Firebug. > (http://cl.ly/310Z211R1I12) unsure what to do about that right now. I can try to come up with a new icon set if you want me to. Not sure if I can do all before 48 rides the trains but at least the ones that are shown by default. Sebastian

Labels changed. Honza

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #97) > 1. 100% fine with the theme string changes, makes sense to me. Patch attached, please review, thanks! > 2. I do a few ui nits that I think can be addressed with follow-up polish > bugs: > - Sometimes the tab looks odd with the background because of the contents of > the pane. (See screenshot: http://cl.ly/3c2c1B1A2G2W) Tools with a secondary > toolbar don’t seem to have this problem. Temporary fix below: > - Man, the command buttons look strange with Firebug. > (http://cl.ly/310Z211R1I12) unsure what to do about that right now. > - I also notice that the Settings cog is sitting a little low: > http://cl.ly/310Z211R1I12 That might be regular theme CSS messing with > Firebug CSS. > - The command-pick, on the other hand, looks a little high: > http://cl.ly/261I1c3j411X Great, I put all into a follow up here: https://bugzilla.mozilla.org/show_bug.cgi?id=1263889#c5 (I created just one collecting all the feedback so far, can be used as meta) > 3. I’m fine with it going out early; I think the one thing I would like to > see happen (since this is such a huge chunk of work) is for us to broadcast > this more widely than our Hacks post. (CC’ing Bryan about that.) That way we > can alert more Firebug users to the change and also reference that this is a > work-in-progress, expect these things to be wonky as they’re known issues, > pls file bugs kthnx, etc. Make sense to me. (In reply to Sebastian Zartner [:sebo] from comment #98) > (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #97) > > - Man, the command buttons look strange with Firebug. > > (http://cl.ly/310Z211R1I12) unsure what to do about that right now. > > I can try to come up with a new icon set if you want me to. Not sure if I > can do all before 48 rides the trains but at least the ones that are shown > by default. That would be great, but please create a follow up for it, thanks. Honza

Here is new try push for the label changes, green (just debugger unrelated orange): https://treeherder.mozilla.org/#/jobs?repo=try&revision=14189f9570a8&selectedJob=19677412 Patches should be applied in this order: 1) bug1244054-theme-images.patch 2) bug1244054-theme-firebug.patch 3) bug1244054-theme-tools.patch 4) bug1244054-theme-labels.patch Honza

https://hg.mozilla.org/integration/fx-team/rev/1fb42ed8baa7 https://hg.mozilla.org/integration/fx-team/rev/0b7fdc7a9088 https://hg.mozilla.org/integration/fx-team/rev/f06ae0dbd3a5 https://hg.mozilla.org/integration/fx-team/rev/d42b25947fc2

(In reply to Jan Honza Odvarko [:Honza] from comment #100) > (In reply to Sebastian Zartner [:sebo] from comment #98) > > (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #97) > > > - Man, the command buttons look strange with Firebug. > > > (http://cl.ly/310Z211R1I12) unsure what to do about that right now. > > > > I can try to come up with a new icon set if you want me to. Not sure if I > > can do all before 48 rides the trains but at least the ones that are shown > > by default. > That would be great, but please create a follow up for it, thanks. I've created bug 1265985 for them. Let me know if it should instead be blocking bug 1263889. Sebastian

https://hg.mozilla.org/mozilla-central/rev/1fb42ed8baa7 https://hg.mozilla.org/mozilla-central/rev/0b7fdc7a9088 https://hg.mozilla.org/mozilla-central/rev/f06ae0dbd3a5 https://hg.mozilla.org/mozilla-central/rev/d42b25947fc2

Release Note Request (optional, but appreciated) [Why is this notable]: Transitions Firebug users to the built-in DevTools [Suggested wording]: Firebug theme available for DevTools [Links (documentation, blog post, etc)]: None yet.

Added to Fx48 Aurora release notes

Documented this here: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Choose_DevTools_theme https://developer.mozilla.org/en-US/Firefox/Releases/48 Please let me know if that's ok for you, Honza. Sebastian

Ritu, could you please link the note to the first page in comment 108. Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #108) > Documented this here: > https://developer.mozilla.org/en-US/docs/Tools/ > Tools_Toolbox#Choose_DevTools_theme > https://developer.mozilla.org/en-US/Firefox/Releases/48 > > Please let me know if that's ok for you, Honza. Looks good to me. Honza

(In reply to Sebastian Zartner [:sebo] from comment #109) > Ritu, could you please link the note to the first page in comment 108. > > Sebastian Done!

Great, thanks to you both! Sebastian

I tried to add the theme on Fx 48.0b5 build ID: 20160630123429 using Windows 10. The theme is working.