775987 - Buttons in the Developer Toolbar should have a hover state

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

The buttons in the Developer Toolbar, Debugger and Inspector should all have hover states (at least on Linux and Windows).

We might not need new graphics if we can just tweak some of the background-gradient/background-color and maybe the border-color of the buttons.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This places a slight border and lightens to font color when the buttons in the developer toolbar are hovered. This doesn't cover other buttons within the developer tools though.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 646351 [details] [diff] [review]
Patch

You're implicitly changing the color for :hover:active as well, which seems unintended.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 4

[Paul Rouget [:paul]]

Jaws, can you please attach some screenshots (linux/windows) and ask for a ui-review (shorlander)?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Video of patch (Windows)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Video of patch (Linux)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 650419 [details] [diff] [review]
Patch v2

you can just use :hover:not(:active) in order to not affect the active state

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v4

I played some more with this patch yesterday and I think it looks better than the previous patch. Hover state now moves the text-shadow below the text by 2px (offset of 1px was too subtle), and brightens the text (maintaining a similar hue).

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Video of patch v2 (Windows)

This is a video of the new patch. I also forgot to mention in the previous comment that I no longer included the hover styles for open=true since I couldn't find something that looked good there.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: Hover state as similar to checked state without background

This patch copies the text shadow and text color from the checked state to the hovered state.
The opacity of text color and shadow color is reduced to 40% instead of the 60% in checked state to give it less importance than checked state. I think even 50% would do. (don't know if I made any sense there)

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 654287 [details] [diff] [review]
Hover state as similar to checked state without background

I tested this out but there's not enough contrast. It's too hard to read the button labels with the bright blue on a bluish/grey background.

See the "Debugger" text in this screenshot: http://screencast.com/t/l7gZ2jVR

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Optimizer's patch with adjusted text color

This is Optimizer's patch with an adjusted color that has higher contrast (and hopefully better readability). This uses the same blue as the icons next to the labels.

This is a video of the patch (SWF): http://screencast.com/t/NsGgpO2zpgLS

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: Hover state similar to checked state without box-shadow's depth effect

Almost similar to the edited patch submitted by Jared. Only difference is the use of hsl instead of rgb and change of opacity of text-shadow so that it appears to be more subtle. rgb to hsl change is done to be in consistent with the rest of the colors used in develoepr-toolbar-button class.

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: Screenshot of attachment 656037 in action

In the screenshot, Timeline is in checked state, and Web Console button is in hover state.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 655917 [details] [diff] [review]
Hover state similar to checked state without box-shadow's depth effect

I'll review this once the UI review is done.

Comment 17

[Paul Rouget [:paul]]

Is the text-shadow' "important" in gnomestripe needed?

So when you click on a button, it goes from "white" (not:checked) to "blue" (:hover) to "white" (:active) to "blue" (checked).

Is that what we want?

Comment 18

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #17)
> Is the text-shadow' "important" in gnomestripe needed?

I don't think they are needed. I applied the patch using an addon, so thats why I think I needed it for the text-shadow to work. But I was not sure.


> So when you click on a button, it goes from "white" (not:checked) to "blue"
> (:hover) to "white" (:active) to "blue" (checked).
> 
> Is that what we want?

I also thought that is a little off the flow.
What if the pressed down state also had the same text color as hover state?
That way it will go from 
white (not hovere) to blue(:hover) to blue(pressed) to blue(checked)

Comment 19

[Paul Rouget [:paul]]

I suggest:

color:blue (no shadow) on :hover
color:blue + background(very dark) on :hover:active (as it is right now, but color:blue)
color:blue + shadow + background(dark) (as it is right now)

This will involve some reordering in the selectors (try to avoid duplication of color values).

Comment 20

[Girish Sharma [:Optimizer]]

Attachment: Patch addressing Paul's comments.

Fixed the hover+active state to have blue text color.
Also changed the text color for the "More Tools" button in Open state to be consistent with the rest of the buttons.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 656037 [details] [diff] [review]
Patch addressing Paul's comments.

>+.developer-toolbar-button:active:hover,
>+.developer-toolbar-button:hover:not(:active):not([open=true]):not([checked=true]) {
>+  color: hsl(208,100%,60%) !important;
>+  text-shadow: 0 0 6px hsl(208,100%,60%);
>+}

:active:hover alongside :hover:not(:active) for the same style rule doesn't seem to make sense.

Comment 22

[Girish Sharma [:Optimizer]]

:active:hover is the state when the user has mousedown on the button, as in he is in the process to press it. This is done to have a continuous user flow, from white to blue on hover to blue on press down to blue on pressed. Without this, the press down state was still white.

:hover:not(:active):not([checked=true]) is the state when the user has pressed the button, and it is in checked state. We don't want the color and text shadow to become less intense upon hovering in this scenario. Maybe here :not(:active) can be traded off.

let me check..

yes, it can be removed. I will add the updated patch with the :not(:active) part removed from second line.

Comment 23

[Dão Gottwald [:dao]]

> :hover:not(:active):not([checked=true]) is the state when the user has
> pressed the button, and it is in checked state.

:not([checked=true]) certainly doesn't represent the checked state.

Comment 24

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.1

Ah, my bad. I must be day dreaming.

Yes, in fact its the opp state, when the button is not checked, but still , we can get rid of :not(:active) since I introduced :hover:active just above it.

So originally what Jared intended was to change color (not actially change color) only when the button is not checked or active.

But then Paul pointed out the break in user flow (white to blue to white to blue) so I added the :hover:active selectors to fix it, and then the :not(:active) became inactive.

Fixing that in this patch.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 663355 [details] [diff] [review]
Patch v1.1

>+.developer-toolbar-button:active:hover,
>+.developer-toolbar-button:hover:not([open=true]):not([checked=true]) {
>+  color: hsl(208,100%,60%) !important;
>+  text-shadow: 0 0 6px hsl(208,100%,60%);
>+}
>+
>+.developer-toolbar-button[checked=true],
>+.developer-toolbar-button[open=true] {
>   color: hsl(208,100%,60%) !important;
>   background: rgba(0,0,0,.4);
>   text-shadow: 0 0 6px hsl(208,100%,60%);

So you're adding :not([open=true]):not([checked=true]) only to set the same color and same text-shadow for [checked=true] and [open=true] a few lines below? This also doesn't seem to make sense.

Comment 26

[Girish Sharma [:Optimizer]]

Do you mean that these two can be clubbed together, and the fact that they are not, does not make sense.

Or you are referring to something else ?

Comment 27

[Dão Gottwald [:dao]]

It looks like the color and text-shadow could be set in one rule.

Comment 28

[Girish Sharma [:Optimizer]]

Okay, will do that.
What about the UI review ? Are we finally going with this hover effect ?

Comment 29

[Dão Gottwald [:dao]]

> What about the UI review ? Are we finally going with this hover effect ?

I don't know.

Comment 30

[Joe Walker [:jwalker]]

Optimizer, please could you upload the latest patch with Dão's comments addressed?
Brian, please could you give this the thumbs up for UX, or arrange for someone else to?

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: Patch with Dao's comments addressed

Combined the styles, :hover:active was not needed so removed it now.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 663433 [details] [diff] [review]
Patch with Dao's comments addressed

>+.developer-toolbar-button:hover:not([open=true]):not([checked=true]),
>+.developer-toolbar-button[checked=true],
>+.developer-toolbar-button[open=true] {
>   color: hsl(208,100%,60%) !important;
>+  text-shadow: 0 0 6px hsl(208,100%,60%);
>+}

Again, :not([open=true]):not([checked=true]) does not make sense when the same rules has selectors with [checked=true] and [open=true]. What you want here is ...:hover:active, ...[checked=true], ...[open=true].

Comment 33

[Girish Sharma [:Optimizer]]

Attachment: Fianl patch (hoping to be)

Done.
Thanks for your patience :)

Comment 34

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #32)
> What you want here is ...:hover:active, ...[checked=true], ...[open=true].

Err, this should have been :hover rather than :hover:active, but you already figured this out.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 663507 [details] [diff] [review]
Fianl patch (hoping to be)

>-.developer-toolbar-button[checked=true] {
>+.developer-toolbar-button[checked=true],
>+.developer-toolbar-button[open=true] {
>   background: rgba(0,0,0,.4);
>-  text-shadow: 0 0 6px hsl(208,100%,60%);
> }

This lighter background is only wanted for [checked=true]. [open=true] should keep the darker background that was set a few lines above, which would have been more obvious if you had generated this patch with eight lines of context by adding this to your ~/.hgrc file:

[diff]
git = 1
showfunc = 1
unified = 8

Comment 36

[Girish Sharma [:Optimizer]]

If you mean darker text-color on [open=true] selector, then it was done to maintain consistency with the other buttons on the developer toolbar.

All buttons (after this patch), will have a blue text color on hover, and also on mouse down, so will the "More Tools" button, but currently it has white text color when the popup is opened, so I had to change that to blue, to match other buttons in their checked state.

Comment 37

[Dão Gottwald [:dao]]

I was talking about the background color.

Comment 38

[Girish Sharma [:Optimizer]]

Attachment: dark background for open=true

Man I am so bad at this .. :|

Comment 39

[Stephen Horlander [:shorlander]]

Comment on attachment 655920 [details]
Screenshot of attachment 656037 [details] [diff] [review] in action

Looks good to me.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97b007a4a587

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97b007a4a587