1270990 - Dark theme refresh issues

Status: VERIFIED FIXED

(DevTools :: Framework, defect, P1)

Description

[Tim Nguyen :ntim]

Attachment: dark-theme-issues-1.png

Here are the issues I've found:
- the tabbar has a 2px bottom border
- the debugger toolbar has 2 different bottom border colors
- the icon highlighted state (debugger paused, memory/perf active) doesn't match the tab top border

Comment 1

[Tim Nguyen :ntim]

Attachment: icon-highlighted-state.png

Comment 2

[Brian Grinstead [:bgrins]]

P1, since these were introduced in 49

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: dark-theme-issues.patch

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8750308 [details] [diff] [review]
dark-theme-issues.patch

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

Code changes work for me, but please wait for Tim's confirmation that this fixes all issues before landing

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: dark-theme-issues.patch

Oh no, sorry—one last small code change brought up in bug 1079135.

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8750388 [details] [diff] [review]
dark-theme-issues.patch

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

Looks like tool-memory-active.svg hasn't been updated with the new green colour.
But otherwise, looks good to me! Thanks !

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8750388 [details] [diff] [review]
dark-theme-issues.patch

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

::: devtools/client/themes/variables.css
@@ +73,4 @@
>    --theme-tab-toolbar-background: #272b35;
>    --theme-toolbar-background: #272b35;
>    --theme-selection-background: #5675B9;
> +  --theme-selection-background-semitransparent: rgba(23, 51, 71, 0.5);

This change makes the selection background much much less obvious.

It's hard to see when the console buttons are checked now.
Perhaps we could find a middle-ground colour between the old and this one ?

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8750388 [details] [diff] [review]
dark-theme-issues.patch

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

::: devtools/client/themes/variables.css
@@ +73,4 @@
>    --theme-tab-toolbar-background: #272b35;
>    --theme-toolbar-background: #272b35;
>    --theme-selection-background: #5675B9;
> +  --theme-selection-background-semitransparent: rgba(23, 51, 71, 0.5);

What is the reason for this change?

Just FWIW, the original intent for the difference between --theme-selection-background and --theme-selection-background-semitransparent was that the latter would have the same RGB values but with some alpha applied

Comment 10

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> ::: devtools/client/themes/variables.css
> @@ +73,4 @@
> >    --theme-tab-toolbar-background: #272b35;
> >    --theme-toolbar-background: #272b35;
> >    --theme-selection-background: #5675B9;
> > +  --theme-selection-background-semitransparent: rgba(23, 51, 71, 0.5);
> 
> What is the reason for this change?
A combo of things. One was that I realized I hadn't captured part of my original designs, so I couldn't argue that the semi-transparent we were using passed AA: http://cl.ly/343f0O0F1W1Z

The second was getting ni?'d on a (old) bug that addressed text and contrast on the old theme, which made me realize the error: bug 1079135

> Just FWIW, the original intent for the difference between
> --theme-selection-background and
> --theme-selection-background-semitransparent was that the latter would have
> the same RGB values but with some alpha applied

Should I go about and rename the variable then, do you think, knowing the above?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > ::: devtools/client/themes/variables.css
> > @@ +73,4 @@
> > >    --theme-tab-toolbar-background: #272b35;
> > >    --theme-toolbar-background: #272b35;
> > >    --theme-selection-background: #5675B9;
> > > +  --theme-selection-background-semitransparent: rgba(23, 51, 71, 0.5);
> > 
> > What is the reason for this change?
> A combo of things. One was that I realized I hadn't captured part of my
> original designs, so I couldn't argue that the semi-transparent we were
> using passed AA: http://cl.ly/343f0O0F1W1Z
> 
> The second was getting ni?'d on a (old) bug that addressed text and contrast
> on the old theme, which made me realize the error: bug 1079135

According to Tim's comment 8, the new color is less obvious.  In particular with the console buttons, so please take a look at those before marking for checkin.  I'm fine either way if you want to tweak the alpha value of the current RGB to get better text contrast or use this new color.

> > Just FWIW, the original intent for the difference between
> > --theme-selection-background and
> > --theme-selection-background-semitransparent was that the latter would have
> > the same RGB values but with some alpha applied
> 
> Should I go about and rename the variable then, do you think, knowing the
> above?

No, I wouldn't rename it either way.

Comment 13

[Helen V. Holmes (:helenvholmes) (:✨)]

Copying some issues discussed over IRC and in bug 1271054: 

- The contrast between the white and purple colors in the syntax highlighting might need to be revisited.
- The red and green both have really high contrast to the point of being a little glaring.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8750388 [details] [diff] [review]
dark-theme-issues.patch

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

Clearing review based on Comments 11 and 13

Comment 15

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: dark-theme-issues.patch

Rolled back the semi-transparent change and updated the active color for the memory tool.

Brian, re: comment 13; would you mind if we addressed those in a separate bug? I think I want to use the devtools in dark theme for a week or so before adjusting it (I'm not sure if it's just the newness that makes me want to change it again so quickly or what.)

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #15)
> Brian, re: comment 13; would you mind if we addressed those in a separate
> bug? I think I want to use the devtools in dark theme for a week or so
> before adjusting it (I'm not sure if it's just the newness that makes me
> want to change it again so quickly or what.)

Works for me, but can you please file a follow up for tweaking the colors?  If you ultimately decide to not change them we can wontfix it, but at least that way the things in Comment 13 will be tracked.

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

See Bug 1273130.

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f3c3592949ad

Comment 20

[Azmina]

I have reproduced this bug with Firefox Nightly 49.0a1 (2016-05-06) on windows 8.1 , 64 bit

The bug's fix is verified on Aurora 49.0a2

Build ID : 20160709004037

User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160713]