Closed Bug 775987 Opened 8 years ago Closed 8 years ago

Buttons in the Developer Toolbar should have a hover state

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: jaws, Assigned: Optimizer)

References

Details

Attachments

(2 files, 14 obsolete files)

12.39 KB, image/png
shorlander
: ui-review+
Details
2.29 KB, patch
dao
: review+
Details | Diff | Splinter Review
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #646351 - Flags: review?(paul)
Comment on attachment 646351 [details] [diff] [review]
Patch

You're implicitly changing the color for :hover:active as well, which seems unintended.
Attachment #646351 - Flags: review?(shorlander)
Attachment #646351 - Flags: review?(paul)
Attachment #646351 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #646351 - Attachment is obsolete: true
Attachment #650419 - Flags: review?(dao)
Jaws, can you please attach some screenshots (linux/windows) and ask for a ui-review (shorlander)?
Attached video Video of patch (Windows) (obsolete) —
Attachment #650721 - Flags: ui-review?(shorlander)
Attachment #650721 - Attachment is patch: false
Attachment #650721 - Attachment mime type: text/plain → video/ogg
Comment on attachment 650419 [details] [diff] [review]
Patch v2

you can just use :hover:not(:active) in order to not affect the active state
Attachment #650419 - Flags: review?(dao) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #650419 - Attachment is obsolete: true
Attachment #652497 - Flags: review?(dao)
Attached patch Patch v4 (obsolete) — Splinter Review
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).
Attachment #652497 - Attachment is obsolete: true
Attachment #652497 - Flags: review?(dao)
Attachment #654257 - Flags: review?(dao)
Attached video Video of patch v2 (Windows) (obsolete) —
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.
Attachment #650721 - Attachment is obsolete: true
Attachment #650723 - Attachment is obsolete: true
Attachment #650721 - Flags: ui-review?(shorlander)
Attachment #654259 - Flags: ui-review?(shorlander)
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)
Attachment #654287 - Flags: feedback?(jaws)
Attachment #654287 - Flags: feedback?(dao)
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
Attachment #654287 - Flags: feedback?(jaws)
Attachment #654287 - Flags: feedback?(dao)
Attachment #654287 - Flags: feedback-
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
Attachment #654287 - Attachment is obsolete: true
Attachment #655783 - Flags: review?(shorlander)
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.
Attachment #655783 - Attachment is obsolete: true
Attachment #655783 - Flags: review?(shorlander)
Attachment #655917 - Flags: review?(shorlander)
In the screenshot, Timeline is in checked state, and Web Console button is in hover state.
Attachment #655920 - Flags: ui-review?(shorlander)
Attachment #655920 - Attachment description: Screenshot of attachment 655914 in action → Screenshot of attachment 655917 in action
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.
Attachment #655917 - Flags: review?(shorlander) → review?(dao)
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?
(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)
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).
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.
Attachment #655917 - Attachment is obsolete: true
Attachment #655917 - Flags: review?(dao)
Attachment #656037 - Flags: review?(dao)
Attachment #655920 - Attachment description: Screenshot of attachment 655917 in action → Screenshot of attachment 656037 in action
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.
Attachment #656037 - Flags: review?(dao) → review-
: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.
> :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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #656037 - Attachment is obsolete: true
Attachment #663355 - Flags: review?(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.
Attachment #663355 - Flags: review?(dao) → review-
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 ?
It looks like the color and text-shadow could be set in one rule.
Okay, will do that.
What about the UI review ? Are we finally going with this hover effect ?
> What about the UI review ? Are we finally going with this hover effect ?

I don't know.
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?
Combined the styles, :hover:active was not needed so removed it now.
Assignee: jaws → scrapmachines
Attachment #663355 - Attachment is obsolete: true
Attachment #663433 - Flags: review?(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].
Attachment #663433 - Flags: review?(dao) → review-
Attached patch Fianl patch (hoping to be) (obsolete) — Splinter Review
Done.
Thanks for your patience :)
Attachment #663433 - Attachment is obsolete: true
Attachment #663507 - Flags: review?(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.
Attachment #654259 - Attachment is obsolete: true
Attachment #654257 - Attachment is obsolete: true
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
Attachment #663507 - Flags: review?(dao) → review-
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.
I was talking about the background color.
Man I am so bad at this .. :|
Attachment #663507 - Attachment is obsolete: true
Attachment #664106 - Flags: review?(dao)
Attachment #664106 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/97b007a4a587
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Summary: Buttons in the Developer Tools should have a hover state → Buttons in the Developer Toolbar should have a hover state
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.