941579 - Update the toolbox tabs design according to shorlander's mockups

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Comment 1

[Albert Juhé]

Attachment: Patch 1

Here I attach the first version of the patch. I need your feedback/help in some parts:

1. In order to make the left and right border from the active tab disappear, I changed "border-right" to "border-left" everywhere, then with css I could select the active tab and the next one and set border-color to transparent. The problem with this approach is shown in the screenshot, there is like a blue line on the left of the active tab (actually, it is the transparent border). Setting border-width to 0 doesn't solve the problem because then it produces the size of the tab to change when you select it. I thought that maybe the best solution would be to use background-image to fake a border, but I wanted to know your opinion first.

2. In the CSS file there is this selector:
.devtools-tab:not([selected=true]).highlighted {...}
but I don't know when the "highlighted" class is used.

Comment 2

[Albert Juhé]

Attachment: Screenshot patch 1.png

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #1)
> Created attachment 8337820 [details] [diff] [review]
> Patch 1
> 
> Here I attach the first version of the patch. I need your feedback/help in
> some parts:
> 
> 1. In order to make the left and right border from the active tab disappear,
> I changed "border-right" to "border-left" everywhere, then with css I could
> select the active tab and the next one and set border-color to transparent.
> The problem with this approach is shown in the screenshot, there is like a
> blue line on the left of the active tab (actually, it is the transparent
> border). Setting border-width to 0 doesn't solve the problem because then it
> produces the size of the tab to change when you select it. I thought that
> maybe the best solution would be to use background-image to fake a border,
> but I wanted to know your opinion first.

There are a few different ways around this problem.  The easiest is probably to set border-width to 0 and add 1 extra px padding to the left and the right.  This will hide the border and prevent the size from collapsing.

Comment 4

[Albert Juhé]

Attachment: Screenshot padding in tabs

