clicking panel should just close menulist inside it (toolbar bookmark menu)

VERIFIED FIXED in mozilla1.9beta3

Status

()

Core
XUL
P1
major
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

({regression})

Trunk
mozilla1.9beta3
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Steps to reproduce:

1. click star icon on address bar. 
2. open the menulist. 
3. click the bookmarks panel. The menulist should close but the panel should
stay open

I think this might be a Windows only regression. The fix here seems to be to remove the extra check for WM_ACTIVATE in nsWindow::EventIsInsideWindow. That supposedly will break bug 45108 but I don't see that happening.
(Assignee)

Updated

10 years ago
Depends on: 395670
(Assignee)

Updated

10 years ago
Duplicate of this bug: 402115
changing os to windows, since we know it happens there.

if it really does happen on mac, we should change os to all.

seeking blocking (as I sought it for bug #402115)
Flags: blocking1.9?
OS: Mac OS X → Windows XP
mconnor and beltzner feel this is an important blocker for 1.9
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Updated

10 years ago
Duplicate of this bug: 403711

Updated

10 years ago
Duplicate of this bug: 406207
(Assignee)

Updated

10 years ago
Duplicate of this bug: 403209

Comment 7

10 years ago
Transferring P1 from bug 403209. Is this the good component/product ?
Severity: normal → major
Keywords: qawanted
OS: Windows XP → All
Priority: P3 → P1
Hardware: PC → All
Summary: clicking panel should just close menulist inside it regressed → clicking panel should just close menulist inside it (toolbar bookmark menu)
(Assignee)

Comment 8

10 years ago
Created attachment 293233 [details] [diff] [review]
use a activateapp here instead

This patch fixes this bug as well as bug 390197. The change is to some code that was added by bug 45108 when Windows+M is pressed to minimize all windows while hovering over the menu. This code skipped checking if the event occured over a popup for the WM_ACTIVATE event. Instead, we'll use the WM_ACTIVATEAPP event.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #293233 - Flags: superreview?(roc)
Attachment #293233 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Blocks: 390197
(Assignee)

Comment 9

10 years ago
Created attachment 293234 [details] [diff] [review]
this is a readable version of the patch!

Is there a better reviewer here?
Attachment #293233 - Attachment is obsolete: true
Attachment #293234 - Flags: superreview?(roc)
Attachment #293234 - Flags: review?(roc)
Attachment #293233 - Flags: superreview?(roc)
Attachment #293233 - Flags: review?(roc)
(Assignee)

Comment 10

10 years ago
Maybe dougt might want to check this patch too?

Updated

10 years ago
Duplicate of this bug: 409351
(Assignee)

Updated

10 years ago
Attachment #293234 - Flags: review?(dougt)

Comment 12

10 years ago
Comment on attachment 293234 [details] [diff] [review]
this is a readable version of the patch!

ere should review this also.

you can comment out / remove the windows ce part of this.  There is no way to test this on the mozilla 1.9 trunk yet (wince doesn't build yet).  When I get windows ce working again, I will investigate.

Updated

10 years ago
Keywords: qawanted
(Assignee)

Updated

10 years ago
Attachment #293234 - Flags: review?(emaijala)

Comment 13

10 years ago
Comment on attachment 293234 [details] [diff] [review]
this is a readable version of the patch!

Neil, did you check that there are no ill effects if config.trim_on_minimize is on?

Comment 14

10 years ago
Comment on attachment 293234 [details] [diff] [review]
this is a readable version of the patch!

+ on the wince part.
Attachment #293234 - Flags: review?(dougt) → review+
(Assignee)

Comment 15

10 years ago
dougt, did you mean the patch is ok as is, or to remove the windows ce part entirely?

Ere, I don't see anything different happening with that pref set, either visibly or to memory use. The code being changed only gets executed when a popup is open though. Is there a specific case you were thinking might be a problem?

Comment 16

10 years ago
you can do either -- when wince is building again on trunk, i can look at this. (hopefully in a few weeks).

Comment 17

10 years ago
Comment on attachment 293234 [details] [diff] [review]
this is a readable version of the patch!

Nevermind about the pref. r=me
Attachment #293234 - Flags: review?(emaijala) → review+
(Assignee)

Updated

10 years ago
Attachment #293234 - Flags: review?(roc)

Updated

10 years ago
Duplicate of this bug: 410051
Attachment #293234 - Flags: superreview?(roc) → superreview+

Comment 19

10 years ago
patch as 3 reviews is it possible to land ?
(Assignee)

Comment 20

10 years ago
Yes, but I won't have access to Windows for a bit, so someone else should do it.
Keywords: checkin-needed
Neil, what should be done? Running a build with the patch and have tests? If I understand it right and no-one can build it on Windows we could use the Tryserver therefor?
(Assignee)

Comment 22

10 years ago
There is no means of creating tests for this currently, it requires native messages to be sent.

Testing involves making sure this and bug 390197 are fixed. I'm not too worried about that though. I just won't be able to fix any problems that occur if the evil test machine fails.

Comment 23

10 years ago
If someone sends me a build I can test it...
(Assignee)

Comment 24

10 years ago
We're not actually looking for someone to test it, but it would still be useful. We're looking for someone to check it in and be able to monitor the tinderbox afterwards.
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.723; previous revision: 3.722
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
OS: All → Windows XP
Hardware: All → PC
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Neil, we have the correct behavior now. But I see another problem which seems to be raised by this patch? When I click on the menulist the complete name field is highlighted. It is reverted when closing the menulist again. Should I file a new bug for that issue? I don't think that is expected.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 27

10 years ago
(In reply to comment #26)
> Should I file a new bug for that issue?

You should always file new bugs.
Duplicate of this bug: 416579

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.