Closed Bug 628049 Opened 9 years ago Closed 9 years ago

Improve the hover styling of split menus in the Firefox menu

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: asa, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Whiteboard: [hardblocker])

Attachments

(5 files, 7 obsolete files)

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.
I'm mostly indifferent on the styling here, big part was getting the interaction working.
Whiteboard: [arewepretty-add]
Attached image 3 states (obsolete) —
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.
>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.
Summary: Firefox menu items with sub-menus delayed hovered state styling improvements → Improve the hover styling of split menus in the Firefox menu
Whiteboard: [arewepretty-add]
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.
blocking2.0: --- → ?
Assignee: nobody → gavin.sharp
blocking2.0: ? → final+
Whiteboard: [hardblocker]
>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).
Attached image Separation between the two sections (obsolete) —
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 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
Attachment #515972 - Flags: ui-review-
Attached image Comparison to native windows start menu (obsolete) —
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.
(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.
Attached image Modified start menu (obsolete) —
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.
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.  ;)
Attached patch patch (obsolete) — Splinter Review
This implements the square dividing line, but it breaks with large font sizes, and I'm not sure how to avoid that.
Assignee: gavin.sharp → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #516085 - Flags: feedback?(dao)
Attached image screenshot with patch
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.
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attached image screenshot on linux
Linux/Ubuntu share the same bug, it shouldn't be cut...
Well actually it depends on the OS theme.  It looks much better with Dust Sand than it does with clearlooks.
Whiteboard: [hardblocker][has patch] → [hardblocker]
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 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.
Attachment #515992 - Flags: ui-review?(shorlander)
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?
Attachment #516085 - Flags: feedback?(dao) → feedback-
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
(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 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.
Attachment #515992 - Flags: ui-review?(shorlander) → ui-review-
Attachment #506220 - Attachment is obsolete: true
Attachment #515972 - Attachment is obsolete: true
Attachment #515980 - Attachment is obsolete: true
Attachment #515992 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #516085 - Attachment is obsolete: true
Attached patch alternate patch (obsolete) — Splinter Review
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.
Attachment #516330 - Flags: feedback?(dao)
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.
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attachment #516330 - Flags: feedback?(dao) → review?(dao)
Attachment #516305 - Attachment is obsolete: true
The current patch doesn't seem to address this problem on Linux. Is somebody else going to tackle that, or what exactly?
OS: Windows 7 → All
Hardware: x86 → All
Not in this bug.
OS: All → Windows 7
Hardware: All → x86
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.
Attachment #516330 - Flags: review?(dao) → review-
Whiteboard: [hardblocker][has patch] → [hardblocker][needs new patch]
Component: Menus → Theme
QA Contact: menus → theme
I got the colors for the disabled state from Alex.

I did test in RTL, and it does work.
Attachment #516330 - Attachment is obsolete: true
Attachment #516353 - Flags: review?(dao)
Whiteboard: [hardblocker][needs new patch] → [hardblocker][needs review dao]
Attachment #516353 - Flags: review?(dao) → review+
Firebug adds a splitmenu under the web dev menu. Please check how your patch handles that.
Whiteboard: [hardblocker][needs review dao] → [hardblocker][has patch]
http://hg.mozilla.org/mozilla-central/rev/9da72b391877
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch] → [hardblocker]
Target Milestone: --- → Firefox 4.0
(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.
Blocks: 638211
(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/
Blocks: 639366
Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.