Closed Bug 628048 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

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

People

(Reporter: asa, Assigned: dao)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 589146
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
>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).
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).
Adjusting summary to just cover the hover state of the menu item itself.
Summary: hovered highlight on menu item and its sub-menu stick around too long after moving to a different menu item → hovered highlight on menu item sticks around too long after moving to a different menu item
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.
blocking2.0: --- → ?
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.
blocking2.0: ? → -
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #512539 - Flags: review?(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?
Attachment #512539 - Flags: review?(dao) → review-
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.
Attached patch better patch (obsolete) — Splinter Review
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.
Attachment #512539 - Attachment is obsolete: true
Attachment #513255 - Flags: review?(dao)
Duplicate of this bug: 635249
I believe this is Windows All, see:

Bug 635198
Bug 635249
OS: Windows 7 → All
Hardware: x86 → All
Duplicate of this bug: 635198
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.
(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?
Attached patch using DOMMenuItemActive (obsolete) — Splinter Review
Assignee: margaret.leibovic → dao
Attachment #515434 - Flags: review?(dolske)
Attachment #515434 - Flags: feedback?(margaret.leibovic)
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...
Attachment #515434 - Attachment is obsolete: true
Attachment #515437 - Flags: review?(dolske)
Attachment #515437 - Flags: feedback?(margaret.leibovic)
Attachment #515434 - Flags: review?(dolske)
Attachment #515434 - Flags: feedback?(margaret.leibovic)
Attachment #513255 - Attachment is obsolete: true
Attachment #513255 - Flags: review?(dao)
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 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).
Attachment #515437 - Flags: feedback?(margaret.leibovic) → feedback+
Won't this cause extreme perf issues (i.e. is DOMMenuItemActive a mutation event that kills perf)?
It's not a mutation event.
Ah, ok. Thought it was based on the menuactive attribute change. Anyhow, these exist in the tree already...
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 :(
Keywords: uiwanted
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?
(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.
Attachment #515437 - Flags: review?(gavin.sharp)
Attachment #515437 - Flags: review?(gavin.sharp)
Attachment #515437 - Flags: review+
Attachment #515437 - Flags: approval2.0+
Attachment #515437 - Flags: review?(dolske)
http://hg.mozilla.org/mozilla-central/rev/b6fd106a4eb3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0
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?
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
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
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.
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
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
(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.
Verified fixed with Mozilla/5.0 (Windows NT 6.1; rv:2.0.2pre) Gecko/20110426 Firefox/4.0.2pre
Status: RESOLVED → VERIFIED
Keywords: uiwanted
You need to log in before you can comment on or make changes to this bug.