Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in mozilla7

Status

()

Core
Widget: Win32
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Kai Liu, Assigned: Kai Liu)

Tracking

Trunk
mozilla7
All
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
+++ 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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → kliu
Status: NEW → ASSIGNED
Attachment #537205 - Flags: review?(jmathies)
(Assignee)

Comment 2

6 years ago
Created attachment 537209 [details]
Before and after

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

Comment 3

6 years ago
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
Attachment #537205 - Flags: review?(jmathies) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 540912 [details] [diff] [review]
move padding calculation to GetWidgetPadding [r=jmathies]

Oops; overlooked the PRBool when copying/pasting.
Attachment #537205 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Summary: Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch → Highlight of on-screen tabs in the List All Tabs menu looks like visual glitch [Windows]
Blocks: 666203
Comment on attachment 540912 [details] [diff] [review]
move padding calculation to GetWidgetPadding [r=jmathies]

Pushed to mozilla-inbound.
Attachment #540912 - Flags: checkin+
Keywords: checkin-needed
Whiteboard: [inbound]
(Assignee)

Comment 6

6 years ago
Thanks for the check-in!
http://hg.mozilla.org/mozilla-central/rev/c2c6a3a55aaf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7

Comment 7

6 years ago
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.