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

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Neil Deakin]

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.

Comment 2

[(not reading, please use [e-mail] instead)]

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)

Comment 3

[Mike Connor [:mconnor]]

mconnor and beltzner feel this is an important blocker for 1.9

Comment 7

[Gabriel Chadwick]

Transferring P1 from bug 403209. Is this the good component/product ?

Comment 8

[Neil Deakin]

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

Comment 9

[Neil Deakin]

Attachment: this is a readable version of the patch!

Is there a better reviewer here?

Comment 10

[Neil Deakin]

Maybe dougt might want to check this patch too?

Comment 12

[Doug Turner (:dougt)]

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.

Comment 13

[Ere Maijala (slow)]

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

[Doug Turner (:dougt)]

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

+ on the wince part.

Comment 15

[Neil Deakin]

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

[Doug Turner (:dougt)]

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

Comment 17

[Ere Maijala (slow)]

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

Nevermind about the pref. r=me

Comment 19

[Gabriel Chadwick]

patch as 3 reviews is it possible to land ?

Comment 20

[Neil Deakin]

Yes, but I won't have access to Windows for a bit, so someone else should do it.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 22

[Neil Deakin]

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

[Gabriel Chadwick]

If someone sends me a build I can test it...

Comment 24

[Neil Deakin]

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.

Comment 25

[Reed Loden [:reed]]

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

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 27

[Neil Deakin]

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

You should always file new bugs.