[Windows 10] Menus and toolbarbutton menus (e.g. bookmarks menu) does not hide submenus when opening another submenu using touch

VERIFIED FIXED in Firefox 42

Status

()

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: avaida, Assigned: Gijs)

Tracking

40 Branch
Firefox 44
All
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40 affected, firefox41 affected, firefox42 verified, firefox43 verified, firefox44 verified)

Details

Attachments

(2 attachments)

Reproducible on:
* Nightly 42.0a1 (2015-07-07)
* Aurora 41.0a2 (2015-07-07)
* Beta 40.0b2 (20150706172413)

Affected platforms:
* Microsoft Surface Pro 2 - Windows 10 Pro x64 (Insider Preview Build 10162)

Steps to reproduce:
1. Launch Firefox.
2. Tap the bookmarks menu button.
3. Tap "Bookmarks Toolbar" → tap "Recently Bookmarked" → tap "Recent Tags".

Expected result:
The sub-view of each of those items is displayed one at a time.

Actual result:
The sub-views of each of those items remain opened after tapping other ones.

Additional notes:
* Happens in both Tablet Mode and non-Tablet Mode.
This happens with toplevel menus as well. I expect it's an issue with our menu implementation.

Neil/Neil, do either of you have cycles to look at this and/or can you give hints as to where to look for solutions to this issue?

(dolske, I'm prio-ing as P2 because it's pretty messy, but it's touch-only. Please adjust as you see fit.)
Flags: needinfo?(neil)
Flags: needinfo?(enndeakin)
Priority: -- → P2
Summary: [Windows 10] The bookmarks menu does not display the sub-views of its items properly → [Windows 10] Menus and toolbarbutton menus (e.g. bookmarks menu) does not hide submenus when opening another submenu using touch
I'm guessing that no mousemove/mouseout events are being sent here?

Normally, a mousemove event on a menu will hide the old menu and open the new one. (This happens in nsMenuFrame::HandleEvent and nsMenuPopupFrame::ChangeMenuItem)
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #2)
> I'm guessing that no mousemove/mouseout events are being sent here?
> 
> Normally, a mousemove event on a menu will hide the old menu and open the
> new one. (This happens in nsMenuFrame::HandleEvent and
> nsMenuPopupFrame::ChangeMenuItem)

If I'm using only touch, no, no such events are being sent, I expect (because there's no mouse). It sounds like having another submenu being opened would also have to trigger hiding the old menu, just like mousemove/mouseout? If so, that sounds like something I can probably work on either next week, or (if I don't find time then) after I'm back from PTO (start of August). If you can confirm, I'll add to my needinfo/assigned list... :-)
Flags: needinfo?(neil) → needinfo?(enndeakin)
> If I'm using only touch, no, no such events are being sent, I expect (because there's no mouse).

But a mousedown event does occur despite there being no mouse?

I'd think that there are a number of places that assume that a mousedown would be preceded by a mouseover or mousemove.

But if that is actually expected, then what you could do is if a mousedown occurred and the menu was not the current one, call ChangeMenuItem first before opening it.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #4)
> > If I'm using only touch, no, no such events are being sent, I expect (because there's no mouse).
> 
> But a mousedown event does occur despite there being no mouse?

Yes. http://www.html5rocks.com/en/mobile/touchandmouse/ seems to imply this is de facto expected. Maybe Olli knows more details here?

> I'd think that there are a number of places that assume that a mousedown
> would be preceded by a mouseover or mousemove.

It'd be useful if you have ideas about what places those would be. :-)
Flags: needinfo?(bugs)
I don't have a touch screen either, but given the previous comments, I was readily able to reproduce this with the keyboard and mouse.

1. Open the View menu (I picked it because it has lots of submenus)
2. Hover over one of the submenus with the mouse.
3. Now with the keyboard, press left, up or down, and right.
   This should open a different submenu to the one you're hovering.
