943883 - DevTools themes - theme sidemenuwidget

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

This is in use in the sources list in the debugger, shader editor, most of the network monitor, and the profiles list in the profiler.  There seems to be some concept of manual themes for this in widgets.css already, so it may not be too big of a deal to switch this over to use theme-body and existing colors.

Comment 1

[Brian Grinstead [:bgrins]]

Victor, if you would be interested in starting on this it would definitely increase the odds of it landing before the next uplift.  If not, I will swing back around and get to it after a few of the others I have in progress.

Comment 3

[Brian Grinstead [:bgrins]]

We may want to merge all three widgets.css files together into a shared file to make theming this (and the breadcrumbs) easier.

Comment 4

[Victor Porof [:vporof][:vp]]

That's the plan.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v1

WIP attempt at this.
Uses only colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors and documents them, so when we switch to CSS variables our lives will be easier.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: dark screenshot

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: light screenshot

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Nice! :D

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Woohoo! I assume this will fix bug 931485 as well?

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #9)
> Woohoo! I assume this will fix bug 931485 as well?

Yes, I'm going to dupe it.

Comment 12

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #7)
> Created attachment 8350517 [details]
> light screenshot

nice.

Comment 13

[Victor Porof [:vporof][:vp]]

Attachment: v2

I think this is pretty much final. I'll keep this patch in my queue and play with the tools for a while to make sure there's no hideousness anywhere, then ask for r+. This depends on bug 951633 anyway.

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: v3

I'm happy with this.

Comment 15

[Victor Porof [:vporof][:vp]]

Making this a P1 mainly just because it blocks a bunch of other bugs.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8355789 [details] [diff] [review]
v3

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

I've looked through all the non-CSS changes, and everything looks fine except that there is a new folder where the shared images should go.  Will review the CSS changes next.

::: browser/themes/linux/jar.mn
@@ +202,5 @@
>  * skin/classic/browser/devtools/scratchpad.css        (devtools/scratchpad.css)
>    skin/classic/browser/devtools/magnifying-glass.png  (devtools/magnifying-glass.png)
>    skin/classic/browser/devtools/option-icon.png       (devtools/option-icon.png)
>    skin/classic/browser/devtools/itemToggle.png        (devtools/itemToggle.png)
> +  skin/classic/browser/devtools/itemArrow-dark-rtl.png (../shared/devtools/itemArrow-dark-rtl.png)

Can you move these images and their references into the new ../shared/devtools/images folder?

Comment 17

[Victor Porof [:vporof][:vp]]

bokay!

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8355789 [details] [diff] [review]
v3

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

The patch is failing to apply on latest - can you rebase it?

Comment 19

[Victor Porof [:vporof][:vp]]

Did you apply the patch in bug 951795?

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #19)
> Did you apply the patch in bug 951795?

No, I hadn't - everything is applying now.

Comment 21

[Victor Porof [:vporof][:vp]]

Try builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-43d2259a5709/

Comment 22

[Victor Porof [:vporof][:vp]]

Bugzilla!

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: ubuntu-osx-windows-light-sidemenu.png

Light theme screenshot of side menu widget in Ubuntu, OSX, and Windows environments

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: ubuntu-osx-windows-dark-sidemenu.png

Dark theme screenshot of side menu widget in Ubuntu, OSX, and Windows environments

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8355789 [details] [diff] [review]
v3

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

Looking at the result in Windows and Linux, I see an issue with the add watch expression box in the debugger.  Besides this, everything that I've checked looks good.

::: browser/devtools/debugger/debugger.xul
@@ -422,5 @@
>            <tab id="variables-tab" label="&debuggerUI.tabs.variables;"/>
>            <tab id="events-tab" label="&debuggerUI.tabs.events;"/>
>          </tabs>
>          <tabpanels flex="1">
> -          <tabpanel id="variables-tabpanel" class="theme-body">

Removing the theme-body class here seems to be causing the white background on the 'add watch expression' box as seen in Attachment 8356151 [details] in Windows and Linux (they are inheriting the white background from the <tabpanels> element, and having theme-body here sets the background color as expected).  This may also be able to be accomplished by setting background transparent on devtools-sidebar-tabs > tabpanels in toolbars.inc.css, but in general there shouldn't be much harm in having theme-body defined twice (since it just defines text and background color).

Comment 26

[Victor Porof [:vporof][:vp]]

(In reply to Brian Grinstead [:bgrins] from comment #25)

I actually think it'd be better for this to be fixed at the toolbars.inc.css level, since this is a specific problem with the tabpanels on a specific platform, not the theme-body colors themselves not being applied as necessary. Setting a transparent background on the tabpanels will avoid having to specify the theme-body class in too many places in the future as well.

What do you think? Do you feel strongly against this?

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #26)
> (In reply to Brian Grinstead [:bgrins] from comment #25)
> 
> I actually think it'd be better for this to be fixed at the toolbars.inc.css
> level, since this is a specific problem with the tabpanels on a specific
> platform, not the theme-body colors themselves not being applied as
> necessary. Setting a transparent background on the tabpanels will avoid
> having to specify the theme-body class in too many places in the future as
> well.
> 
> What do you think? Do you feel strongly against this?

I don't feel strongly about it, it seems reasonable to try and fix it in toolbars.inc.css.  As long as we double check the other places using this same devtools-sidebar-tabs element to make sure that they are not negatively affected.  Seems that this class is used in debugger, inspector, netmonitor, scratchpad, and webconsole.

Comment 28

[Victor Porof [:vporof][:vp]]

Attachment: v4

Fixed tabpanels being transparent. Everything looks good as far as I can tell.
Here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=706261d4fcfc

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8357096 [details] [diff] [review]
v4

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

Looks good to me - issue with watch expressions is resolved on Windows and Linux

Comment 30

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/ad0783cbd9e5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad0783cbd9e5

Comment 33

[Tim Nguyen :ntim]

Nice work ! But shouldn't the gradient be gone ? Stephen's mockups makes the UI flat so...

Comment 34

[Tim Nguyen :ntim]

Btw, style editor still has old look unfortunatly.

Comment 35

[Victor Porof [:vporof][:vp]]

(In reply to Tim Nguyen [:ntim] from comment #34)
> Btw, style editor still has old look unfortunatly.

That's because the Style Editor doesn't use the widget unfortunately, but an older "splitview" implementation. There was a bug about switching, but I can't find it right now.

Comment 36

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #35)
> (In reply to Tim Nguyen [:ntim] from comment #34)
> > Btw, style editor still has old look unfortunatly.
> 
> That's because the Style Editor doesn't use the widget unfortunately, but an
> older "splitview" implementation. There was a bug about switching, but I
> can't find it right now.

The Style Editor sidebar styles will be updated in Bug 957117.