Closed Bug 947836 Opened 6 years ago Closed 6 years ago

Non-toolbox devtools toolbars are grey

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: paul, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [good first verify])

Attachments

(4 files, 1 obsolete file)

Attached image screenshot
No description provided.
Responsive mode and global developer toolbar are affected.
Blocks: 916766
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Hardware: x86 → All
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.
Attached patch theme-toolbars-browser.patch (obsolete) — Splinter Review
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.
Attachment #8344689 - Flags: review?(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)?
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.
Attachment #8344689 - Attachment is obsolete: true
Attachment #8344689 - Flags: review?(paul)
Attachment #8344909 - Flags: review?(paul)
Screenshot with patch applied
Duplicate of this bug: 947976
Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

r+ assuming it looks good on Linux and Windows.
Attachment #8344909 - Flags: review?(paul) → review+
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.
Attachment #8344909 - Flags: ui-review?(scrapmachines)
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?
Attachment #8344909 - Flags: ui-review?(past)
Attachment #8344909 - Flags: superreview?
Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

I don't think that super review is required :)
Attachment #8344909 - Flags: superreview?
(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.
Attachment #8344909 - Flags: ui-review?(past) → ui-review+
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.
Blocks: 948324
Attachment #8344909 - Flags: ui-review?(scrapmachines) → ui-review?(jwalker)
Comment on attachment 8344909 [details] [diff] [review]
theme-toolbars-browser.patch

Looks good to me. Will attach screeny
Attachment #8344909 - Flags: ui-review?(jwalker) → ui-review+
Duplicate of this bug: 950483
Blocks: 941673
https://hg.mozilla.org/mozilla-central/rev/ea1f2c5da310
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
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:
Attachment #8344909 - Flags: approval-mozilla-aurora?
Attachment #8344909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: regression
Summary: Regression: Non-toolbox devtools toolbars are grey → Non-toolbox devtools toolbars are grey
Whiteboard: [good first verify]
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?
Status: RESOLVED → VERIFIED
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.