628048 - hovered highlight on menu item sticks around too long after moving to a different menu item

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Asa Dotzler [:asa]]

Attachment: screenshot showing slow exit from sub-menu

With the new hover triggers submenu in the Firefox menu feature -- bug 589146 -- I'm seeing a delay on the roll-up and un-style of the sub-menu and the hovered item when moving to a different item. 

Steps: 
1. click Firefox menu
2. mouse down to Print and hover for a second to open the sub-menu
3. mouse down to Web Developer

Results:
The Print menu's main item stays in the highlighted state and the sub-menu stays open for too long. 

Expected:
The Print sub-menu should immediately disappear and the highlighted state of the Print menu item should also immediately disappear.

Comment 1

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

Margaret noticed this when testing Dao's patch, but we concluded that we can't really fix it without re-implementing the "keep the menu highlighted/open while quickly mousing down down/sideways to access a sub menu item" logic from nsMenuFrame/nsXULPopupManager [1], which didn't seem like a good idea. Maybe I'm missing something, though - I didn't investigate it thoroughly.

[1] http://hg.mozilla.org/mozilla-central/annotate/812710794ca1/layout/xul/base/src/nsXULPopupManager.cpp#l1659

Comment 2

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

>Expected:
>The Print sub-menu should immediately disappear and the highlighted state of
>the Print menu item should also immediately disappear.

I was about to file a bug on the opposite, that it's too difficult to hit a target in the sub menu with the current timer (the sub menu seems to disappear instantly when you exit at an angle headed towards the bottom item in the sub-menu).

Comment 3

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

I filed bug 628807 on the menus dismissing too fast (sometimes).  Either way, the hover effect should disappear immediately, it's just the menu that should stick around.  And should of course never be two main commands with hover effects shown at the same time (as pictured in the attachment).

Comment 4

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

Adjusting summary to just cover the hover state of the menu item itself.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

I'd like to nominate this for blocking final. 

The reason is that the new menu is one of the first things people encounter in
Firefox 4, and the current hover behavior makes it feel unstable and weird,
since it doesn't match the hover and display behaviors elsewhere in the OS.

The other half of this is bug 633656.

Comment 6

[Dietrich Ayala (:dietrich)]

I think should not block. There's nothing here that blocks any major functionality, nor minor functionality. It's UI polish that we should take if it makes the already unreasonably-late Fx4 train.

Comment 7

[:Margaret Leibovic]

Attachment: patch

Without the patch for bug 633656 applied, sometimes the arrow will stay highlighed but the main menuitem won't. However, with both patches applied the behavior seems to be exactly what we want.

Comment 8

[Dão Gottwald [:dao]]

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

>       <handler event="mouseout"><![CDATA[
>+        this.removeAttribute("active");
>         if (this.menu.open)
>           return;
> 
>         let node = event.relatedTarget;
>         while (node) {
>           if (node == this)
>             return;
>           node = node.parentNode;
>         }
>-        this.removeAttribute("active");
>       ]]></handler>

This leaves a bunch of dead code behind.

Doesn't the mouseout event bubble up from descendant nodes?

Comment 9

[:Margaret Leibovic]

Sorry, that patch was kind of stupid. I think the main issue is the this.menu.open check. Removing that check fixes the issue of the highlight sticking around too long. However, with further testing I found that without that check, we can get into a state where the menupopup is open but the highlight is gone, which is bad.

It seems like what we really want is to immediately remove the active state only if another menuitem become active, but I don't know if we have the ability to do that.

Comment 10

[:Margaret Leibovic]

Attachment: better patch

This patch feels pretty good. It probably isn't a perfect solution, but I think it gets us closer than where we are right now.

Comment 12

[alex_mayorga]

I believe this is Windows All, see:

Bug 635198
Bug 635249

Comment 14

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

Dao: can we get this reviewed soon, we still really want to get this fixed for Firefox 4 since the current double highlight behavior makes the product feel a bit sluggish and unresponsive.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #9)
> It seems like what we really want is to immediately remove the active state
> only if another menuitem become active, but I don't know if we have the ability
> to do that.

Can you use the DOMMenuItemActive event?

Comment 16

[Dão Gottwald [:dao]]

Attachment: using DOMMenuItemActive

Comment 17

[Dão Gottwald [:dao]]

Attachment: using DOMMenuItemActive, v2

Right after submitting my patch, I noticed that when closing the app menu with a splitmenu highlighted, the highlight would stick around when reopening the app menu...

Comment 18

[Johnathan Nightingale [:johnath]]

Dolske - I know you only got tagged for this yesterday, but there are a couple of FF menu UX bugs that beltzner is talking about making into a collective hard-blocker and I suggested that while we debate that, we could just get them done. :)

Comment 19

[:Margaret Leibovic]

Comment on attachment 515437 [details] [diff] [review]
using DOMMenuItemActive, v2

This is the correct version of what my patch was trying to do with heuristics, so it eliminates the edge case-y problems with my patch. When I applied the patch, the menu behaved as expected. I feel like it's kind of messy to be firing out own DOMMenuItemActive event, but it seems like that's the only way to get the behavior we want, and this splitmenu implementation is basically guaranteed to be a little messy, since we don't have splitmenu XUL widgets.

One issue I noticed is that if you move your mouse from an open menupopup to a different splitmenu, only the left side of that splitmenu will be highlighted before its menupopup opens. You can reproduce this by moving your mouse into the "Options" menupopup, then moving it directly onto the "Help" splitmenu from there. However, this issue also exists in the current nightly, so it could probably be filed as a separate bug (it may be bug 628743, but I'm not sure exactly what that bug is describing).

Comment 20

[Nochum Sossonko [:Natch]]

Won't this cause extreme perf issues (i.e. is DOMMenuItemActive a mutation event that kills perf)?

Comment 21

[Dão Gottwald [:dao]]

It's not a mutation event.

Comment 22

[Nochum Sossonko [:Natch]]

Ah, ok. Thought it was based on the menuactive attribute change. Anyhow, these exist in the tree already...

Comment 23

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

UX: based on yesterday's meeting, I think we're waiting on a follow up here that clearly states what the expected behaviour is in terms of mouseIn and mouseOut. Note that we also observed some strangeness depending on whether you hover over the menuItem or the arrow :(

Comment 24

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

Comment on attachment 515437 [details] [diff] [review]
using DOMMenuItemActive, v2

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+            case "popuphidden":
>+              if (event.target == this._parentMenupopup)
>+                this.removeAttribute("active");

This seems to de-activate menu items whose parentMenupopup closed... that's to cover comment 17 I suppose.

>-      <handler event="popuphidden"><![CDATA[
>-        if (event.target == this.firstChild)
>-          this.removeAttribute("active");
>-      ]]></handler>

...while this de-activates menu items whose child menupopups are closed. I guess this is mostly made unnecessary by the fact that menu items now deactivate themselves when other menu items are activated, but it still seems possible to close submenus without de-activating their item (using e.g. "Esc"), so maybe this should stay?

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> This seems to de-activate menu items whose parentMenupopup closed... that's to
> cover comment 17 I suppose.

Yes.

> ...while this de-activates menu items whose child menupopups are closed. I
> guess this is mostly made unnecessary by the fact that menu items now
> deactivate themselves when other menu items are activated, but it still seems
> possible to close submenus without de-activating their item (using e.g. "Esc"),
> so maybe this should stay?

I get the same behavior for a <menu> closed with ESC, so I think this code can and should go.

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b6fd106a4eb3

Comment 27

[Siddhartha Dugar [:sdrocking]]

I am using a build with this fix. With the Web Developer submenu open when I hover to the Print button above, both remain highlighted for some time. Is this intentional?

Comment 28

[nigelgrimshawjones]

Hi, Im still seeing this with Firefox 4 just downloaded tonight and some javascript hovers that were working fine in the previous version.
The site is http://candylane.co.nz and its the timetable section.
Its pretty bad. So um this is NOT fixed.

Nigel

Comment 29

[nigelgrimshawjones]

Hi, Im still seeing this with Firefox 4 just downloaded tonight and some javascript hovers that were working fine in the previous version.
The site is http://candylane.co.nz and its the timetable section.
Its pretty bad. So um this is NOT fixed.

Nigel

Comment 30

[AndreiD[QA]]

Reproducible on:
Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre

I can still see 2 menu items simultaneously highlighted but only for a flash of a second (example: print and web developer). It's unclear for me whether there is more to solve on this issue or not.

Comment 31

[AndreiD[QA]]

Verified also on the following builds:
WFM on:  
Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre

Reproducible on:
Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110403 Firefox/4.2a1pre

Comment 32

[nigelgrimshawjones]

Hi, The issue is still there you need to look at the timetable page

http://candylane.co.nz/studios-home/classes-overview/event/

With this calendar the hovers stick very badly going from left to right. For some reason going down a column there is no issue.

Nige

Comment 33

[Asa Dotzler [:asa]]

(In reply to comment #32)
> Hi, The issue is still there you need to look at the timetable page

Thanks for searching first and trying to avoid a duplicate report, but this one isn't the same as your bug. This one's about the Firefox menu. You should probably file a separate bug on the website problem you're seeing.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Windows NT 6.1; rv:2.0.2pre) Gecko/20110426 Firefox/4.0.2pre