628049 - Improve the hover styling of split menus in the Firefox menu

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Asa Dotzler [:asa]]

Attachment: Screenshot showing current and desired styles for hovered menu item with sub-menus

With the new Firefox menu items with sub-menus open on hover behavior from bug 589146 the style for the hovered state is a bit off.  Instead of looking like a split button, they look like two buttons butted up against each other. 

Screenshot showing current and desired styles attached.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm mostly indifferent on the styling here, big part was getting the interaction working.

Comment 2

[pino_sb]

Attachment: 3 states

It'd be nice if there were three states, like the Windows 7 startmenu and Microsoft Office do it. That helps both to make clearer the two buttons are related, and to indicate which part you are currently about to click (which is not shown with the current design).

I created an example to illustrate what I mean, using a little bit of CSS. The userchrome code I used was: 

  splitmenu:hover{
    border: 1px solid #B2D3FB !important;
    background-color: rgba(245,249,255,.50) !important;
    border-radius: 2px !important;
  }
  
  splitmenu:hover > * {
    margin: -1px !important;
    }

It works better without bug 589146 fixed than with it.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'm mostly indifferent on the styling here

Ok, had the weekend to mule this over and now have a more articulate position than being mostly indifferent :)  In terms of interaction, we need to make it clear that there are separate controls, and that you can hit the main item to act on it.  Two ways to do this include:

1) Have a small amount of white space in between the hover regions
2) Have a hard line, and use different gradients on the part that is hovered (native to the start menu, jump lists, and office)

Both of these aid the interaction, but the approach proposed in this bug is visually native, so therefore preferable.

>Instead of looking like a
>split button, they look like two buttons butted up against each other. 

+ the two different areas need to have different gradients based on color, otherwise it starts to look and feel like a normal menu, without a main item that you can interact with.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

I think the dividing line in the middle should be square, and the perimiter should be rounded.

I'd really like to see this fixed. I think this is a [hardblocker] because of visibility in Windows 7.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>I think the dividing line in the middle should be square, and the perimiter
>should be rounded.

Agreed, but I think you might have applied the ui-reviews backwards.  It is really important that the interface visually coveys a separation between the main command and the expansion (so that users realize that this isn't just a bunch of sub-menus, and there actually is a main command).

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Separation between the two sections

This shows the user separation between the two sections, to convey that there is a main command.

Color values are:


selected (lighter)

outline color="#aeccf1"
gradient top color="#fafbfd" (100%)
gradient bottom color="#ebf3fd" (100%)

--------------------

selected (darker)

outline: color="#7a9fcb"
gradient top color="#dceafc" (100%)
gradient bottom color="#c1dbfc" (100%)

Since this is a custom widget we will need to take into account classic and high contrast themes.  Alternate XP themes is less of a priority since the Firefox button isn't standard there.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515972 [details]
Separation between the two sections

I disagree - this looks bad as well. We should copy the Windows 7 start menu appearance here, including the different gradients, sure: http://grab.by/9ehW

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Comparison to native windows start menu

Here is how this is basically the same as the windows 7 start menu.  The colors I'm using are still system native, but they are from explorer so that we can make the distinction stronger.

Comment 9

[:Margaret Leibovic]

(In reply to comment #6)
> Since this is a custom widget we will need to take into account classic and
> high contrast themes.  Alternate XP themes is less of a priority since the
> Firefox button isn't standard there.

Classic, XP, and Linux themes use a solid color for the active menuitem highlight, so I don't think we can use this as a solution for those themes. Also, I feel like it would be strange to imitate the Windows 7 start menu for non-Windows 7 platforms (and, as was mentioned, the Firefox button isn't standard there anyway). Therefore, I think these changes should be made only for Windows aero.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Modified start menu

What if we drop the gradient on the expansion arrow entirely, so there is a more significant difference?  The start menu gradient difference is very subtle.

Comment 11

[Damon Sicore (:damons)]

Better hurry here guys.  We're down to the last couple of bugs.  This may be the last bug left to RC if you don't act quick.   No pressure.  ;)

Comment 12

[:Margaret Leibovic]

Attachment: patch

This implements the square dividing line, but it breaks with large font sizes, and I'm not sure how to avoid that.

Comment 13

[:Margaret Leibovic]

Attachment: screenshot with patch

Comment 14

[:Margaret Leibovic]

Also, I was testing this patch with the patch for bug 628048 applied. I noticed
that without that patch, sometimes the width of the secondary pane will change
sizes because the _moz-menuactive will hang around on .splitmenu-menuitem
longer than it should.

Comment 15

[Antoine Turmel [:GeekShadow]]

Attachment: screenshot on linux

Linux/Ubuntu share the same bug, it shouldn't be cut...

Comment 16

[Bill Gianopoulos [:WG9s]]

Well actually it depends on the OS theme.  It looks much better with Dust Sand than it does with clearlooks.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515992 [details]
Modified start menu

While I like "stronger difference" I think it still looks a little strange. I would expect there to be *some* colour in the arrow ... or some more.

