flack Reporter
	Description • 8 years ago

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

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 1 • 8 years ago

Honza, any comments on this?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 2 • 8 years ago

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

	Ruturaj Vartak Assignee
	Comment 3 • 8 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)

	flack Reporter
	Comment 5 • 8 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

	Ruturaj Vartak Assignee
	Comment 6 • 8 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

	Ruturaj Vartak Assignee
	Comment 7 • 8 years ago

Tim, your views on the blue colored Firebug theme ?

	Tim Nguyen :ntim
	Comment 8 • 8 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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 9 • 8 years ago

(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

	Ruturaj Vartak Assignee
	Comment 10 • 8 years ago

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

	Ruturaj Vartak Assignee
	Comment 11 • 8 years ago

Firebug overall theme comparison

	Ruturaj Vartak Assignee
	Comment 12 • 8 years ago

Network panel, and sub-toolbar button updates..

	flack Reporter
	Comment 13 • 8 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)

	flack Reporter
	Comment 14 • 8 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)

	Ruturaj Vartak Assignee
	Comment 15 • 8 years ago

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.

	Tim Nguyen :ntim
	Comment 16 • 8 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.

	Ruturaj Vartak Assignee
	Comment 17 • 8 years ago

Hey Tim,

Fixed as per your suggestions
- variable-ized backgrounds
- removed unwanted CSS cursor and font-size

	Tim Nguyen :ntim
	Comment 18 • 8 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 ?

	Ruturaj Vartak Assignee
	Comment 19 • 8 years ago

- Worked on suggestions given by :ntim
- Variablized hover and checked states of toolbarbutton

	Tim Nguyen :ntim
	Comment 20 • 8 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.

	Ruturaj Vartak Assignee
	Comment 21 • 8 years ago

- Worked on :ntim 's suggestions

	Ruturaj Vartak Assignee
	Comment 22 • 8 years ago

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

	Ruturaj Vartak Assignee
	Comment 23 • 8 years ago

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

Forgot to ni

	Tim Nguyen :ntim
	Comment 24 • 8 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.

	Tim Nguyen :ntim
	Comment 26 • 8 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.

	Ruturaj Vartak Assignee
	Comment 27 • 8 years ago

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.

	Ruturaj Vartak Assignee
	Comment 28 • 8 years ago

Small fix added on your patch

	Tim Nguyen :ntim
	Comment 29 • 8 years ago

Unfortunately, the fix doesn't work well on OSX.

	Tim Nguyen :ntim
	Comment 30 • 8 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.

	Tim Nguyen :ntim
	Comment 31 • 8 years ago

Can you test my suggestion in comment 30 ? Thanks!

	Ruturaj Vartak Assignee
	Comment 32 • 8 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

	Ruturaj Vartak Assignee
	Comment 33 • 8 years ago

The patch created some issue*

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

	Tim Nguyen :ntim
	Comment 34 • 8 years ago

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

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

Thanks!

	Ruturaj Vartak Assignee
	Comment 35 • 8 years ago

Thanks a lot Tim.

	Pulsebot
	Comment 36 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb91cd2e2fc
Fix Firebug layout problems. r=ntim

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 37 • 8 years ago

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

	Pulsebot
	Comment 38 • 8 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f64896c095
Firebug layout problems. r=ntim.

	Tim Nguyen :ntim
	Comment 39 • 8 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.

	Ruturaj Vartak Assignee
	Comment 40 • 8 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

	Tim Nguyen :ntim
	Comment 41 • 8 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.

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/40f64896c095

	Ruturaj Vartak Assignee
	Comment 43 • 8 years ago

thanks for explanation Tim.

	Forhad Hossain
	Comment 45 • 7 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]