1079098 - PanelUI subviews have no hover feedback on high contrast themes

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Follow up from bug 1008603#c28, there is no hover feedback for panelUI subviews on high contrast themes.

Comment 1

[:Gijs (out for now; he/him)]

Philip, can you mockup what this should look like?

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

There's this screenshot from bug 1008603:
https://bugzilla.mozilla.org/attachment.cgi?id=8420562

Assuming that the highlights elsewhere in Firefox and on the system look similar as in the screen shot, is there a reason to do it differently?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: panelUI-HChover.patch

This patch implements the highlight colors for non-default themes. Unfortunately there is no possibility to split between the Classic theme and the High-Contrast themes.

Philipp, please check also with Classic theme if the colors are okay.

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8554180 [details] [diff] [review]
panelUI-HChover.patch

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

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +205,5 @@
> +
> +  #BMB_bookmarksPopup .menu-text,
> +  #BMB_bookmarksPopup menupopup {
> +    color: inherit;
> +  }

This doesn't work, because you're changing the color but not the background-color - and in this case, it's inheriting the text color of the hovered menu item, so if you hover over a bookmarks folder item, the items in the submenu all inherit the white text, which is wrong (and makes the text unreadable).

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Classic style screenshot

UI-review can probably happen while the patch is being improved. Philipp, this is what it looks like in classic mode

Comment 6

[Richard Marti (:Paenglab)]

Attachment: panelUI-HChover.patch

Fixed the issue with the white text in popups.

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8554677 [details] [diff] [review]
panelUI-HChover.patch

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

r+ assuming explanations for the below.

Can you please file a followup bug for the |> child indicators on the items with submenus in the bookmarks menu? They should be switched to SVG so they have the right color.

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +165,5 @@
> +    background-color: Highlight;
> +    color: highlighttext;
> +  }
> +
> +  panelview .toolbarbutton-1:-moz-any(@buttonStateActive@,[checked=true]),

Why is the checked=true here? Which buttons does this apply to?

@@ +172,5 @@
> +  menuitem.subviewbutton@menuStateActive@,
> +  .widget-overflow-list .toolbarbutton-1@buttonStateActive@,
> +  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateActive@ {
> +    background-color: Highlight;
> +    border-color: ThreeDLightShadow;

Is all this just to keep this the same as the hover case in the active case? Because it doesn't actually change anything, it seems.

FWIW, IE11 still gives feedback on button active states for toolbar buttons. No feedback is given for menuitems, though, so I'm a bit on the fence as to what we should do here. Considering the feedback on toolbar buttons is very subtle, this is probably OK.

Comment 8

[Richard Marti (:Paenglab)]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8554677 [details] [diff] [review]
> panelUI-HChover.patch
> 
> Review of attachment 8554677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming explanations for the below.
> 
> Can you please file a followup bug for the |> child indicators on the items
> with submenus in the bookmarks menu? They should be switched to SVG so they
> have the right color.
> 
> ::: browser/themes/windows/customizableui/panelUIOverlay.css
> @@ +165,5 @@
> > +    background-color: Highlight;
> > +    color: highlighttext;
> > +  }
> > +
> > +  panelview .toolbarbutton-1:-moz-any(@buttonStateActive@,[checked=true]),
> 
> Why is the checked=true here? Which buttons does this apply to?

This and the following selectors are to overwrite the rules from http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#810.

The checked=true is not used in actual default buttons, but add-ons could use it. If not desired, the same question could be asked for the selector which is already in tree.

