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

(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?

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>.

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.

>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?

Dao said to reduce the scope here to just the switcher

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

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.

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

Pushed by dharvey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f0431504aa3 Update sidebar header button hover states. r=dao

Is this something that we need to uplift to 57 or can it ride the 58 train?

This doesn't need uplift.

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

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

Ok, thanks for your help, based on comment 25 and comment 26 I will mark this as verified fixed.