Adjust Firebug theme for OSX and Linux to be gray instead of blue

RESOLVED FIXED in Firefox 53

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: flack, Assigned: ruturaj, Mentored)

Tracking

({good-first-bug})

49 Branch
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(8 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
Posted image Spectacle.i26629.png
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161025164528

Steps to reproduce:

I've attached a screenshot with Firebug 2 and the firebug-themed devtools open. there are several problems:

- font size in the inspector window are too small
- tab headings in the rules editor are too cramped (padding missing?)
- the breadcrumb line at the bottom of the devtools window has two separator icons at each level (one of which overlaps with the text)
- while Firebug 2 uses my distribution's theme, firebug devtools theme seems to be hardcoded to that blueish gradient look

Also, as you can plainly see, practically all the colors are different, so if the goal was to make the devtools theme look like original Firebug, I guess there's still some work needed :-)

Tested on Firefox 49.0.2 on Kubuntu 16.10

Updated

3 years ago
Component: Untriaged → Developer Tools
Honza, any comments on this?
Flags: needinfo?(odvarko)
Thanks for the report and screenshot!

All the issues listed in comment #0 seem to be related to CSS.

Some pointers:
1) The markup view styles (i.e. tree of DOM elements): https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/markup.css#291
This file should be used to fix font-sizes and colors

2) The tab-title cramped padding (as visible on the attached screenshot) should be fixed in this file:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/tabs/tabs.css#124
(btw. I am not seeing the issue on Win10)

3) Styles for breadcrumbs toolbar are here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/inspector.css#123

Anyone working on this, feel free to ask for more help.

Honza
Mentor: odvarko
Flags: needinfo?(odvarko)
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
Getting fixed in Bug#1318259
- The font size (specifically Linux)

Fixed in Bug#1320304
- Cramped tabs (the Network sidebar had bad tabs, inspector sidebar seemed alright in master branch)
(Assignee)

Updated

2 years ago
Assignee: nobody → ruturaj
(Reporter)

Comment 4

2 years ago
(Reporter)

Comment 5

2 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> Thanks for the report and screenshot!


Thanks for the replies so far!

Just one question because you didn't mention it specifically: What about the issue that the Firebug theme always has this blueish gradient look? Old Firebug somehow always adapted to the Firefox UI, as you can see in my Linux screenshot and another one I did on Mac. The blue version (and also the icons to close/minify in the top right corner) might look almost seamless on some version of Windows, but on other platforms they are kind of alien
(Assignee)

Comment 6

2 years ago
I'm not sure... but the [blue] theme is synchronous with how it looks in Windows' firebug. I was once debugging on a friend's Windows Box and thats how it looked. I personally prefer the gray version. 

Would probably need a change over here.
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/variables.css#142
(Assignee)

Comment 7

2 years ago
Tim, your views on the blue colored Firebug theme ?
Flags: needinfo?(ntim.bugs)

Comment 8

2 years ago
As mentioned in comment 6, Windows uses the blue theme, while Mac and Linux use a gray theme. So it could be possible to set platform specific variables in variables.css.

:root.theme-firebug[platform="win"] {
 (variables for the blue theme)
}

:root.theme-firebug:not([platform="win"]) {
 (gray theme)
}

Honza, what do you think about this?
Flags: needinfo?(ntim.bugs) → needinfo?(odvarko)
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #8)
> As mentioned in comment 6, Windows uses the blue theme, while Mac and Linux
> use a gray theme. So it could be possible to set platform specific variables
> in variables.css.
> 
> :root.theme-firebug[platform="win"] {
>  (variables for the blue theme)
> }
> 
> :root.theme-firebug:not([platform="win"]) {
>  (gray theme)
> }
> 
> Honza, what do you think about this?
Sorry for the delay.

Yes, I agree that Win should use blue and Mac/Linux gray (just like Firebug 2 does)
So, the CSS snippet makes perfect sense to me.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 10

2 years ago
Posted patch fix-1314919-1.patch (obsolete) — Splinter Review
Fixing Firebug theme / layout issues.

- CSS Rules
- Background colors of tabs
- Background color of tab buttons
- borders of tabs / buttons
- netmonitor: cell header color, sorted cell header color
- widgets: cell header color, sorted cell header color
- Toolbar Buttons
- Fixing breadcrumbs in HTML inspector
- JSON View tabs
Attachment #8820158 - Flags: review?(ntim.bugs)
(Assignee)

