Closed Bug 1008723 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget: Win32, defect)

29 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 + verified
firefox32 --- verified

People

(Reporter: alice0775, Assigned: masayuki)

References

Details

(Keywords: regression, Whiteboard: [parity IE] [qa!])

Attachments

(1 file, 1 obsolete file)

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
Someone reported the problem to Mozilla Input.
https://input.mozilla.org/en-US/dashboard/response/4413504
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8420969 - Flags: review?(enndeakin)
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.
Attachment #8420969 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/5892d2b327dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
Oops, I forgot that 31 will be next ESR. I think that this regression should be fixed on 31.
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.
Attachment #8420969 - Flags: approval-mozilla-aurora?
Oops, this is the fixed version of what is addressed by the review. See above request for approval request. Sorry for the spam.
Attachment #8420969 - Attachment is obsolete: true
Attachment #8420969 - Flags: approval-mozilla-aurora?
Attachment #8431275 - Flags: approval-mozilla-aurora?
Attachment #8431275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [parity IE] → [parity IE] [qa+]
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?
Status: RESOLVED → VERIFIED
Flags: needinfo?(masayuki)
Whiteboard: [parity IE] [qa+] → [parity IE] [qa!]
(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.
Flags: needinfo?(masayuki)
(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.
You need to log in before you can comment on or make changes to this bug.