661846 - Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Windows]

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Kai Liu]

+++ This bug was initially created as a clone of Bug #659993 +++

Same as bug 659993, except for Windows instead of Linux.

The reason we have this problem on Windows with classically-rendered menus (i.e., non-Aero; themed XP uses the classic menu rendering code path) is that the padding for the menu item is done in ClassicGetWidgetBorder:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#2494

This use of ClassicGetWidgetBorder for the menuitem padding was introduced over half a decade ago in bug 243078.  It's not clear to me why this approach was taken, and on a cursory glance, it seems wrong.  I want to play around a bit with alternative ways of doing this so that it doesn't cause problems with bug 626903.

Comment 1

[Kai Liu]

Attachment: patch?

This appears to fix the problem without causing any problems of its own.  But I am still somewhat wary because it is not clear to me why the patch in bug 243078 used WidgetBorder instead of the more obvious WidgetPadding.

This patch also moves the padding calculation for top-level menu items (i.e., the menu buttons on the menu bar) from WidgetBorder to WidgetPadding.  While this particular change is not necessary for fixing this bug, I felt that it made sense from a semantic and organizational point of view.

Comment 2

[Kai Liu]

Attachment: Before and after

...to demonstrate exactly what this patch is trying to fix.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 537205 [details] [diff] [review]
patch?

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

Nice. This also repos on Win7/classic, I've confirmed it fixes it there as well.

::: widget/src/windows/nsNativeThemeWin.cpp
@@ +1794,5 @@
>      return PR_TRUE;
>    }
>  
>    if (!theme)
> +    return ClassicGetWidgetPadding(aContext, aFrame, aWidgetType, aResult);

Note, PRBool return result..

@@ +2475,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsNativeThemeWin::ClassicGetWidgetPadding(nsDeviceContext* aContext,

PRBool

@@ +2491,5 @@
> +
> +      rv = ClassicGetThemePartAndState(aFrame, aWidgetType, part, state, focused);
> +      if (NS_FAILED(rv))
> +        return rv;
> +

return PR_FALSE;

::: widget/src/windows/nsNativeThemeWin.h
@@ +113,5 @@
>                               nsIFrame* aFrame,
>                               PRUint8 aWidgetType,
>                               nsIntMargin* aResult);
>  
> +  nsresult ClassicGetWidgetPadding(nsDeviceContext* aContext,

PRBool

Comment 4

[Kai Liu]

Attachment: move padding calculation to GetWidgetPadding [r=jmathies]

Oops; overlooked the PRBool when copying/pasting.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 540912 [details] [diff] [review]
move padding calculation to GetWidgetPadding [r=jmathies]

Pushed to mozilla-inbound.

Comment 6

[Kai Liu]

Thanks for the check-in!
http://hg.mozilla.org/mozilla-central/rev/c2c6a3a55aaf

Comment 7

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3