We should punt to shorlander for the colour/gradient decision, IMO.

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 515992 [details]
Modified start menu

Stephen, can you suggest a prettier version that still makes it very clear to the user that the main command is an actual click target.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 516085 [details] [diff] [review]
patch

>       <splitmenu id="appmenu_history"
>-                 iconic="true"

gnomestripe sets an icon on this.

>--- a/browser/themes/winstripe/browser/browser-aero.css
>+++ b/browser/themes/winstripe/browser/browser-aero.css

>+.splitmenu-menuitem[_moz-menuactive],
>+.splitmenu-menu[_moz-menuactive] {
>+  -moz-appearance: none;
...

This needs to depend on the windows-default-theme metric.

Can you be more specific about what breaks with a larger font?

Comment 20

[Valerio]

In Margaret's patch the value for -moz-border-radius is 2px in this lines: 

.splitmenu-menuitem[_moz-menuactive]:-moz-locale-dir(ltr),
.splitmenu-menu[_moz-menuactive]:-moz-locale-dir(rtl) {
  -moz-border-radius-topleft: 2px;
  -moz-border-radius-bottomleft: 2px;
}

.splitmenu-menu[_moz-menuactive]:-moz-locale-dir(ltr),
.splitmenu-menuitem[_moz-menuactive]:-moz-locale-dir(rtl) {
  -moz-border-radius-topright: 2px;
  -moz-border-radius-bottomright: 2px;
}

but the correct value should be 3px to be like as the other menuitem

Comment 21

[:Margaret Leibovic]

(In reply to comment #19)
> Can you be more specific about what breaks with a larger font?

When I changed the system setting to a larger text display, the highlighted splitmenus became too tall. I assumed it was because of this padding:

+.splitmenu-menuitem[_moz-menuactive],
+.splitmenu-menu[_moz-menuactive] {
...
+  padding-top: 2px;
+  padding-bottom: 2px;
...
+}

However, with 150% text size, removing that padding makes the problem better, but does not completely fix it. It seems like we would need more styles on the children elements, but this seems really tricky and messy.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515992 [details]
Modified start menu

Let's deal with this in a different bug, as this aspect of the issue isn't a hardblocker and we're trying to close as quickly as possible.

Filed bug 638116 to track.

Comment 23

[:Margaret Leibovic]

Attachment: updated patch

I updated the patch address the comments so far, but the text size issue is still causing problems, and I'm worried we might need a different approach to address it properly, but I don't know what that approach would look like.

Comment 24

[:Margaret Leibovic]

Attachment: alternate patch

This patch does not completely solve the text size problem, but it prevents the splitmenu items from jumping around by applying the custom margin/padding styles to the splitmenus at all times, not just when they are active.

Comment 25

[:Margaret Leibovic]

Attachment: text size issue (100%, 125%, 150%)

Here are some screenshots that illustrate the problem with increasing the system text size. It's 1px too tall at 125% and 6px too tall at 150%.

With my new patch, these menuitems don't change size on hover, so it's not nearly as bad as it was before.

Comment 26

[Reed Loden [:reed]]

The current patch doesn't seem to address this problem on Linux. Is somebody else going to tackle that, or what exactly?

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Not in this bug.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 516330 [details] [diff] [review]
alternate patch

This lacks styling for the disabled state. r- because of this.

This looks rtl-safe, but did you test it, just to be sure?

>+  .splitmenu-menuitem:-moz-locale-dir(ltr),
>+  .splitmenu-menu:-moz-locale-dir(rtl) {
>+    -moz-border-radius-topleft: 3px;
>+    -moz-border-radius-bottomleft: 3px;
>+  }

use non-moz border-radius properties

>+    background-image: -moz-linear-gradient(top, #fafbfd, #ebf3fd);

"top, " is redundant

I don't think this really needs to be a hardblocker. If it causes problems beyond the known font-size issue, we should back it out and ship.

Comment 29

[:Margaret Leibovic]

Attachment: alternate patch v2

I got the colors for the disabled state from Alex.

I did test in RTL, and it does work.

Comment 30

[Dão Gottwald [:dao]]

Firebug adds a splitmenu under the web dev menu. Please check how your patch handles that.

Comment 31

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/9da72b391877

Comment 32

[:Margaret Leibovic]

(In reply to comment #30)
> Firebug adds a splitmenu under the web dev menu. Please check how your patch
> handles that.

It looks decent, but the background is not transparent, so the gutter is not visible though the highlight background. I will file a follow-up for a transparent gradient.

Comment 33

[Bill Gianopoulos [:WG9s]]

(In reply to comment #24)
> Created attachment 516330 [details] [diff] [review]
> alternate patch
> 
> This patch does not completely solve the text size problem, but it prevents the
> splitmenu items from jumping around by applying the custom margin/padding
> styles to the splitmenus at all times, not just when they are active.

I am now including this patch in my builds available at http://www.wg9s.com/mozilla/firefox/

Comment 34

[Vlad [QA]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre