Closed Bug 1374812 Opened 7 years ago Closed 7 years ago

Sidebar switcher hover state needs updated styling

Categories

(Firefox :: Menus, defect, P1)

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- disabled
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: abenson, Assigned: daleharvey)

References

Details

(Keywords: regression, Whiteboard: [reserve-photon-visual][p4])

Attachments

(3 files)

The hover state for the switcher label and the closing x should match other Photon menu hover styles. Details here: https://mozilla.invisionapp.com/share/PYC5LJJXG#/229940647_Toolbars
Flags: qe-verify?
Whiteboard: [photon-structure] → [photon-structure] [triage]
(In reply to Aaron Benson from comment #0)
> The hover state for the switcher label and the closing x should match other
> Photon menu hover styles. Details here:
> https://mozilla.invisionapp.com/share/PYC5LJJXG#/229940647_Toolbars

A more detailed description (with screenshots) of the problem would be helpful, but on the assumption this is about the rounded corners and the border on the main switcher label, that changed as part of bug 1367242, which I assumed was intentional... It now sounds as if it wasn't? Why was that change made?
Blocks: 1367242
No longer blocks: 1353421
Flags: needinfo?(nhnt11)
Flags: needinfo?(abenson)
Keywords: regression
Whiteboard: [photon-structure] [triage] → [photon-visual] [triage]
Attached image hover_states.png
Flags: needinfo?(abenson)
I think the changes for bug 1367242 were mostly around the font size in the switcher label, though we probably should have caught this too. This is a case of noticing some style inconsistencies in the design now that things are landing in Nightly and I was doing a bit of a UX QA pass. I ran this by Stephen and we agreed on the changes.

Attached an image to better illustrate the point, and the doc linked in the bug description should provide all the detail for the styling of those states. Apologies for not including the screenshot up front.
(In reply to Aaron Benson from comment #3)
> I think the changes for bug 1367242 were mostly around the font size in the
> switcher label, though we probably should have caught this too. This is a
> case of noticing some style inconsistencies in the design now that things
> are landing in Nightly and I was doing a bit of a UX QA pass. I ran this by
> Stephen and we agreed on the changes.
> 
> Attached an image to better illustrate the point, and the doc linked in the
> bug description should provide all the detail for the styling of those
> states. Apologies for not including the screenshot up front.

FWIW, on beta 55 the header looks (at a glance, I can't easily compare with the toolbar because the toolbarbutton styling doesn't look like that in 55) better, so I'm fairly sure 1367242 regressed this.
The hover states were changed intentionally based on <https://people-mozilla.org/~shorlander/projects/photon/Mockups/macOS.html>.
Attached image hover_spec.png
Okay, we want to model the hover style used in the toolbar instead (invision spec linked in the description). I've attached a summary image.
(In reply to Aaron Benson from comment #6)
> Created attachment 8880047 [details]
> hover_spec.png
> 
> Okay, we want to model the hover style used in the toolbar instead

Are all UX people on board with that? It seems like a rather bold statements given that the mockups still don't agree.
I ran it by Stephen Horlander and Bryan Bell - both are on board.
Flags: needinfo?(nhnt11)
Priority: -- → P3
Whiteboard: [photon-visual] [triage] → [reserve-photon-visual][p3]
No longer blocks: 1377003
Whiteboard: [reserve-photon-visual][p3] → [reserve-photon-visual][p4]
Flags: qe-verify? → qe-verify+
QA Contact: brindusa.tot
>The hover state for the switcher label and the closing x should match other Photon menu hover styles

On Windows 10, the toolbar icons have a `border-radius` of 2px, but the sidebar switcher and close buttons have a `border-radius` of 4px. Is this intentional? If so, I think it looks a little out of place on Windows and a tad too curvy and MacOS-y. If not, should I file a separate bug?
Priority: P3 → P4
Assignee: nobody → dharvey
Status: NEW → ASSIGNED
Priority: P4 → P1
Dao said to reduce the scope here to just the switcher
Summary: Sidebar switcher hover state (and closing x) needs updated styling → Sidebar switcher hover state needs updated styling
Attachment #8917317 - Flags: review?(dao+bmo)
Attachment #8917317 - Flags: review?(dao+bmo)
Comment on attachment 8917317 [details]
Bug 1374812 - Update sidebar header button hover states.

https://reviewboard.mozilla.org/r/188334/#review194036

I don't see an updated patch here
Attachment #8917317 - Flags: review?(dao+bmo)
Comment on attachment 8917317 [details]
Bug 1374812 - Update sidebar header button hover states.

https://reviewboard.mozilla.org/r/188334/#review196426

::: browser/themes/shared/sidebar.inc.css:92
(Diff revision 2)
> -#sidebar-switcher-target.active {
> -  border-color: rgba(0, 0, 0, 0.25);
> + * Currently in dark theme the sidebar is still light, for now
> + * override the hover / active states so they are visible, but
> + * remove once sidebar supports the dark theme:
> + *  - https://bugzilla.mozilla.org/show_bug.cgi?id=1385518
> + */
> +#sidebar-switcher-target:hover:-moz-lwtheme-brighttext {

I think we'll need to do this for all lightweight themes (i.e. :-moz-lwtheme), otherwise there might be mismatches depending on the OS theme and the lightweight theme colors.
Attachment #8917317 - Flags: review?(dao+bmo) → review-
Comment on attachment 8917317 [details]
Bug 1374812 - Update sidebar header button hover states.

https://reviewboard.mozilla.org/r/188334/#review197688

::: browser/themes/shared/sidebar.inc.css:91
(Diff revision 4)
> -#sidebar-switcher-target:hover:active,
> -#sidebar-switcher-target.active {
> -  border-color: rgba(0, 0, 0, 0.25);
> +/* Ensure we do not lose contrast between lightweight and OS theme colours */
> +#sidebar-switcher-target:hover:-moz-lwtheme {
> +  background: hsla(240, 5%, 5%, 0.1);
> +}
> +#sidebar-switcher-target:hover:active:-moz-lwtheme,
> +#sidebar-switcher-target:hover:active.active:-moz-lwtheme {

This needs to be #sidebar-switcher-target.active:-moz-lwtheme
Attachment #8917317 - Flags: review?(dao+bmo) → review+
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f0431504aa3
Update sidebar header button hover states. r=dao
https://hg.mozilla.org/mozilla-central/rev/0f0431504aa3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Is this something that we need to uplift to 57 or can it ride the 58 train?
Flags: needinfo?(dharvey)
Version: unspecified → 56 Branch
This doesn't need uplift.
Flags: needinfo?(dharvey)
Ovidiu to verify open the sidebar, the header on hover should not have a border and match hover state of the toolbar buttons
Tested on Mac OS X 10.12 and Windows 10 with the latest Nightly 58.0a1(2017-11-05), I don't see any border but the colors of the header on hover doesn't match the color of the toolbar buttons on hover. 

Here are the code colors:

On Mac Os X 10.12:    The sidebar header on hover has - #DADADA      and the toolbar buttons color is: #E1E1E2

On Windows 10 the difference is minor:  The sidebar header on hover has - #E1E1E1  and the toolbar buttons color is: #E1E1E2
Flags: needinfo?(dharvey)
The --toolbarbutton-hover-background they were set to has transparency so depends on the the colour underneath it, this is an expected part of the change
Flags: needinfo?(dharvey)
Ok, thanks for your help, based on comment 25 and comment 26 I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: