Last Comment Bug 661846 - Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Windows]
: Highlight of on-screen tabs in the List All Tabs menu looks like visual glitc...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All Windows XP
: -- normal (vote)
: mozilla7
Assigned To: Kai Liu
:
Mentors:
Depends on:
Blocks: 626903 666203
  Show dependency treegraph
 
Reported: 2011-06-03 09:01 PDT by Kai Liu
Modified: 2013-11-12 00:57 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch? (5.04 KB, patch)
2011-06-03 12:11 PDT, Kai Liu
jmathies: review+
Details | Diff | Review
Before and after (63.63 KB, image/png)
2011-06-03 12:41 PDT, Kai Liu
no flags Details
move padding calculation to GetWidgetPadding [r=jmathies] (5.22 KB, patch)
2011-06-21 15:25 PDT, Kai Liu
mounir: checkin+
Details | Diff | Review

Description Kai Liu 2011-06-03 09:01:15 PDT
+++ 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 2011-06-03 12:11:36 PDT
Created attachment 537205 [details] [diff] [review]
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 2011-06-03 12:41:28 PDT
Created attachment 537209 [details]
Before and after

...to demonstrate exactly what this patch is trying to fix.
Comment 3 Jim Mathies [:jimm] 2011-06-21 14:12:52 PDT
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 2011-06-21 15:25:29 PDT
Created attachment 540912 [details] [diff] [review]
move padding calculation to GetWidgetPadding [r=jmathies]

Oops; overlooked the PRBool when copying/pasting.
Comment 5 Mounir Lamouri (:mounir) 2011-06-22 03:39:23 PDT
Comment on attachment 540912 [details] [diff] [review]
move padding calculation to GetWidgetPadding [r=jmathies]

Pushed to mozilla-inbound.
Comment 6 Kai Liu 2011-06-22 09:02:51 PDT
Thanks for the check-in!
http://hg.mozilla.org/mozilla-central/rev/c2c6a3a55aaf
Comment 7 Vlad [QA] 2011-08-31 07:21:23 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3

Note You need to log in before you can comment on or make changes to this bug.