508734 - <menuitem> checkbox ugliness with high DPI

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Sylvain Pasche]

Attachment: menu checkboxes on FF/IE at 100/125% font size.

The checkbox isn't drawn at the correct size, and the nearest neighbors interpolation makes it look dead ugly (try setting font size to 125% for instance).

Comment 1

[Sylvain Pasche]

Attachment: Draw the checkbox using correct size, v1

Current code assumes that the MENU_POPUPCHECKBACKGROUND and the MENU_POPUPCHECK can be drawn using the same rectangle (http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp?mark=1119-1120#1119). That rectangle contains the MENU_POPUPCHECK size with margins. However the MENU_POPUPCHECK should be drawn without these margins, using a smaller rectangle inside the MENU_POPUPCHECKBACKGROUND. (I found this article on MSDN which helped me understand some of that stuff: http://msdn.microsoft.com/en-us/library/bb756947.aspx).

The patch renames the checkbox functions/variables that deals with the MENU_POPUPCHECKBACKGROUND to checkboxBG and adds a new GetCheckboxMargins function.

There's a small behavior change in the patch: the existing versions computes the size of the background checkbox by adding the theme part size of the MENU_POPUPCHECK with the TMT_SIZINGMARGINS margin from the MENU_POPUPCHECKBACKGROUND part.
I didn't quite understand why it was doing so. What I did in the patch is to add the theme part size of the MENU_POPUPCHECK with the TMT_CONTENTMARGINS margins of the MENU_POPUPCHECK. That's what is done in that MSDN article.
(By the way, the TMT_CONTENTMARGINS margins of the MENU_POPUPCHECK are the same as the TMT_SIZINGMARGINS margins of the MENU_POPUPCHECKBACKGROUND part on my system (3px for each border at 96dpi)).

Comment 2

[Sylvain Pasche]

Comment on attachment 394742 [details] [diff] [review]
Draw the checkbox using correct size, v1

Jim, do you want to review?

Comment 3

[Jim Mathies [:jimm]]

+    MARGINS checkboxContent;

&

+    MARGINS checkboxBGSizing;
+    MARGINS checkboxBGContent;

lets be safe -

MARGINS checkboxContent = {0};

r+ with that.

Comment 4

[Sylvain Pasche]

Attachment: Draw the checkbox using correct size, v2

With comments addressed. Thanks for the review.

I guess it doesn't require sr?

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Created an attachment (id=395344) [details]
> Draw the checkbox using correct size, v2
> 
> With comments addressed. Thanks for the review.
> 
> I guess it doesn't require sr?

You'll need an r+ from someone like vlad, I'm not an official peer of widget/windows.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 395344 [details] [diff] [review]
Draw the checkbox using correct size, v2

actually, vlad's not a peer anymore.

Comment 7

[[e-mail]]

"try setting font size to 125% for instance" refers to which font size?

Comment 8

[Sylvain Pasche]

I mean changing the Windows DPI settings (125% would be 120DPI):

http://msdn.microsoft.com/en-us/library/dd464660%28VS.85%29.aspx#appendix_a_setting_high_DPI_in_windows

Comment 9

[[e-mail]]

Comment on attachment 395344 [details] [diff] [review]
Draw the checkbox using correct size, v2

OK, that doesn't count as something I can easily test, so I'm assuming jmathies has done the honours in that department.

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1cb9e3096e8b

Comment 11

[Sylvain Pasche]

Would be cool to have this on 1.9.2, the change is low risk and improves the look of checkbox/radio menu entries for users with a DPI setting greater than 96.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 395344 [details] [diff] [review]
Draw the checkbox using correct size, v2

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.