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

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Windows 10 x64 (10162)

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.

Comment 1

[:Gijs (he/him)]

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.)

Comment 2

[Neil Deakin]

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)

Comment 3

[:Gijs (he/him)]

(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... :-)

Comment 4

[Neil Deakin]

> 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.

Comment 5

[:Gijs (he/him)]

(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. :-)

Comment 6

[[e-mail]]

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.

Comment 7

[Neil Deakin]

I don't see that issue at all. Pressing Left at step 3 should close the submenu.

Comment 8

[[e-mail]]

(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.

Comment 9

[Neil Deakin]

OK, I see that now. That would be the same change needed then.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Certain mouse events should fire even when touch events are used
http://www.w3.org/TR/touch-events/#mouse-events

Comment 11

[Neil Deakin]

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.

Comment 12

[:Gijs (he/him)]

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.

Comment 13

[:Gijs (he/him)]

Attachment: 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 14

[Neil Deakin]

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.

Comment 15

[:Gijs (he/him)]

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 16

[:Gijs (he/him)]

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

Like this?

Comment 17

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2b7f8c89cee

Comment 18

[:Gijs (he/him)]

(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...)

Comment 19

[:Gijs (he/him)]

(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.

Comment 20

[:Gijs (he/him)]

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?

Comment 21

[:Gijs (he/him)]

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.

Comment 22

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e08dae2f9b

Comment 23

[:Gijs (he/him)]

(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

Comment 24

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/58a514b9e26e

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/58a514b9e26e

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue on 2015-09-21 Nightly with a MS Pro 2 device.
Confirming the fix on latest Nightly, build ID: 20151005030206

Comment 27

[:Gijs (he/him)]

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

Comment 28

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1467488dd1cb

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/40d4dbc06eaf

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

I've also verified this fix on the same device using 42.0b5, build ID: 20151008162217 and latest 43.0a2 Aurora, build ID: 20151008004031.