594158 - Exiting the new Firefox menu by clicking outside the Firefox window results in an active Firefox button on an inactive window

Status: RESOLVED WORKSFORME

(Core :: Widget: Win32, defect)

Description

[hjalte]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100906 Firefox/4.0b6pre Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100906 Firefox/4.0b6pre Firefox/4.0b6pre

D2D and D3D9 enabled on the latest hourly.

Picture of active Firefox menu on inactive window: http://i51.tinypic.com/mw6q0g.jpg

Reproducible: Always

Steps to Reproduce:
1. Click on the new Firefox menu
2. Exit the menu by clicking anywhere outside the Firefox window (Taskbar, desktop, other windows)

Actual Results:  
Firefox menu remains active although the Firefox window is inactive.

Expected Results:  
Firefox menu should revert to its inactive state.

Comment 1

[Jim Mathies [:jimm]]

confirmed

Comment 2

[Dão Gottwald [:dao]]

Sounds like a Win32 widget bug? :-moz-window-inactive is used extensively on OS X and I haven't heard of problems there.

Comment 4

[Jim Mathies [:jimm]]

Hmm, took a shot and applied the patch in bug 593959, but it didn't seem to solve this.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #2)
> Sounds like a Win32 widget bug? :-moz-window-inactive is used extensively on OS
> X and I haven't heard of problems there.

Doesn't seem to happen on any other button with a menu, so assuming we use the same methodology, it doesn't look like a widget bug. I'll have to dig into what takes place here to see.

Comment 6

[Timothy Nikkel (:tnikkel)]

Note that I think the patch in bug 593959 needs to be updated to include hover cases as well as active cases, and the Firefox button does change colour when you hover over it when it is on an inactive window. So it could still be bug 593959.

Comment 7

[Joe Drew (not getting mail)]

Olli, can you take a look at what Timothy said in comment 6, to see if it fixes this bug?

Also, I'm not convinced it's a blocker, but I don't want to unilaterally override Benjamin.

Comment 8

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

bug 593959 is about different documents. This one sounds like about
deactivating a window.

I don't have a system for windows development, but I'll try to look at this a bit.

Comment 9

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

So how do we style the Firefox button? Based on :active or 
:-moz-window-active or what?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Both :active and :-moz-window-inactive are used.
This is the relevant styling: http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#112

Comment 11

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

So as a quickhack, would
changing #appmenu-button:hover:active
to
#appmenu-button:hover:active:not(:-moz-window-inactive)
help here?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

It doesn't seem to help.

I'm wondering if this is related to how the focus+activate messages are handled. The widget code expects the messages to be ordered as activate then focus. maybe in this case the messages are reversed.

Comment 13

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

So I'm now on Win7, and I can see the problem.
But for some reason the button doesn't change state immediately
when the window becomes (de)activated. 

What causes -moz-window-inactive updates?
Is it possible that some inactive message goes to the (native) widget which
controls the popup menu and not to the main window?

It would be great if someone with a system for Win7 development could look at this.

Comment 14

[Joe Drew (not getting mail)]

Olli, that's bug 594367.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

WM_ACTIVATE is consumed on DealWithPopups. It looks like there's no problem to not consume that message. (The handling of the message remains the same, it's only the flag returned to consume the event that changes)

Jim, Masayuki, any input?

Comment 16

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

I have no idea about any new regressions by the patch, Kimura-san, how about you?

# I guess that WM_ACTIVATEAPP might have similar hidden bug.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

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

I sent this to tryserver and have also been using the patch for some days and didn't notice any regression. I think this change is innocuous, but it's probably better to get this in early, so setting the review flag.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Comment on attachment 487007 [details] [diff] [review]
> Patch
> 
> I sent this to tryserver and have also been using the patch for some days and
> didn't notice any regression. I think this change is innocuous, but it's
> probably better to get this in early, so setting the review flag.

So I guess we've removed the glass non-active state, which makes this hard to test. With this patch though I still occasionally see the hover state remain on the button. Is that supposed to be fixed by this patch as well?

Comment 19

[Jim Mathies [:jimm]]

One thing I did notice, this addresses a problem with active window state. (If you test on aero basic you can see the problem.) If the hover state issue isn't supposed to be part of this, I'll r+ the patch.

Comment 20

[Joe Drew (not getting mail)]

Is this even still an issue now that the button stays orange always?

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> Is this even still an issue now that the button stays orange always?

Yeah but it's subtle, the button sometimes keeps the orange hover glow.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

not happening anymore, probably fixed by bug 511010