4. Click and notice that two submenus are now open.
I don't see that issue at all. Pressing Left at step 3 should close the submenu.
(In reply to Neil Deakin from comment #7)
> I don't see that issue at all. Pressing Left at step 3 should close the
> submenu.

Sure, but clicking the mouse re-opens it, without closing the submenu that pressing up/down then right opened.
OK, I see that now. That would be the same change needed then.
Certain mouse events should fire even when touch events are used
http://www.w3.org/TR/touch-events/#mouse-events
Flags: needinfo?(bugs)
OK, so that and the other link imply that mousemove should be sent before the mousedown event. We should add a fix here for the issue in comment 6 but also investigate to ensure that the right mouse events are being sent here.
We get 1 mousemove event, but because of gEatMouseMove being set to true after the first submenu is opened, we eat the next mousemove event we get.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn
Attachment #8634031 - Flags: review?(enndeakin)
Comment on attachment 8634031 [details]
MozReview Request: Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn

I think you meant to leave the call to OpenMenu in there as well. Otherwise, clicking the mouse doesn't open the menu. For example, open a submenu, press left, then click.
Attachment #8634031 - Flags: review?(enndeakin) → review-
Attachment #8634031 - Flags: review-
Comment on attachment 8634031 [details]
MozReview Request: Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn

Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn
Comment on attachment 8634031 [details]
MozReview Request: Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn

Like this?
Attachment #8634031 - Flags: review?(enndeakin)
(In reply to :Gijs Kruitbosch from comment #17)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2b7f8c89cee

Well, clearly infra doesn't like this test... (it works locally...)
(In reply to :Gijs Kruitbosch (away Jul 18 - Aug 2) from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > remote:  
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2b7f8c89cee
> 
> Well, clearly infra doesn't like this test... (it works locally...)

... and so does running the entire directory locally. :-\

Neil, maybe you have ideas as to why this breaks on infra? I'm a little confused as to what's wrong here.
Blocks: 1158152
No longer blocks: windows-10
Attachment #8634031 - Flags: review?(enndeakin) → review+
Neil, thanks for the r+ - but while the test is orange on infra, I'm not sure how to proceed - any idea what's wrong with it?
Flags: needinfo?(enndeakin)
Did another try push in the hope that the test issue has magically gone away. If not, I'll poke at this again when I'm back home early next week.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #22)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e08dae2f9b

Err, this seems to not actually have the patch. Sigh.

I tried on OSX and when running by directory I could now reproduce a timeout. Pushed to try with something that fixed that for me locally:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=828405c8ce11
remote:   https://hg.mozilla.org/integration/fx-team/rev/58a514b9e26e
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/58a514b9e26e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Reproduced the initial issue on 2015-09-21 Nightly with a MS Pro 2 device.
Confirming the fix on latest Nightly, build ID: 20151005030206
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Comment on attachment 8634031 [details]
MozReview Request: Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn

Approval Request Comment
[Feature/regressing bug #]: touch usability esp. on win8 and win10
[User impact if declined]: confusing menu behaviour (also reproducible without touch, see earlier comments)
[Describe test coverage new/current, TreeHerder]: comes with an automated tests, the XUL popups and menus already have existing test coverage as well
[Risks and why]: pretty low - one line code change, comes with tests, in an area that generally already has tests
[String/UUID change made/needed]: no
Attachment #8634031 - Flags: approval-mozilla-beta?
Attachment #8634031 - Flags: approval-mozilla-aurora?
Comment on attachment 8634031 [details]
MozReview Request: Bug 1181560 - ensure previous menus get closed when opening new ones, r?Enn

Visual regression, taking it.
Should be in 42 beta 5.
Attachment #8634031 - Flags: approval-mozilla-beta?
Attachment #8634031 - Flags: approval-mozilla-beta+
Attachment #8634031 - Flags: approval-mozilla-aurora?
Attachment #8634031 - Flags: approval-mozilla-aurora+
I've also verified this fix on the same device using 42.0b5, build ID: 20151008162217 and latest 43.0a2 Aurora, build ID: 20151008004031.
You need to log in before you can comment on or make changes to this bug.