947836 - Non-toolbox devtools toolbars are grey

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Description

[Paul Rouget [:paul]]

Attachment: screenshot

Comment 1

[Paul Rouget [:paul]]

Responsive mode and global developer toolbar are affected.

Comment 2

[Brian Grinstead [:bgrins]]

So it seems we need to move the toolbar styles into common.css to be included in the browser UI.  I wonder if it would make sense to include toolbars.inc.css from common.css instead of cutting/pasting a bunch of styles across.

An additional issue is that we want to be able to theme these toolbars in the future (in Bug 942292) - we could wait until that bug to figure out those issues.

Comment 3

[Paul Rouget [:paul]]

The devtools CSS code that will be included in browser.css should not come with any useless stuff (this code will be loaded by all the users).

The responsive mode toolbar has nothing in common with the toolbox.
The gcli toolbar will soon be killed, so no need to support the white theme.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #3)
> The devtools CSS code that will be included in browser.css should not come
> with any useless stuff (this code will be loaded by all the users).
> 
> The responsive mode toolbar has nothing in common with the toolbox.
> The gcli toolbar will soon be killed, so no need to support the white theme.

What do you mean the responsive mode toolbar has nothing in common with the toolbox - are you talking about not needing to theme it?  It has the same devtools-toolbar class that is shared with the toolbox, and we will need to theme devtools-toolbar since it is used throughout devtools.  Since the responsive panel is being loaded in browser.xul we need to move at least some of the styles across to common.css.  I can go ahead and move the minimum needed to fix this regression instead of importing the whole file.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbars-browser.patch

Move shared styles from toolbars.inc.css to common.css.  Also copies over old background and border properties for .menulist-dropmarker to fix extra background on OSX.

Comment 6

[Paul Rouget [:paul]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Paul Rouget [:paul] from comment #3)
> > The devtools CSS code that will be included in browser.css should not come
> > with any useless stuff (this code will be loaded by all the users).
> > 
> > The responsive mode toolbar has nothing in common with the toolbox.
> > The gcli toolbar will soon be killed, so no need to support the white theme.
> 
> What do you mean the responsive mode toolbar has nothing in common with the
> toolbox

If you look at the "look" of the responsive mode toolbar, it doesn't look like a devtools toolbar (it's transparent, you don't even see it).

> are you talking about not needing to theme it?

Yes.

> It has the same
> devtools-toolbar class that is shared with the toolbox, and we will need to
> theme devtools-toolbar since it is used throughout devtools.  Since the
> responsive panel is being loaded in browser.xul we need to move at least
> some of the styles across to common.css.  I can go ahead and move the
> minimum needed to fix this regression instead of importing the whole file.

What if we don't move the style, but write specific code for these toolbars (dropping the devtools-toolbar class)?

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbars-browser.patch

This patch removes the devtools-toolbar class from the responsive UI and commandline, and copies over existing styles to theme responsive UI.  This way we avoid adding anything extra to common.css, which is loaded in the browser.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Screenshot 2013-12-09 16.23.47.png

Screenshot with patch applied

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

r+ assuming it looks good on Linux and Windows.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

Would you mind taking a look at the UI in Windows?  This patch updates the GCLI bar and responsive view toolbars, they should look normal with it applied.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

Panos,
Can you take a look at this on Linux and let me know if the responsive mode buttons and gcli buttons look normal?

Comment 13

[Girish Sharma [:Optimizer]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

I don't think that super review is required :)

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #13)
> Comment on attachment 8344909 [details] [diff] [review]
> theme-toolbars-browser.patch
> 
> I don't think that super review is required :)

Oops, thanks :)

Comment 15

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

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

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

Looks good on stock Ubuntu.

Comment 16

[Brian Grinstead [:bgrins]]

Just noticed that chrome://browser/content/devtools/connect.xhtml is using the devtools-toolbarbutton class as well, so we may want to pull the copied changes in under .devtools-toolbarbutton instead of .devtools-responsiveui-toolbarbutton.  May be a bit of confusion given the same class name, but otherwise the connect screen buttons will not get styled.

Comment 17

[Joe Walker [:jwalker]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

Looks good to me. Will attach screeny

Comment 18

[Joe Walker [:jwalker]]

Attachment: win-ss.png

Comment 20

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/ea1f2c5da310
https://tbpl.mozilla.org/?tree=Fx-Team&rev=ea1f2c5da310

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 941673
User impact if declined: Responsive view and the browser console will have mismatching toolbar styles.
Testing completed (on m-c, etc.): Been on m-c since 12/16
Risk to taking this patch (and alternatives if risky): This patch changes the contents of browser.css, though the changes are all prefixed with devtools-.  Basically a CSS only change (only removes a CSS class from the markup in browser.xul).
String or IDL/UUID changes made by this patch:

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80cc67d0dc99

Comment 24

[Merih Akar]

I can confirm that toolbar icons looks normal in Firefox 29 Nightly.

Comment 25

[u279076]

(In reply to merihakar from comment #24)
> I can confirm that toolbar icons looks normal in Firefox 29 Nightly.

Thanks, would you mind also checking tomorrow's Aurora build?

Comment 26

[Merih Akar]

Sure, I can check.

Comment 27

[Merih Akar]

I am not sure if I am on the latest Aurora build, however right now the problem still exists in Aurora. I will check again tomorrow morning.

Comment 28

[Merih Akar]

Aurora looks fine too.

Comment 29

[u279076]

Thanks!