410719 - Menu buttons, menulists, autocomplete dropdown buttons and toolbarbuttons with menupopups should appear pressed when the menupopup is open

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Victor Bielawski]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010104 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008010104 Minefield/3.0b3pre

(Currently, the button or toolbarbutton's state follows the menupopup's.) This is very similar to bug 408578 , only that this one also applies to buttons and toolbarbuttons.

Reproducible: Always

Steps to Reproduce:
On a Windows machine, click to open any menubutton, menulist, or split toolbarbutton.
Actual Results:  
The button appears in the hover state.

Expected Results:  
The button should appear in the depressed state.

Comment 1

[Victor Bielawski]

Note: This happens on all Windows versions when using a visual style (Classic has its own problems) and used to happen on gtk before bug 408578 landed.

Comment 2

[Victor Bielawski]

Attachment: Screenshot of bug

This shows the error with a split button. The same error occurs with menubutton and menulists.

Comment 3

[Daniel Luz]

I experience it on every single one of the several Windows (XP) machines I've used since the first releases of Firefox. I can also confirm Linux nightlies fixed it, as bielawski1 said, though HTML <select> elements are still misrendered (strangely, their XUL counterparts are good).

Comment 5

[Dão Gottwald [:dao]]

This is quite annoying and seems easy to fix. We shouldn't tie this to bug 288082. Note that bug 437009 already fixed menu buttons.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 364705 [details] [diff] [review]
patch

Looks ok to me, but stick some { } around the hover/open check in nsNativeThemeWin code?  Or move the comment above the if, either one.

Comment 8

[[e-mail]]

Am I, under Windows XP with the Classic theme, suppposed to notice anything?

Comment 9

[Dão Gottwald [:dao]]

Nope, that's expected, as Classic was already fine without this patch.

Comment 10

[[e-mail]]

Comment on attachment 364705 [details] [diff] [review]
patch

>+      if (aState == TS_HOVER && IsOpenButton(aFrame))
>+        // No hover effect while the dropdown menu is open.
>+        aState = TS_NORMAL;
FYI my testing suggests that the true state is more complex still:
PRInt32 eventState = GetContentState(aFrame, aWidgetType);
aState = IsOpenButton(aFrame) ?
    eventState & NS_EVENT_STATE_ACTIVE ? TS_ACTIVE : TS_NORMAL :
    eventState & NS_EVENT_STATE_HOVER ?
      eventState & NS_EVENT_STATE_ACTIVE ? TS_ACTIVE : TS_HOVER : TS_NORMAL;

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> FYI my testing suggests that the true state is more complex still:
> PRInt32 eventState = GetContentState(aFrame, aWidgetType);
> aState = IsOpenButton(aFrame) ?
>     eventState & NS_EVENT_STATE_ACTIVE ? TS_ACTIVE : TS_NORMAL :
>     eventState & NS_EVENT_STATE_HOVER ?
>       eventState & NS_EVENT_STATE_ACTIVE ? TS_ACTIVE : TS_HOVER : TS_NORMAL;

To sum it up, the difference would be that aState can be TS_ACTIVE while the dropmarker is "open":

> IsOpenButton(aFrame)                0 0 0 0 1 1 1 1
> eventState & NS_EVENT_STATE_ACTIVE  0 0 1 1 0 0 1 1
> eventState & NS_EVENT_STATE_HOVER   0 1 0 1 0 1 0 1
> ___________________________________________________
> neil                                N H N A N N A A
> dao                                 N H N A N N N N
> 
>          N = TS_NORMAL, A = TS_ACTIVE, H = TS_HOVER

As far as I remember, I tried doing it your way, but it wasn't quite right, because NS_EVENT_STATE_ACTIVE applies when you press the mouse button within the menupopup, or something like that. So I think ruling out TS_ACTIVE for IsOpenButton(aFrame) is closer to the expected behavior.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

OK, based on the IRC conversation and after more testing, I think I have a slightly better understanding now of what the expected states and their conditions are, and where we're failing. As expected, I couldn't fully implement it (see XXX comments), but I think it's an improvement over the current weirdness.

Comment 13

[[e-mail]]

Comment on attachment 366429 [details] [diff] [review]
patch v2

Sorry for the delay. I tested this on Luna, but forgot that you'd changed the Classic behaviour too, so I had to go back and retest it.

Comment 14

[Dão Gottwald [:dao]]

The summary isn't quite accurate. Anyway...

http://hg.mozilla.org/mozilla-central/rev/b7897c3a39e4

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Looks like this broke the Windows CE build -- DFCS_FLAT doesn't exist on CE (or in the CE SDK), and the patch moved its usage outside of #ifdef WINCE.  Should make the whole block conditional on #ifndef WINCE instead of doing an early return.

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #15)
> Looks like this broke the Windows CE build -- DFCS_FLAT doesn't exist on CE (or
> in the CE SDK), and the patch moved its usage outside of #ifdef WINCE.  Should
> make the whole block conditional on #ifndef WINCE instead of doing an early
> return.

http://hg.mozilla.org/mozilla-central/rev/aa75d2c8a862

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch for 1.9.1 branch

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367382 [details] [diff] [review]
patch for 1.9.1 branch

a191=beltzner

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bda3ef8aae90

Comment 20

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre ID:20090422044118

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

This bug effected a lot of control types, so definitely P1.