Rework the metric computations for uxthemed menus and eliminate the use of magic numbers

ASSIGNED
Assigned to

Status

()

Core
Widget: Win32
P3
normal
ASSIGNED
10 years ago
9 years ago

People

(Reporter: Kai Liu, Assigned: Kai Liu)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 322597 [details] [diff] [review]
patch v1

The current uxtheme menu code in nsNativeThemeWin.cpp contains a lot of magic numbers in GetWidgetPadding, and the methods used to compute the metrics are at odds with the documentation that Microsoft provides (well, if you count their sample code as documentation; see URL).

This patch refactors the methods used to compute the various menu metrics so that they are consistent with Microsoft's documented approach, and along the way, this allows us to get rid of the hard-coded values used for the Vista menu.  This also fixes three minor problems with the current uxtheme menu appearance:

1) At large font sizes (where the height of the font exceeds the size of the checkbox height), the existing code renders the menu items a couple of px shorter than native.

2) Icons should be horizontally centered with respect to the radio/checkbox graphics.

3) Menu separators are missing a couple of pixels of vertical padding.

I've tested this patch with various font sizes, and everything works as expected.

(I expect bug 435604 to land before this, so this patch was made against that patch instead of trunk.)
Attachment #322597 - Flags: superreview?(vladimir)
Attachment #322597 - Flags: review?(vladimir)
(Assignee)

Updated

10 years ago
Blocks: 427392
(Assignee)

Comment 1

10 years ago
Created attachment 323251 [details] [diff] [review]
patch v2

Notable changes in this patch:

* This patch gets rid of the hard-coded values for NS_THEME_MENUARROW (I didn't notice them when I made patch v1)

* Added comment to menu.css

* This patch was made against the patches in bug 435604 and bug 403458.
Attachment #322597 - Attachment is obsolete: true
Attachment #323251 - Flags: superreview?(vladimir)
Attachment #323251 - Flags: review?(vladimir)
Attachment #322597 - Flags: superreview?(vladimir)
Attachment #322597 - Flags: review?(vladimir)
(Assignee)

Updated

10 years ago
Blocks: 436779
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?
(Assignee)

Updated

10 years ago
No longer depends on: 435604
(Assignee)

Comment 2

10 years ago
Created attachment 326689 [details] [diff] [review]
patch v2 (unbitrot)

Patch applied on top of bug 441452 and bug 403458.
Attachment #323251 - Attachment is obsolete: true
Attachment #326689 - Flags: superreview?(vladimir)
Attachment #326689 - Flags: review?(vladimir)
Attachment #323251 - Flags: superreview?(vladimir)
Attachment #323251 - Flags: review?(vladimir)
(Assignee)

Updated

10 years ago
Attachment #326689 - Attachment is obsolete: true
Attachment #326689 - Flags: superreview?(vladimir)
Attachment #326689 - Flags: review?(vladimir)
Product: Core → SeaMonkey
(Assignee)

Updated

10 years ago
Component: Themes → Widget: Win32
Product: SeaMonkey → Core
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P3
Created attachment 333476 [details] [diff] [review]
patch v2 (nsUXThemeData bitrot removed)

I unbitrotted this patch, but I haven't tested how this affects XP and classic
I also didn't test how this affects Vista. Under the bookmarks menu, there are three major issues that I noticed: the menu arrows are not lined up, the icons in the gutter are no longer centered, and the "Mozilla Firefox" bookmark folder is too tall.
(Assignee)

Comment 5

10 years ago
Yea, there's a bug I introduced when I did the menu arrow metrics; that wasn't present in the previous versions of the patch.  I've been meaning to get around to unbitrotting this and correcting that mistake, but I've been busy (and traveling).  I may have a chance at revisiting this later tonight.

This should have no effect on XP and Classic, as the menus there are rendered with the "classic" code path with the only difference between those two being the appearance of the beveling.  I had also tested this patch with third-party menu styles (via Windows Blinds), and it looks fine there...
QA Contact: themes → win32
You need to log in before you can comment on or make changes to this bug.