Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[Affected versions]:
- Firefox 61.0.1, 62.0b9, 63.0a1 (2018-07-18), 52.8.0esr, 60.0.2esr

[Affected platforms]:
- win_10x64, Ubuntu 16.04LTS, macOS 10.13.4

[Steps to reproduce]:
1. Launch Firefox;
2. Open DevTools - Network tab;
3. Navigate to any webpage;
4. Click on any method listed;
5. Zoom/Resize the Requests detail pane until the headers are not fully visible and the triangle-picker button is displayed.

[Expected result]:
- on hover the button should have a background color change;
- on click the button/background color should change

[Actual result]:
- the button style is the same.

[Regression range]:
- not a regression;

[Additional notes]:
- attached screenshot with the issue;
- can be bumped down as an enhancement.

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

Thanks for the report!
I can reproduce it on my machine (Win10)

Indeed, the button would deserve some better styling.

Some info for anyone interested in fixing it:

1) The button is implemented here:
https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/devtools/client/shared/components/tabs/Tabs.js#329

2) Styles for the button are here:
https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/devtools/client/themes/common.css#750

3) We need more styling for `.all-tabs-menu:hover`


Honza

	Florens Verschelde :fvsch Assignee
	Comment 2 • 6 years ago

Hi, I’m interested. :)

	Florens Verschelde :fvsch Assignee
	Comment 3 • 6 years ago

The simplest possible fix is adding:

.all-tabs-menu:hover {
  background-color: var(--theme-toolbar-hover);
}

.all-tabs-menu:hover:active {
  background-color: var(--theme-toolbar-hover-active);
}

This is the hover color used for buttons and tabs in the same tab bar.

The :active style uses a color that is the same as the :hover one. So while there are 2 variables defined in variables.css and --theme-toolbar-hover-active is used in some places, including for tabs, it kinda looks like this style was dropped. Maybe the few uses of --theme-toolbar-hover-active in the codebase is dead code that should be removed instead?

Then there are two small issues:

1. Keyboard navigation: while it's possible to use the left/right arrow keys to switch tabs, the all-tabs-menu button is a div, so it can't be focused and used with a keyboard. Should it be changed to a button in `client/shared/components/tabs/Tabs.js`? (I tried it and, with a CSS fix or two it looks alright.)

2. Contrast: the button has already very poor contrast in the light theme, the central arrow is B6BABF (hardcoded in dropmarker.svg) on top of a #ededf0 background, a 1.67:1 ratio (minimum in WCAG2 is 4.5:1):
https://webaim.org/resources/contrastchecker/?fcolor=B6BABF&bcolor=EDEDF0
The contrast becomes even worse when we darken the background on :hover.
In the dark mode on the other hand, contrast is alright since #B6BABF is close to the text color.

My suggestion for the second issue would be to use a context-fill in the SVG icon (fill="context-fill #b6babf") and use it in the CSS, same as the 3-pane expand/collapse button in the Inspector.

.all-tabs-menu {
  ...
  -moz-context-properties: fill;
  fill: var(--theme-toolbar-photon-icon-color);
}

This leaves the dark theme mostly untouched, but makes the button much more contrasted in the light theme.

As a side note, maybe this part of themes/toolbars.css should be changed to use --theme-toolbar-photon-icon-color? (Both colors are identical or very close in dark theme, and it's maybe a bit less contrasted in light theme):

/*
 * Apply the default fill color to toolbar icons
 */
.devtools-toolbarbutton > image,
.devtools-button::before,
.scrollbutton-up > .toolbarbutton-icon,
.scrollbutton-down > .toolbarbutton-icon,
#black-boxed-message-button .button-icon,
#canvas-debugging-empty-notice-button .button-icon,
#toggle-breakpoints[checked] > image,
.event-tooltip-debugger-icon {
  -moz-context-properties: fill;
  fill: var(--theme-toolbar-color);
}

	Florens Verschelde :fvsch Assignee
	Comment 4 • 6 years ago

Last issue: other menu buttons with dropdowns retain the "active" style when the dropdown is visible. See devtools' top bar "chevron" overflow menu and the "ellipsis" menu. In light theme they get one step darker when "active". This is implemented with a button and [aria-pressed="true"] in CSS.

Should we try to reproduce that? It would be ideal, but I’m not sure if that easily doable with the current React implementation. We would need a way to know that "hey, that dropdown was dismissed".

	Florens Verschelde :fvsch Assignee
	Comment 5 • 6 years ago

Screenshot of current result with better contrast in light theme and hover style (second window).

	Florens Verschelde :fvsch Assignee
	Comment 6 • 6 years ago

Here's my implementation for:

1. Background color change on :hover (same as tabs)
2. Correct icon contrast in light theme
3. Using `button` instead of `div` to enable keyboard use (ideally it would need focus styles and a label, but maybe that's better than nothing?)

I’d like feedback on whether those changes are alright or scope-creepy (initial scope was #1 only). Maybe #3 should be dropped, but I'd like to keep #2.

Not sure who to ask for feedback or review.

	Julian Descottes [:jdescottes]
	Comment 7 • 6 years ago

Comment on attachment 8994163 [details] [diff] [review]
bug-1476902-all-tabs-menu.patch

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

Thanks for the patch!
You can just add r=jdescottes in the commit message and we should be able to land this.

On a sidenote we have the devtools-button class that we use to set hover backgrounds on other buttons. But it looks like here it would not be a good fit.

::: devtools/client/themes/common.css
@@ +763,5 @@
>    background-image: url("chrome://devtools/skin/images/dropmarker.svg");
>    background-repeat: no-repeat;
>    background-position: center;
> +  -moz-context-properties: fill;
> +  fill: var(--theme-toolbar-photon-icon-color);

Looks like the good approach, now this icon matches the color of other icons. 

Note that this dropmarker.svg is used in two other spots:
- iframe picker https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/devtools/client/themes/toolbox.css#307
- animation inspector https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/devtools/client/themes/animation.css#72

We could have a follow up bug to apply the same approach for them as well. We might want to share a CSS class to avoid duplicating the rules.

::: devtools/client/themes/images/dropmarker.svg
@@ +2,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="8" height="4" viewBox="0 0 8 4">
> +  <polygon points="0,0 4,4 8,0" fill="context-fill #b6babf"/>
> +</svg>

Was wondering why the diff was showing the whole file as updated. The previous file was using CRLF endings for some reason. Thanks for fixing it :)

	Florens Verschelde :fvsch Assignee
	Comment 8 • 6 years ago

Update patch with r=jdescottes in commit message

	Julian Descottes [:jdescottes]
	Comment 9 • 6 years ago

Thanks for updating the patch!

Pushed the changeset to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1d9f6b3ff56ad4fc1475cac21b77eb3dc2b1025 

This will run our test suite against your patch, to check we don't introduce any regression here.
Will push if try is green.

	Pulsebot
	Comment 10 • 6 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4ad6db1723
all-tabs-menu contrast and hover style; r=jdescottes

	Natalia Csoregi [:nataliaCs]
	Comment 11 • 6 years ago
bugherder

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

	Cristian Fogel [:cfogel] Reporter
	Comment 12 • 6 years ago

Looking good on Win10, macOS 10.13.4, Ubuntu16.0 with the latest Nightly.