Comment 11

2 years ago
Posted image firebug-overall.png
Firebug overall theme comparison
(Assignee)

Comment 12

2 years ago
Network panel, and sub-toolbar button updates..

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 13

2 years ago
(In reply to Ruturaj Vartak from comment #12)
> Created attachment 8820160 [details]
> firebug-network-buttons.png
> 
> Network panel, and sub-toolbar button updates..

just to give my two cents: Looks definitely better, but I think the active item could use a bit of border-radius (and should maybe use a gradient instead of an inset box-shadow for the background)
(Reporter)

Comment 14

2 years ago
(In reply to Ruturaj Vartak from comment #11)
> Created attachment 8820159 [details]
> firebug-overall.png
> 
> Firebug overall theme comparison

Nice! Just a small nit: The font for the tab headers (Inspector/Console/etc., Rules/Computed/etc.) could be a bit bigger and a bit less black (i.e. brighter)
(Assignee)

Comment 15

2 years ago
Posted patch fix-1314919-2.patch (obsolete) — Splinter Review
Fixing Firebug theme / layout issues.

- CSS Rules
- Background colors of tabs
- Background color of tab buttons
- borders of tabs / buttons
- netmonitor: cell header color, sorted cell header color
- widgets: cell header color, sorted cell header color
- Toolbar Buttons
- Fixing breadcrumbs in HTML inspector
- JSON View tabs

@flack - Fixed the background of active buttons and used border instead of outline.
Attachment #8820158 - Attachment is obsolete: true
Attachment #8820158 - Flags: review?(ntim.bugs)
Attachment #8821459 - Flags: review?(ntim.bugs)

Comment 16

2 years ago
Comment on attachment 8821459 [details] [diff] [review]
fix-1314919-2.patch

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

I'd really like not to introduce big extra blocks of CSS (like for the toolbar buttons) for the firebug theme.

::: devtools/client/shared/components/tabs/tabs.css
@@ +74,4 @@
>    border-width: 0;
>    border-inline-start-width: 1px;
>    border-color: var(--theme-splitter-color);
> +  cursor: pointer;

Can you remove the cursor changes ? We shouldn't have theme-specific cursors

@@ +143,4 @@
>  .theme-firebug .tabs .tabs-menu-item {
>    position: relative;
>    border-inline-start-width: 0;
> +  cursor: default;

Same comment about the cursor changes.

::: devtools/client/themes/common.css
@@ +517,5 @@
>  
> +.theme-firebug .menu-filter-button {
> +  background: transparent linear-gradient(rgba(255, 255, 255, 0.4), rgba(255, 255, 255, 0.2)) no-repeat !important;
> +  color: var(--theme-body-color) !important;
> +  border: 1px solid transparent;

Can the toolbar button states be stored in variables, and be overridden for each themes?

Something like:

<- light/dark-theme states ->
:root {
--toolbarbutton-background: <...>
--toolbarbutton-border-color: <...>
--toolbarbutton-hover-background: <...>
--toolbarbutton-hover-border-color: <...>
...
}

:root.theme-firebug {
--toolbarbutton-background: transparent;
--toolbarbutton-border-color: transparent;
--toolbarbutton-hover-background: linear-gradient(rgba(255, 255, 255, 0.4), rgba(255, 255, 255, 0.2));
--toolbarbutton-hover-border-color: silver;
...
}

::: devtools/client/themes/firebug-theme.css
@@ +87,4 @@
>    color: var(--theme-body-color);
>    -moz-box-flex: initial;
>    min-width: 0;
> +  font-size: 12px;

I don't like having font-size changes at a lot of different levels.
We should settle only for one font-size across the whole firebug theme.

::: devtools/client/themes/netmonitor.css
@@ +280,4 @@
>  }
>  
>  .theme-firebug .requests-menu-header-button[data-sorted] {
> +  background-color: #FAC8AF;

This is not right, the header uses the Windows style, but the selected header uses the Linux style.

::: devtools/client/themes/variables.css
@@ +194,5 @@
>  }
>  
> +:root.theme-firebug[platform="win"] {
> +  --theme-tab-toolbar-background: #d8eaf9 !important;
> +  --theme-toolbar-background: #f0f1f2 !important;

It'd be nice if you could change --theme-toolbar-background (and --theme-tab-toolbar-background if appropriate) to include the gradient.

@@ +196,5 @@
> +:root.theme-firebug[platform="win"] {
> +  --theme-tab-toolbar-background: #d8eaf9 !important;
> +  --theme-toolbar-background: #f0f1f2 !important;
> +  --theme-toolbar-tab-selected-background: rgb(247, 251, 254) !important;
> +  --theme-splitter-color: #aabccf !important;

You shouldn't need !important for all of these.

I suspect it might not work on Windows, because we might end up with stuff like:

color: green !important !important;

after variable substitution.
Attachment #8821459 - Flags: review?(ntim.bugs)
(Assignee)

Comment 17

2 years ago
Posted patch fix-1314919-3.patch (obsolete) — Splinter Review
Hey Tim,

Fixed as per your suggestions
- variable-ized backgrounds
- removed unwanted CSS cursor and font-size
Attachment #8821459 - Attachment is obsolete: true
Attachment #8822418 - Flags: review?(ntim.bugs)

Comment 18

2 years ago
Comment on attachment 8822418 [details] [diff] [review]
fix-1314919-3.patch

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

Other than these comments, I see a lot of vars are missed used/used out of context.

Can you ask :bgrins for your next review? I won't be available.

::: devtools/client/themes/common.css
@@ +517,5 @@
>  
> +.theme-firebug .menu-filter-button {
> +  background: var(--toolbarbutton-background) !important;
> +  color: var(--theme-body-color) !important;
> +  border: 1px solid transparent;

The point of these new variables is to avoid these .theme-firebug rules.

border: 1px solid transparent; should be set above with the .menu-filter-button rules.

I don't think color: var(--theme-body-color) makes a lot of difference, can you drop it ?

@@ +521,5 @@
> +  border: 1px solid transparent;
> +}
> +
> +.theme-firebug .menu-filter-button:hover {
> +  border-color: var(--toolbarbutton-border-color);

This rule should be with the .menu-filter-button:hover rules above

@@ +525,5 @@
> +  border-color: var(--toolbarbutton-border-color);
> +}
> +
> +.theme-firebug .menu-filter-button:not(:active).checked {
> +  background-image: linear-gradient(rgba(0, 0, 0, 0.1), transparent) !important;

I would expect this toolbar button state to be a variable as well.

For the default themes, the value of the var would be --theme-selection-background, while for the firebug theme it would be the gradient.

@@ +526,5 @@
> +}
> +
> +.theme-firebug .menu-filter-button:not(:active).checked {
> +  background-image: linear-gradient(rgba(0, 0, 0, 0.1), transparent) !important;
> +  border-color: var(--toolbarbutton-border-color);

Same.

::: devtools/client/themes/firebug-theme.css
@@ +165,4 @@
>  .theme-firebug .theme-toolbar,
>  .theme-firebug toolbar,
>  .theme-firebug .devtools-toolbar {
> +  border-bottom: 1px solid var(--theme-splitter-color) !important;

The border-bottom rule is a bug, it should be removed altogether since it causes a double border on the breadcrumbs.

@@ +165,5 @@
>  .theme-firebug .theme-toolbar,
>  .theme-firebug toolbar,
>  .theme-firebug .devtools-toolbar {
> +  border-bottom: 1px solid var(--theme-splitter-color) !important;
> +  background: var(--theme-tab-toolbar-background) !important;

Why theme-tab-toolbar-background ? shouldn't it be theme-toolbar-background ?
Attachment #8822418 - Flags: review?(ntim.bugs) → review-
(Assignee)

Comment 19

2 years ago
Posted patch fix-1314919-4.patch (obsolete) — Splinter Review
- Worked on suggestions given by :ntim
- Variablized hover and checked states of toolbarbutton
Attachment #8822418 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8825030 - Flags: review?(ntim.bugs)

Comment 20

2 years ago
Comment on attachment 8825030 [details] [diff] [review]
fix-1314919-4.patch

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

Looks great!

At devtools/client/themes/tooltips.css#353, can you change var(--theme-toolbar-background) to var(--theme-body-background) ? Otherwise the color property will no longer apply with the firebug theme.

::: devtools/client/themes/common.css
@@ +403,4 @@
>  .theme-firebug .devtools-button.checked,
>  .theme-firebug .devtools-toolbarbutton:not([label])[checked=true],
>  .theme-firebug .devtools-toolbarbutton:not([label])[open=true] {
> +  background: transparent linear-gradient(rgba(255, 255, 255, 0.4), rgba(255, 255, 255, 0.2)) no-repeat;

This makes the checked state almost invisible.

How about: var(--toolbarbutton-checked-background) ?

::: devtools/client/themes/variables.css
@@ +196,5 @@
> +  --toolbarbutton-background: transparent linear-gradient(rgba(255, 255, 255, 0.4), rgba(255, 255, 255, 0.2)) no-repeat;
> +  --toolbarbutton-hover-background: transparent;
> +  --toolbarbutton-checked-background: linear-gradient(rgba(0, 0, 0, 0.1), transparent);
> +  --toolbarbutton-checked-color: var(--theme-body-color);
> +  --toolbarbutton-hover-border: 1px solid silver;

I think var(--theme-splitter-color) is better than silver. It blends in better with the Windows firebug theme

@@ +201,5 @@
> +  --toolbarbutton-checked-border: var(--toolbarbutton-hover-border);
> +}
> +
> +:root.theme-firebug[platform="win"] {
> +  --theme-tab-toolbar-background: #d8eaf9 linear-gradient(rgba(255, 255, 255, 0.8), transparent);

According to the previous CSS, this should be:

 #d8eaf9 linear-gradient(rgba(253, 253, 253, 0.2), rgba(253, 253, 253, 0));

@@ +202,5 @@
> +}
> +
> +:root.theme-firebug[platform="win"] {
> +  --theme-tab-toolbar-background: #d8eaf9 linear-gradient(rgba(255, 255, 255, 0.8), transparent);
> +  --theme-toolbar-background: #f0f1f2 linear-gradient(rgba(255, 255, 255, 0.8), transparent);

And this should be:
 #d8eaf9 linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2))

::: devtools/client/themes/widgets.css
@@ +408,4 @@
>  .theme-firebug .breadcrumbs-widget-container .scrollbutton-down {
>    padding: 0;
>    box-shadow: none;
> +  outline: 1px solid silver;

I think var(--theme-splitter-color) is better than silver. It blends in better with the Windows firebug theme.
Attachment #8825030 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 21

2 years ago
Posted patch fix-1314919-5.patch (obsolete) — Splinter Review
- Worked on :ntim 's suggestions
Attachment #8825030 - Attachment is obsolete: true
Attachment #8827339 - Flags: review?(ntim.bugs)
(Assignee)

Comment 22

2 years ago
Posted image json-view.png
Hey Tim,

When we changed ...
devtools/client/themes/variables.css
...
:root.theme-firebug[platform="win"] {
--theme-tab-toolbar-background: #d8eaf9 linear-gradient(rgba(253, 253, 253, 0.2), rgba(253, 253, 253, 0));
...

The JSON View's tabs are affected, Hence in the attachment#8827339 [details] [diff] [review] , I have not used that suggestion.

Please let me know if that is OK.

- Ruturaj
(Assignee)

Comment 23

2 years ago
Comment on attachment 8827344 [details]
json-view.png

Forgot to ni
Flags: needinfo?(ntim.bugs)

Comment 24

2 years ago
Comment on attachment 8827339 [details] [diff] [review]
fix-1314919-5.patch

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

::: devtools/client/themes/variables.css
@@ +202,5 @@
> +}
> +
> +:root.theme-firebug[platform="win"] {
> +  --theme-tab-toolbar-background: #d8eaf9 linear-gradient(rgba(255, 255, 255, 0.8), transparent);
> +  --theme-toolbar-background: #d8eaf9 linear-gradient(rgba(255, 255, 255, 0.8), rgba(255, 255, 255, 0.2));

The JSON viewer issue was due to the JSON viewer using the incorrect variables.
I'm going to publish a new patch fixing that.
Attachment #8827339 - Flags: review?(ntim.bugs) → review+

Comment 25

2 years ago
Posted patch fix-1314919-6.patch (obsolete) — Splinter Review
Flags: needinfo?(ntim.bugs)
Attachment #8827587 - Flags: review+

Comment 26

2 years ago
Ruturaj, here's a diff of the changes I've done: https://bugzilla.mozilla.org/attachment.cgi?oldid=8827339&action=interdiff&newid=8827587&headers=1

Let me know if you have any questions/comments before I land the patch.
Flags: needinfo?(ruturaj)
(Assignee)

Comment 27

2 years ago
Posted patch fix-1314919-7.patch (obsolete) — Splinter Review
Hey Tim,
Thanks for the big rewrite !

I applied your patch using "git apply" on master@cc67c49 . It worked like charm.

The thing that was off, was network tab's side panel's tab padding, I've fixed. Please see the subsequent attachment.
Flags: needinfo?(ruturaj)
Attachment #8827777 - Flags: review?(ntim.bugs)
(Assignee)

Comment 28

2 years ago
Small fix added on your patch

Comment 29

2 years ago
Posted image before-after-mac.png
Unfortunately, the fix doesn't work well on OSX.

Comment 30

2 years ago
Comment on attachment 8827777 [details] [diff] [review]
fix-1314919-7.patch

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

::: devtools/client/themes/firebug-theme.css
@@ +97,4 @@
>  
>  .theme-firebug .devtools-sidebar-tabs tab {
>    margin: 3px 0 -1px 0;
> +  padding: 4px 8px;

I think simply removing this line works great as fix. Since there's another padding rule set above (that includes more side-padding).

It doesn't give as much padding as your screenshot, but it does look better than before without breaking OSX.
Attachment #8827777 - Flags: review?(ntim.bugs) → review+

Comment 31

2 years ago
Can you test my suggestion in comment 30 ? Thanks!
Flags: needinfo?(ruturaj)
(Assignee)

Comment 32

2 years ago
Hey Tim,

Removing "padding: 4px 8px;" line from firebug-theme.css works on Linux, If that works well on OSX (without creating bottom border line on tab) then we are fine, I can't say how it'll affect Windows

I found another problem, on Storage columns. The patch some created an issue, The sorting arrows weren't loading. I've added a small fix for that please see if that is OK

Thanks
Flags: needinfo?(ruturaj)
Attachment #8827916 - Flags: review?(ntim.bugs)
(Assignee)

Comment 33

2 years ago
The patch created some issue*

The fix is on devtools/client/themes/widgets.css

Comment 34

2 years ago
Comment on attachment 8827916 [details] [diff] [review]
fix-1314919-8.patch

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

Thanks!
Attachment #8827916 - Flags: review?(ntim.bugs) → review+

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Attachment #8827777 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8827587 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8827339 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
Thanks a lot Tim.
This patch appears to have turned browser_jsonview_filter.js permafail on some platform: https://treeherder.mozilla.org/logviewer.html#?job_id=70117090&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/98ee7f4c3b1a
Flags: needinfo?(ruturaj)
Flags: needinfo?(ntim.bugs)

Comment 39

2 years ago
(In reply to Wes Kocher (:KWierso) from comment #37)
> This patch appears to have turned browser_jsonview_filter.js permafail on
> some platform:
> https://treeherder.mozilla.org/logviewer.html#?job_id=70117090&repo=mozilla-
> inbound
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/98ee7f4c3b1a

Fixed and relanded.
Flags: needinfo?(ruturaj)
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 40

2 years ago
Hey Tim,

If you have time, can u explain me...

- how did HTML / CSS affect timeout in a test ?
- what fix you made to it?

Thanks
Flags: needinfo?(ntim.bugs)

Comment 41

2 years ago
(In reply to Ruturaj Vartak from comment #40)
> - how did HTML / CSS affect timeout in a test ?
This change caused the timeout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb91cd2e2fc#l2.13
The test kept looking for .searchBox, but didn't find it, so it timed out.

> - what fix you made to it?

I've added searchBox to the class names. Although changing searchBox to devtools-filterinput in the test would have been a valid fix as well.
Flags: needinfo?(ntim.bugs)

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40f64896c095
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Comment 43

2 years ago
thanks for explanation Tim.

Updated

2 years ago
Duplicate of this bug: 1268494

Updated

2 years ago
Summary: Layout problems in Firebug theme → Adjust Firebug theme for OSX and Linux to be gray instead of blue

Comment 45

2 years ago
I have reproduced the bug in Nightly 52.0a1(2016-11-03) with the instruction from comment 0 and Linux 64 Bit

Bug is fixed now on Latest Beta 53.0b7 (Build ID:20170327151342)
User Agent:Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170329]

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.