Responsive mode and global developer toolbar are affected.

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.

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.

(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.

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.

(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)?

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.

Screenshot with patch applied

Comment on attachment 8344909 [details] [diff] [review] theme-toolbars-browser.patch r+ assuming it looks good on Linux and Windows.

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 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 on attachment 8344909 [details] [diff] [review] theme-toolbars-browser.patch I don't think that super review is required :)

(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 on attachment 8344909 [details] [diff] [review] theme-toolbars-browser.patch Review of attachment 8344909 [details] [diff] [review]: ----------------------------------------------------------------- Looks good on stock Ubuntu.

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 on attachment 8344909 [details] [diff] [review] theme-toolbars-browser.patch Looks good to me. Will attach screeny

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

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:

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

(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?

Sure, I can check.

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.

Aurora looks fine too.

Thanks!