418027 - Win32 buttons do not enter hover state when both focused and hovered (or: hover state should take precedence over focused)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Victor Bielawski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021504 Minefield/3.0b4pre

There is nothing to add to the summary except a screenshot: http://img528.imageshack.us/img528/5906/win32buttonfocushoverbuph5.png

Reproducible: Always

Steps to Reproduce:
1. Focus a button
2. Hover the mouse over it.

Actual Results:  
The button is in the focused state.

Expected Results:  
The button should be in the hover state.

This bug requires visual styles to be turned on.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: nsNativeThemeWin::StandardGetState was checking for focused and returning early before checking for hover

When a button is focused, and mouse hovers over, it will now show the hovered state and focus rect.  

Please let me know if there should be a test case defined for this, and if so any hints of similar bugs with similar tests would be appreciated.

I ran this patch through the following automated and there were no fails, but it ran very quickly, please let me know if there are other tests I should be running as well.  
I ran:
TEST_PATH=widget/tests/ make -C $(OBJDIR) mochitest-plain

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 535808 [details] [diff] [review]
nsNativeThemeWin::StandardGetState was checking for focused and returning early before checking for hover

Review of attachment 535808 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/nsNativeThemeWin.cpp
@@ +510,5 @@
>  
>  PRInt32
>  nsNativeThemeWin::StandardGetState(nsIFrame* aFrame, PRUint8 aWidgetType,
>                                     PRBool wantFocused)
>  {

This will apply to a number of ui elements, not just buttons. Are you sure this is the appropriate default behavior for everything this applies to?

http://mxr.mozilla.org/mozilla-central/search?string=StandardGetState

Comment 4

[Brian R. Bondy [:bbondy]]

Upon further inspection, fix looks good and also fixes another problem (see NS_THEME_TAB).
I mention below the before and after effect for all of the widget types that use the modified function:

When focused and the pointer is on top:

NS_THEME_BUTTON
- Before shows focus outline but no highlight color for mouse pointer (original reported problem)
- After Shows both focus outline and highlight color

NS_THEME_CHECKBOX
NS_THEME_RADIO
- Before shows focus outline and highlight color when pointer goes over
- After works same

NS_THEME_TEXTFIELD
NS_THEME_TEXTFIELD_MULTILINE
- Before shows slight border change with pointer on top, slightly darker change with focus
- After works same 

NS_THEME_SPINNER_UP_BUTTON
NS_THEME_SPINNER_DOWN_BUTTON
- Cannot focus, gets highlight color (no focus even with -moz-user-focus: normal;)
- After works same

NS_THEME_TAB
- Before shows focus only on the selected tab, other tabs show highlight color as mouse pointer goes over. 
  If I force focus on non active tabs with tab { -moz-user-focus: normal } though it has the same bug as the original reported bug for button
- After shows same if no explicit focus is specified.  If -moz-user-focus:normal on non active tabs, fixes problem as with NS_THEME_BUTTON

NS_THEME_TREEVIEW_HEADER_CELL
- Before does not get focus, does get highlight color
  -moz-user-focus:normal has no effect
- After works same

On a side note, should the spinner not being able to be focused with "-moz-user-focus: normal;" be posted?  Please advise and I can attempt to post if it is not already posted.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 535808 [details] [diff] [review]
nsNativeThemeWin::StandardGetState was checking for focused and returning early before checking for hover

Thanks for the detailed write up! I tested each of these and compared them to IE, everything looks ok.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 535808 [details] [diff] [review]
nsNativeThemeWin::StandardGetState was checking for focused and returning early before checking for hover

http://hg.mozilla.org/integration/mozilla-inbound/rev/ffb4d47e4ada

I think the rule on this new flag is to just set checkin? w/o an email. People who land stuff will come around and find it. Note this bug should remain open until the patch merges to mc.

Comment 7

[Marco Bonardo [:mak]]

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