(In reply to Brian Grinstead [:bgrins] from comment #3)
> There are a few different ways around this problem.  The easiest is probably
> to set border-width to 0 and add 1 extra px padding to the left and the
> right.  This will hide the border and prevent the size from collapsing.

Sorry not to mention, I've already tried this and doesn't work. When setting a padding it makes the space for the text smaller but doesn't change the tab size (see screenshot). I also tried setting -moz-box-sizing: content-box; but it doesn't work either.

What does seem to work is margin, but it doesn't solve the problem.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8337820 [details] [diff] [review]
Patch 1

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

::: browser/themes/linux/devtools/toolbox.css
@@ +199,3 @@
>    color: #f5f7fa;
> +  background-color: #1d4f73;
> +  border-color: transparent;

> Sorry not to mention, I've already tried this and doesn't work. When setting
> a padding it makes the space for the text smaller but doesn't change the tab
> size (see screenshot). I also tried setting -moz-box-sizing: content-box;
> but it doesn't work either.
> 
> What does seem to work is margin, but it doesn't solve the problem.

Try replacing border-color: transparent here with 

   border-width: 0;
   padding-left: 1px;

This seems to get the desired effect in my testing (no size changes, gets rid of border).  I was testing by setting the border-left width and padding left to 10px so that any changes should be been noticeable, but let me know if this still isn't working.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: highlighted-tab-debugger.png

(In reply to Albert Juhé from comment #1)

> 2. In the CSS file there is this selector:
> .devtools-tab:not([selected=true]).highlighted {...}
> but I don't know when the "highlighted" class is used.

This selector is used in the debugger when you are stopped at a breakpoint and switch to a different tab.

Comment 7

[Albert Juhé]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Try replacing border-color: transparent here with 
> 
>    border-width: 0;
>    padding-left: 1px;
> 
> This seems to get the desired effect in my testing (no size changes, gets
> rid of border).  I was testing by setting the border-left width and padding
> left to 10px so that any changes should be been noticeable, but let me know
> if this still isn't working.

Yes, this works! There is only one detail that is still not working properly: when you click on a tab, the blue color covers from the left separator (included) until the right separator (not included). So it looks like the right separator "disappears" while the left changes its color.

I tried to set padding-left and padding-right in the active tab and no padding in the next tab, to make the active tab to occupy one more pixel, but this makes the tabs change their size in some cases when clicked. I think this is caused by

> .devtools-tab {... max-width: 127px; ...}

but I couldn't find how to solve it. Tomorrow I will take another look at this.

(In reply to Brian Grinstead [:bgrins] from comment #6)
> This selector is used in the debugger when you are stopped at a breakpoint
> and switch to a different tab.

Thank you! In the mockups I couldn't find how it should look, any advice?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > Try replacing border-color: transparent here with 
> > 
> >    border-width: 0;
> >    padding-left: 1px;
> > 
> > This seems to get the desired effect in my testing (no size changes, gets
> > rid of border).  I was testing by setting the border-left width and padding
> > left to 10px so that any changes should be been noticeable, but let me know
> > if this still isn't working.
> 
> Yes, this works! There is only one detail that is still not working
> properly: when you click on a tab, the blue color covers from the left
> separator (included) until the right separator (not included). So it looks
> like the right separator "disappears" while the left changes its color.
> 
> I tried to set padding-left and padding-right in the active tab and no
> padding in the next tab, to make the active tab to occupy one more pixel,
> but this makes the tabs change their size in some cases when clicked. I
> think this is caused by
> 
> > .devtools-tab {... max-width: 127px; ...}
> 
> but I couldn't find how to solve it. Tomorrow I will take another look at
> this.

I'm not sure I understand exactly the problem - are you saying that you want the top border of the selected tab to extend one extra px to the right?

> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > This selector is used in the debugger when you are stopped at a breakpoint
> > and switch to a different tab.
> 
> Thank you! In the mockups I couldn't find how it should look, any advice?

I'd say just stick with the existing background color / icon for the tab, but maybe make the top green border at the top more solid, like the active tabs are in your patch.

Comment 9

[Paul Rouget [:paul]]

Comment on attachment 8337820 [details] [diff] [review]
Patch 1

Albert, I won't have time to help you here. And apparently, Brian is already looking at your code. Please ask Brian for review/feedback.

(and come by IRC!)

Comment 10

[Brian Grinstead [:bgrins]]

Albert,
I've checked it out and it looks good.  Can you upload the latest version of the patch once it is ready for further review?

Comment 11

[Albert Juhé]

Attachment: Patch 2

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Albert,
> I've checked it out and it looks good.  Can you upload the latest version of
> the patch once it is ready for further review?

Here it is.

Comment 12

[Albert Juhé]

Attachment: Screenshot patch 2.png

(In reply to Brian Grinstead [:bgrins] from comment #8)
>I'd say just stick with the existing background color / icon for the tab, but maybe make the top green border at the top more solid, like the active tabs are in your patch.

I removed the gradient but I set a green background (see screenshot). I am not sure if you meant that.

Comment 13

[Albert Juhé]

Attachment: separators.png

(In reply to Brian Grinstead [:bgrins] from comment #8)
>I'm not sure I understand exactly the problem - are you saying that you want the top border of the selected tab to extend one extra px to the right?

More or less. I attached a screenshot to make it clear. The problem is that the blue color covers the left separator but not the right one, which only "disappears".

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8338710 [details] [diff] [review]
Patch 2

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

> I removed the gradient but I set a green background (see screenshot). I am
> not sure if you meant that.

Yes, this looks good.

> More or less. I attached a screenshot to make it clear. The problem is that
> the blue color covers the left separator but not the right one, which only
> "disappears".

I see now, I think this can be fixed as described inline.

::: browser/themes/linux/devtools/toolbox.css
@@ +199,4 @@
>    color: #f5f7fa;
> +  background-color: #1d4f73;
> +  border-width: 0;
> +  padding-left: 1px;

I believe to fix the extra 1px issue you are describing, all you need to do is add a padding-right: 1px; next to the padding-left here.

@@ +205,5 @@
> +}
> +
> +.devtools-tab[selected=true] + .devtools-tab {
> +  border-left-width: 0;
> +  padding-left: 1px;

and remove the padding-left: 1px; here.

This will make the sibling tab shrink 1px, but will be pushed back into place with the 1px padding on the selected tab.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbox-tabs2.png

Darrin,
Can you take a look at this screenshot of the redesigned toolbox tabs compared to the new designs?  Disregarding the button positioning, which I think we are going to tackle separately, do you see anything else that we should update?

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbox-tabs2.png

Had forgotten to add the text for the hover state in the image.  Same questions as in Comment 15 though.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Comment on attachment 8338710 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 8338710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > I removed the gradient but I set a green background (see screenshot). I am
> > not sure if you meant that.
> 
> Yes, this looks good.
> 
> > More or less. I attached a screenshot to make it clear. The problem is that
> > the blue color covers the left separator but not the right one, which only
> > "disappears".
> 
> I see now, I think this can be fixed as described inline.
> 
> ::: browser/themes/linux/devtools/toolbox.css
> @@ +199,4 @@
> >    color: #f5f7fa;
> > +  background-color: #1d4f73;
> > +  border-width: 0;
> > +  padding-left: 1px;
> 
> I believe to fix the extra 1px issue you are describing, all you need to do
> is add a padding-right: 1px; next to the padding-left here.
> 
> @@ +205,5 @@
> > +}
> > +
> > +.devtools-tab[selected=true] + .devtools-tab {
> > +  border-left-width: 0;
> > +  padding-left: 1px;
> 
> and remove the padding-left: 1px; here.
> 
> This will make the sibling tab shrink 1px, but will be pushed back into
> place with the 1px padding on the selected tab.

Seems like this is working for expanding the background an extra px, but is causing text to center oddly with the 1 missing px.  I don't think this issue you point out is not a huge deal (I didn't notice it until you sent over the screenshots), but I will think about it a little bit more.  Then, once Bug 941673 lands  we will just need to copy over any changes into the shared toolbars file.

Comment 18

[Darrin Henein [:darrin]]

Comment on attachment 8338849 [details]
theme-toolbox-tabs2.png

(Comparing to https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail])

- seems the blue in the mockup is slightly darker than the blue used in the screenshot (I see #1A4666 in the mockup)
- the mockup has a *very light blue* top border (not white... I see #D7F1FF)
- the top border has a glow/light shadow effect, not sure of performance implications here
- the dividers between tabs seem darker in the mockup (I'm measuring #42484F in the mockup)

Not sure if these shades are supposed to be toolbox-wide (especially the darker blue), I will ask shorlander.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Darrin Henein [:darrin] from comment #18)

> - seems the blue in the mockup is slightly darker than the blue used in the
> screenshot (I see #1A4666 in the mockup)

The color being used in this patch, #1d4f73, is grabbed from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors as "Select Highlight Blue".  Should we update the doc to use the new color instead?

Comment 20

[Albert Juhé]

Attachment: patch3.patch

I made the changes in the colors and added a glow the shadow effect in the active tab.

Comment 21

[Albert Juhé]

Attachment: patch3screenshot.png

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8339372 [details]
patch3screenshot.png

Darrin,
Can you take a look at this updated screenshot and see if there are more UI changes needed?

Comment 23

[Darrin Henein [:darrin]]

Comment on attachment 8339372 [details]
patch3screenshot.png

The only difference I see is the tab color (now correct) and the glow (see below).

- the selected tab should have a *very light blue* top border (not white... please use #D7F1FF)
- the shadow/glow should be less intense... not sure how I can give you a spec here (suggestions?)
- the dividers between tabs should be  #42484F (they still appear too bright)

Let me know if there are any questions!

Comment 24

[Albert Juhé]

Attachment: patch4.patch

I attached a patch with the changes proposed in comment 23 and taking into account the changes produced by bug 941673.

There is one thing I would like to advise. For specificity reasons I added some awkward selectors where I could use !important.

It is between line 517 and 531. This code:

> .devtools-tab[selected=true],
> .devtools-tab[selected=true]:hover:active {
>   color: #f5f7fa;
>   background-color: #1a4666;
>   border-width: 0;
>   padding-left: 1px;
>   box-shadow: 0 2px 0 #d7f1ff inset,
>               0 8px 3px -5px #2b82bf inset,
>               0 -2px 0 rgba(0,0,0,.2) inset;
> }
> 
> .devtools-tab[selected=true]:first-child,
> .devtools-tab[selected=true]:first-child:hover:active {
>   padding-left: 0;
> }

Could be replaced by this one:

> .devtools-tab[selected=true] {
>   color: #f5f7fa !important;
>   background-color: #1a4666 !important;
>   border-width: 0;
>   padding-left: 1px;
>   box-shadow: 0 2px 0 #d7f1ff inset,
>               0 8px 3px -5px #2b82bf inset,
>               0 -2px 0 rgba(0,0,0,.2) inset;
> }
> 
> .devtools-tab[selected=true]:first-child {
>   padding-left: 0;
> }

I don't know which approach is better, usually I try to avoid !important as much as possible, but in this case the code relying on !important is much simpler.

Comment 25

[Albert Juhé]

Attachment: patch4screenshot.png

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8343330 [details]
patch4screenshot.png

Darrin, see anything that should be updated based on this latest screenshot?

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #24)
> Created attachment 8343327 [details] [diff] [review]
> patch4.patch
> 
> I attached a patch with the changes proposed in comment 23 and taking into
> account the changes produced by bug 941673.
> 
> There is one thing I would like to advise. For specificity reasons I added
> some awkward selectors where I could use !important.
> 
> It is between line 517 and 531. This code:
> 
> > .devtools-tab[selected=true],
> > .devtools-tab[selected=true]:hover:active {
> >   color: #f5f7fa;
> >   background-color: #1a4666;
> >   border-width: 0;
> >   padding-left: 1px;
> >   box-shadow: 0 2px 0 #d7f1ff inset,
> >               0 8px 3px -5px #2b82bf inset,
> >               0 -2px 0 rgba(0,0,0,.2) inset;
> > }
> > 
> > .devtools-tab[selected=true]:first-child,
> > .devtools-tab[selected=true]:first-child:hover:active {
> >   padding-left: 0;
> > }
> 
> Could be replaced by this one:
> 
> > .devtools-tab[selected=true] {
> >   color: #f5f7fa !important;
> >   background-color: #1a4666 !important;
> >   border-width: 0;
> >   padding-left: 1px;
> >   box-shadow: 0 2px 0 #d7f1ff inset,
> >               0 8px 3px -5px #2b82bf inset,
> >               0 -2px 0 rgba(0,0,0,.2) inset;
> > }
> > 
> > .devtools-tab[selected=true]:first-child {
> >   padding-left: 0;
> > }
> 
> I don't know which approach is better, usually I try to avoid !important as
> much as possible, but in this case the code relying on !important is much
> simpler.

I think you are right to avoid !important here.  In this case, you are basically wanting a rule where the selector has one attribute (.devtools-tab[selected=true]) to have a higher priority over another where the selector has two pseudo classes (.devtools-tab:hover:active).  Here are two ways you could do this:

1) you could prefix the selector with a parent element's ID, like this:

> #toolbox-tabs .devtools-tab[selected=true] {
>   color: #f5f7fa;
>   background-color: #1a4666;
>   border-width: 0;
>   padding-left: 1px;
>   box-shadow: 0 2px 0 #d7f1ff inset,
>               0 8px 3px -5px #2b82bf inset,
>               0 -2px 0 rgba(0,0,0,.2) inset;
> }
>
> .devtools-tab[selected=true]:first-child {
>   padding-left: 0;
> }

2) You could do what you are already doing - though you shouldn't need the additional selector on the second rule since padding isn't explicitly set by the original .devtools-tab:hover:active rule.

> .devtools-tab[selected=true],
> .devtools-tab[selected=true]:hover:active {
>   color: #f5f7fa;
>   background-color: #1a4666;
>   border-width: 0;
>   padding-left: 1px;
>   box-shadow: 0 2px 0 #d7f1ff inset,
>               0 8px 3px -5px #2b82bf inset,
>               0 -2px 0 rgba(0,0,0,.2) inset;
> }
>
> .devtools-tab[selected=true]:first-child {
>   padding-left: 0;
> }

In this case I would probably just do 2, since it is a pretty simple case and easy to read and figure out.

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8343327 [details] [diff] [review]
patch4.patch

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

The CSS looks good to me.  I think you can either leave things as they are for this [selected=true]:active:hover stuff, or could modify it a bit as noted.  Awaiting UI review from Darrin, but I think the patch will be ready if you make the minor change(s) noted.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +510,2 @@
>    color: #ced3d9;
>  }

Could you add a new line here for whitespace consistency?  The current file is missing it.

@@ +525,5 @@
> +              0 -2px 0 rgba(0,0,0,.2) inset;
> +}
> +
> +.devtools-tab[selected=true]:first-child,
> +.devtools-tab[selected=true]:first-child:hover:active {

I mentioned in Comment 27 that the .devtools-tab[selected=true]:first-child:hover:active could be removed from this selector.  However, looking at this selector more closely, that may still not apply because [selected=true]:hover:active will still take priority over [selected=true]:first-child.  In this case, you could either keep things as they are, or remove the padding-left: 1px from the rule above and do this:

.devtools-tab[selected=true]:not(:first-child) {
  padding-left: 1px;
}

The selector is more complicated, but it gets rid of the duplication.  I think either way is OK.

Comment 29

[Albert Juhé]

Attachment: patch5.patch

I applied your first suggestion from comment 27 and the suggestions from comment 28.

I don't upload a new screenshot as it looks the same than patch 4.

Comment 30

[Darrin Henein [:darrin]]

Comment on attachment 8343330 [details]
patch4screenshot.png

This looks much better. Not sure if shorlander has new icons to drop in... the original mocks show a brighter icon than in this screenshot.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8343643 [details] [diff] [review]
patch5.patch

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

This code looks good, let's go ahead and land it (you can set the checkin-needed keyword).  There is something else we need to do as a follow up, Bug 947309.  Since it requires reorganizing some of the file/folder layout for the themes I think it is best done separately.

Comment 32

[Tim Nguyen :ntim]

Albert, please make sure to make the selected tab icon at full opacity as shown in the mockups (or is it the linux graphics ?)

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to ntim007 from comment #32)
> Albert, please make sure to make the selected tab icon at full opacity as
> shown in the mockups (or is it the linux graphics ?)

I believe this was a bug in the Linux styles that got copied into the shared file.  Let's go ahead and handle this in Bug 947346, since we will be handling regressions from that shared toolbar bug there.  Here is part of a patch that fixes this: https://bugzilla.mozilla.org/attachment.cgi?id=8343971&action=diff#a/browser/themes/shared/devtools/toolbars.inc.css_sec2.

Comment 34

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/5b91fd1ed7d6

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b91fd1ed7d6

Comment 36

[Victor Porof [:vporof][:vp]]

There are a few small UI oddities, at least on OS X, that should be taken care of. I filed bug 947708 and bug 947709.

Comment 37

[Saheda Reza [:Antora]]

Windows 8.1(32bit)

I found this issue,just like this attachment..https://bug941579.bmoattachments.org/attachment.cgi?id=8338721

on Firefox Nightly

Build ID	20131121030201
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:28.0) Gecko/20100101 Firefox/28.0

It's okay ,Latest Nightly Latest Beta and Latest Developer Edition

Firefox Latest nightly

Build ID 	20150806030208
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0

Latest Developer Edition

Build ID 	20150806004006
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0

Latest Beta

Build ID 	20150730171029
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:40.0) Gecko/20100101 Firefox/40.0

 BUT..

According this attachment,I saw that issue also happens on Latest Nightly,Latest Beta and Latest Developer Edition. It showed a massage just like that link.

https://bug941579.bmoattachments.org/attachment.cgi?id=8337936-----isn't it bug???

Comment 38

[Tim Nguyen :ntim]

Thanks for verifying !

(In reply to Saheda Reza [:Antora] from comment #37)
> It showed a massage just like that link.
> 
> https://bug941579.bmoattachments.org/attachment.cgi?id=8337936-----isn't it
> bug???

This isn't a bug, just a warning we show when the debugger is paused.