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

Philip, can you mockup what this should look like?

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?

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

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

Fixed the issue with the white text in popups.

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.

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

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

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?

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

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 on attachment 8554490 [details] Classic style screenshot Just setting ui-r- for now until the issues in comment #14 get addressed.

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

(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

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/richard.marti@gmail.com-c4ccf49cff55/try-win32/

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?

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

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

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!

Philipp, you can find a build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/richard.marti@gmail.com-157303359f23/try-win32/

Comment on attachment 8561453 [details] [diff] [review] panelUIfix.patch Great, that's much better! Thanks for going the extra mile!

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

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?

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

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

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

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

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

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

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.

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

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

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?

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

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.

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.