1008723 - menupopup does not stay open, when I clicked the right mouse twice on the menupopup

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Enable Menubar or Bookmarks toolbar(incl. some folders)
2. Click Bookmark folder to expand a menupopup
3. Right click on a bookmark item in the menupopup
4. Right click on a bookmark item again

Actual Results:
The menupopup is closed

Expected Results:
The menupopup should stay open.
IE9 does.


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/10d56cf4ca6d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140113154517
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/79595e16949f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140113161435
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=10d56cf4ca6d&tochange=79595e16949f

regressed by :
79595e16949f	Masayuki Nakano — Bug 957019 Don't check mouse cursor position at handling non-mouse message in nsWindow::DealWithPopups() r=jimm+enndeakin

Comment 1

[Alice0775 White]

Someone reported the problem to Mozilla Input.
https://input.mozilla.org/en-US/dashboard/response/4413504

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This is my mistake. I removed the code checking mouse cursor position at WM_ACTIVE. However, it's necessary when its wParam is WA_CLICKACTIVE.

This just restore old behavior. However, right-clicking another item looks buggy. E.g., new item isn't highlighted. And also right-clicking on ancestor menu of first right-clicked menu causes the ancestor menu becoming top-most. We should file follow up bug later.

Comment 3

[Neil Deakin]

Comment on attachment 8420969 [details] [diff] [review]
Patch

>+      } else if (LOWORD(aWParam) == WA_CLICKACTIVE) {
>+        // If the WM_ACTIVATE message is caused by a click in a popup,
>+        // we should not rollup any popups at now.

Remove 'at now' from the comment.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5892d2b327dd

Thanks!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5892d2b327dd

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Alice-san, do you have idea whether it's worthwhile to uplift them? I don't think this makes a lot of users want to complain about this regression. If so, we shouldn't uplift the patch for safer release.

Comment 7

[Alice0775 White]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> Alice-san, do you have idea whether it's worthwhile to uplift them? I don't
> think this makes a lot of users want to complain about this regression. If
> so, we shouldn't uplift the patch for safer release.


I think it is not necessary to fix Aurora and Beta.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I forgot that 31 will be next ESR. I think that this regression should be fixed on 31.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8420969 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 957019
User impact if declined: Some users might feel inconvenient like this regression. On bookmark item of sub-folders in bookmark toolbar, user may want to reopen context menu with another bookmark item. Then, the bookmark folder is closed by this bug. I believe that this should be fixed in next ESR.
Testing completed (on m-c, etc.): Landed on m-c and tested for half of a month.
Risk to taking this patch (and alternatives if risky): Not so risky. Bug 957019 removed mouse cursor position check at handling some messages which are not related to mouse cursor (the handler is run only while one or more popup is open). At this time, by my mistake, WM_ACTIVATE with WA_CLICKACTIVE becomes not to check the mouse cursor position. Therefore, popups are closed. This patch just restores the check only in this condition.
String or IDL/UUID changes made by this patch: Nothing.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch (r=enndeakin)

Oops, this is the fixed version of what is addressed by the review. See above request for approval request. Sorry for the spam.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7070b91f488

Comment 13

[Ada [:adalucinet]]

Reproduced with Nightly 2014-05-07.
Verified as fixed with latest Nightly 32.0a1 (Build ID: 20140609030202) and latest Aurora 31.0a2 (Build ID: 20140609004001) on Windows 7 x64. 
The only visible issue is the one described in comment 2 - Is there any follow up bug already logged on this matter?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #13)
> Reproduced with Nightly 2014-05-07.
> Verified as fixed with latest Nightly 32.0a1 (Build ID: 20140609030202) and
> latest Aurora 31.0a2 (Build ID: 20140609004001) on Windows 7 x64. 

Thank you for your work!

> The only visible issue is the one described in comment 2 - Is there any
> follow up bug already logged on this matter?

Not yet. I'll file a bug tomorrow, sorry, I completely forgot it.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> However, right-clicking another item looks
> buggy. E.g., new item isn't highlighted. And also right-clicking on ancestor
> menu of first right-clicked menu causes the ancestor menu becoming top-most.
> We should file follow up bug later.

Filed bug 1024841.