> @@ +172,5 @@
> > +  menuitem.subviewbutton@menuStateActive@,
> > +  .widget-overflow-list .toolbarbutton-1@buttonStateActive@,
> > +  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateActive@ {
> > +    background-color: Highlight;
> > +    border-color: ThreeDLightShadow;
> 
> Is all this just to keep this the same as the hover case in the active case?
> Because it doesn't actually change anything, it seems.

Without this rule, when :active, there is no visible feedback of hover/active, especially in High-contrast modes.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Richard Marti (:Paenglab) from comment #8)

Great, let's ship it.

(for sherriffs: I don't think this needs a separate try push, considering it's only CSS changes)

Comment 10

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/6fbc7d37e16b

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fbc7d37e16b

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I just tried this in the product. Unfortunately, it also changes menu items on the top level (think bookmarks panel) on classic themes. I suspect that there's a fair number of Windows 7 users with the classic theme, so that seems too big a trade-off.

Perhaps we could try just drawing borders (no backgrounds) on hover in classic mode and in high-contrast themes?

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #12)
> I just tried this in the product. Unfortunately, it also changes menu items
> on the top level (think bookmarks panel) on classic themes. I suspect that
> there's a fair number of Windows 7 users with the classic theme, so that
> seems too big a trade-off.
> 
> Perhaps we could try just drawing borders (no backgrounds) on hover in
> classic mode and in high-contrast themes?

This can be done like the panel buttons. But:

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #2)
> 
> Assuming that the highlights elsewhere in Firefox and on the system look
> similar as in the screen shot, is there a reason to do it differently?

The highlight on normal menus is white on blue which is now consistent. If you still want only highlighting the border, I can do this.

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: suggested changes

Actually, you're right about the consistency of the blue highlights.
There are two tweaks that should happen in this case though: The items currently still have a grey border which should also be blue (same color as the background color) and the arrow glyph needs to be inverted on hover, just as it is in system menus.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8554490 [details]
Classic style screenshot

Just setting ui-r- for now until the issues in comment #14 get addressed.

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #12)
> I just tried this in the product. Unfortunately, it also changes menu items
> on the top level (think bookmarks panel) on classic themes. I suspect that
> there's a fair number of Windows 7 users with the classic theme, so that
> seems too big a trade-off.

I'm confused by this; do you mean the bookmarks button on the toolbar and the panel attached to it, or the main menu's bookmarks menu and its submenus? ("panel" and "top level" confuses me here; the toplevel menus are not panels, and I wouldn't call the bookmark menu button's panel a "toplevel" item)

Comment 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond
> from comment #12)
> > I just tried this in the product. Unfortunately, it also changes menu items
> > on the top level (think bookmarks panel) on classic themes. I suspect that
> > there's a fair number of Windows 7 users with the classic theme, so that
> > seems too big a trade-off.
> 
> I'm confused by this; do you mean the bookmarks button on the toolbar and
> the panel attached to it, or the main menu's bookmarks menu and its
> submenus? ("panel" and "top level" confuses me here; the toplevel menus are
> not panels, and I wouldn't call the bookmark menu button's panel a
> "toplevel" item)

Sorry for the ambiguity, I was talking about the thing you see in https://bugzilla.mozilla.org/attachment.cgi?id=8560215

Comment 18

[Richard Marti (:Paenglab)]

Attachment: panelUIfix.patch

This patch makes the border on hovered menu/menuitems the same color as the hover body. The arrow changes his color in menus on Classic/HC-themes.

Philipp, for easier testing I made a try build: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4ccf49cff55/try-win32/

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8560874 [details] [diff] [review]
panelUIfix.patch

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

Nice! r=me assuming the arrow shape gets OK'd by Philipp. See minor nits below.

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ -153,5 @@
>    menu.subviewbutton@menuStateHover@,
> -  menuitem.subviewbutton@menuStateHover@,
> -  .widget-overflow-list .toolbarbutton-1@buttonStateHover@,
> -  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateHover@ {
> -    border-color: ThreeDLightShadow !important;

Did this not actually need to be !important ? Can't believe I missed that in the previous review... :-\

@@ +191,5 @@
>  
> +  menu[_moz-menuactive].subviewbutton > .menu-right {
> +    list-style-image: url(chrome://browser/skin/customizableui/menu-arrow.svg#arrow-hover);
> +  }
> +

Nit: Can we add this directly after the normal and disabled style?

Comment 20

[Richard Marti (:Paenglab)]

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8560874 [details] [diff] [review]
> panelUIfix.patch
> 
> Review of attachment 8560874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! r=me assuming the arrow shape gets OK'd by Philipp. See minor nits
> below.
> 
> ::: browser/themes/windows/customizableui/panelUIOverlay.css
> @@ -153,5 @@
> >    menu.subviewbutton@menuStateHover@,
> > -  menuitem.subviewbutton@menuStateHover@,
> > -  .widget-overflow-list .toolbarbutton-1@buttonStateHover@,
> > -  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateHover@ {
> > -    border-color: ThreeDLightShadow !important;
> 
> Did this not actually need to be !important ? Can't believe I missed that in
> the previous review... :-\

I'll check this this evening again, especially the copy/paste buttons, but as I checked it on the panel last time it's really not needed. And I can't say now for which special case I added it.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: panelUIfix.patch

(In reply to Richard Marti (:Paenglab) from comment #20)
> (In reply to :Gijs Kruitbosch from comment #19)
> > Comment on attachment 8560874 [details] [diff] [review]
> > panelUIfix.patch
> > 
> > Review of attachment 8560874 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Nice! r=me assuming the arrow shape gets OK'd by Philipp. See minor nits
> > below.
> > 
> > ::: browser/themes/windows/customizableui/panelUIOverlay.css
> > @@ -153,5 @@
> > >    menu.subviewbutton@menuStateHover@,
> > > -  menuitem.subviewbutton@menuStateHover@,
> > > -  .widget-overflow-list .toolbarbutton-1@buttonStateHover@,
> > > -  .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton@buttonStateHover@ {
> > > -    border-color: ThreeDLightShadow !important;
> > 
> > Did this not actually need to be !important ? Can't believe I missed that in
> > the previous review... :-\
> 
> I'll check this this evening again, especially the copy/paste buttons, but
> as I checked it on the panel last time it's really not needed. And I can't
> say now for which special case I added it.

Yes, it was the middle button of the #edit-controls and #zoom-controls which needed the !important. Best visible in HC-mode.


> > @@ +191,5 @@
> > >  
> > > +  menu[_moz-menuactive].subviewbutton > .menu-right {
> > > +    list-style-image: url(chrome://browser/skin/customizableui/menu-arrow.svg#arrow-hover);
> > > +  }
> > > +
> > 
> > Nit: Can we add this directly after the normal and disabled style?

Moved there with a media query around it to only apply on non-default themes.

Comment 22

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Apologies for the slow turnaround here.
Unfortunately my Windows build environment seems to be completely busted. Could you upload a build somewhere? That would be great!

Comment 23

[Richard Marti (:Paenglab)]

Philipp, you can find a build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-157303359f23/try-win32/

Comment 24

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8561453 [details] [diff] [review]
panelUIfix.patch

Great, that's much better! Thanks for going the extra mile!

Comment 25

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

[Tracking Requested - why for this release]:
Requesting uplift to 38 since that's where the original changes landed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b7e62cd3d58b

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7e62cd3d58b

Comment 28

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 35.0a1 (2014-10-06) using Windows 7 x64.
Verified fixed on Firefox 39.0a1 (2015-03-02) using Windows 7 x64 and Windows 8.1 x64 with High Contrast #1, High Contrast #2, High Contrast Black and High Contrast White.

I have noticed some issues:
	- the shortcut commands are not visible on Windows 8.1 using High Contrast White
	- the arrows are not visible on Windows 7 using Windows Classic Theme

Should I file separate bugs for the issues mentioned?

Comment 29

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #28)
> I was able to reproduce this issue on Firefox 35.0a1 (2014-10-06) using
> Windows 7 x64.
> Verified fixed on Firefox 39.0a1 (2015-03-02) using Windows 7 x64 and
> Windows 8.1 x64 with High Contrast #1, High Contrast #2, High Contrast Black
> and High Contrast White.
> 
> I have noticed some issues:
> 	- the shortcut commands are not visible on Windows 8.1 using High Contrast
> White
> 	- the arrows are not visible on Windows 7 using Windows Classic Theme
> 
> Should I file separate bugs for the issues mentioned?

Are you sure you have a version with this patch applied? The arrows on Win7 classic worked for me when I applied the patch. It's not in the current Nightly yet though...

Comment 30

[Vasilica Tamas, QA [:vtamas]]

This is what I get on Nightly with buildID 20150302030204: http://i.imgur.com/ZRtBehO.jpg

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #30)
> This is what I get on Nightly with buildID 20150302030204:
> http://i.imgur.com/ZRtBehO.jpg

As Philipp said, that Nightly doesn't have this fix yet. You should try today's Nightly instead.

Comment 32

[Vasilica Tamas, QA [:vtamas]]

 - I confirm that the arrows are visible on Nightly (2015-03-03) using Windows 7 x64 with Windows Classic Theme.

 - But the shortcut commands are still not visible on hover on Nightly (2015-03-03) using Windows 8.1 x64 with High Contrast Black ( http://i.imgur.com/9tdvr7g.jpg ) and High Contrast White ( http://i.imgur.com/nGsgQLX.jpg ).

   Should I log a new bug on this?

Comment 33

[Richard Marti (:Paenglab)]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #32)
> 
>    Should I log a new bug on this?

Yes, and please cc me to this bug.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38.  
Philipp, can you nominate your patch(es) for uplift to 38? Thanks!

Comment 35

[:Gijs (out for now; he/him)]

Comment on attachment 8561453 [details] [diff] [review]
panelUIfix.patch

Approval Request Comment
[Feature/regressing bug #]: Australis
[User impact if declined]: can't see hover styles in panel menu
[Describe test coverage new/current, TreeHerder]: no, styling only
[Risks and why]: pretty low, only styling changes, and should only affect high contrast themes
[String/UUID change made/needed]: nope


NB: please include the followup from bug 1139091 in the uplift.

Comment 36

[:Gijs (out for now; he/him)]

(In reply to Gijs Kruitbosch (unavailable until Friday March 13th) from comment #35)
> NB: please include the followup from bug 1139091 in the uplift.

... and bug 1139337. Vasilica, please always add followup bugs to blocker lists so others can find them back later.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Gijs, if the other two bugs need uplift to 38, can you request uplift in the patches on those bugs?

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8561453 [details] [diff] [review]
panelUIfix.patch

Approving for uplift to 38 since this looks like a low-risk fix for a UI issue. 

Gijs, do both patches need to be uplifted or just this one?

Comment 39

[:Gijs (out for now; he/him)]

(In reply to Liz Henry :lizzard from comment #37)
> Gijs, if the other two bugs need uplift to 38, can you request uplift in the
> patches on those bugs?

I can do, but it'll just be more flags, and it only really makes sense to take all three csets for transplanting. Without the cset for this bug, we don't need the ones from the other two..

From this bug, only the second patch needs uplift because the first one landed on 38. So the csets for transplanting would be:

https://hg.mozilla.org/mozilla-central/rev/b7e62cd3d58b
https://hg.mozilla.org/mozilla-central/rev/d6580fa1125f
https://hg.mozilla.org/mozilla-central/rev/f078882f6e3a

Is it OK if I (or RyanVM or whoever does uplifts these days) just land these three on aurora and then update the three bugs appropriately?

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Gijs: OK, I understand now and that makes sense. Thanks! 

My rationale for always going through doing the approval requests and setting the flag, is so that, in the future, if there were some problem on one of the other bugs, it would be easier for people to unravel the story of what happened, when it happened, and why.  600 bugs later, it may not be so easy to reconstruct those stories from multiple bugs and sets of patches.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03cef824009e

Comment 42

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0

Verified fixed on Firefox 38 beta 5, build ID: 20150416143048.