654698 - <select> appearance on RTL

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[ebrahim]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0

Hi, I think <select> tag appearance on RTL mode is a bit wrong, <select> button for expanding options is designed just for LTR.

Reproducible: Always

Steps to Reproduce:
1. Open my link
2. Put your mouse on that.
3. You can see wrong rounding on right side of <select> button.

Comment 1

[Thomas Ahlblom]

Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

I've tried to reproduce this, but I see nothing special. Could you please attach a screenshot of the button so we know what to look for?

Does the issue still occur if you start Firefox in Safe Mode?
https://support.mozilla.com/en-US/kb/Safe+Mode

How about with a new, empty profile?
https://support.mozilla.com/en-US/kb/Basic%20Troubleshooting#w_8-make-a-new-profile

Comment 2

[Kai Liu]

Hmm, I swear I've seen this problem before, but I can't seem to find a bug for it!  Anyway, it should be a pretty trivial fix; we just need to make sure that NS_THEME_DROPDOWN_BUTTON is drawn using DrawThemeBGRTLAware instead of DrawThemeBG.

Comment 3

[ebrahim]

Attachment: Select button in RTL

@Thomas Ahlblom: I mean right rounding of button that you can see in attachment.

I think this button can designed by a way that not needed different face in RTL or LTR

Comment 4

[Kai Liu]

(In reply to comment #3)
> I think this button can designed by a way that not needed different face in RTL
> or LTR

The appearance of this button is not "ours"; this button comes from the Windows theme.  In XP's default theme, the button was direction-agnostic, but in Vista's Aero theme, it became directional.  Gecko's Windows widget code is drawing this button using the default native theme drawing function instead of the direction-aware function.  Should be a trivial fix (just need to add one line of code).

A number of similar issues were addressed a few years ago, so I'm surprised this one hadn't been fixed; I guess it didn't show up on the radar.

Comment 5

[ebrahim]

Sorry, I am not expert on this and maybe my comment be nonsense but I think Firefox don't use OS native elements on this as you can't see this button on IE and Chrome that have more Native UI than Firefox.

Comment 6

[Kai Liu]

(In reply to comment #5)
> Sorry, I am not expert on this and maybe my comment be nonsense but I think
> Firefox don't use OS native elements on this as you can't see this button on IE
> and Chrome that have more Native UI than Firefox.

No browser uses (or can use) truly native controls; it's simply impossible.  Every browser emulates the native appearance of the native controls.  With respect to the <select> dropdown, Firefox does a better job of this than Chrome or IE.  Just because IE is made by Microsoft does not automatically mean that IE does the best job of emulating Windows controls (e.g., look at all the problems with their handling of scroll bars: incomplete hover effects and a gripper that can overflow outside of the scroll button) and it certainly does not mean that whatever IE does is "correct".

Comment 7

[ebrahim]

Oh, okay :)

Comment 8

[(no longer active)]

Hmm, at first I wanted to blame this on bug 643945, but I backed out that patch locally and this bug is still present...

Comment 9

[(no longer active)]

Attachment: Patch (v1)

I wish I knew of a good way of testing this... :/

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Ah, I wish some day I remember to set tab expansion settings on every editor on every machine I sit at!

Comment 11

[Kai Liu]

(In reply to comment #8)
> Hmm, at first I wanted to blame this on bug 643945, but I backed out that patch
> locally and this bug is still present...

Yea, it's not a regression; it's been like this since Fx3.

And thanks for taking care of this, Ehsan.  I haven't gotten around to restoring my build environment.  Patch looks good; it's exactly what I would've done.

Comment 12

[Kai Liu]

(In reply to comment #9)
> I wish I knew of a good way of testing this... :/

As for testing, this wasn't a regression to begin with, and I can't see how this could regress (unless DrawThemeBGRTLAware breaks, in which case a number of other things will break too).

And as you said, there isn't a good (rather, any) way to reliably test this, esp. since this is all dependent on what the OS theme does.  For all we know, Windows 8 might go back to using a direction-neutral drop button.

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Created attachment 530462 [details] [diff] [review] [review]
> Patch (v1)
> 
> Ah, I wish some day I remember to set tab expansion settings on every editor
> on every machine I sit at!

Do we also need to adjust overflow:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1782

Also, is classic working correctly?

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1782

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> (In reply to comment #10)
> > Created attachment 530462 [details] [diff] [review] [review] [review]
> > Patch (v1)
> > 
> > Ah, I wish some day I remember to set tab expansion settings on every editor
> > on every machine I sit at!
> 
> Do we also need to adjust overflow:
> 
(edit)

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1777

> 
> Also, is classic working correctly?
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/
> nsNativeThemeWin.cpp#1782

Comment 15

[Kai Liu]

(In reply to comment #13)
> Do we also need to adjust overflow:

That particular block of code is #if 0'ed, but yes, that's a good catch:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1218

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #10)
> > > Created attachment 530462 [details] [diff] [review] [review] [review]
> > > Patch (v1)
> > > 
> > > Ah, I wish some day I remember to set tab expansion settings on every editor
> > > on every machine I sit at!
> > 
> > Do we also need to adjust overflow:
> > 
> (edit)
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1777

Yeah, I think we should.

> > Also, is classic working correctly?
> > 
> > http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/
> > nsNativeThemeWin.cpp#1782

I need to test it tomorrow.

Comment 17

[Kai Liu]

None of the stuff mentioned here--DrawThemeBG[RTLAware] or the adjustment of the drawing rectangle--fall within the code path used by classic rendering, so there shouldn't be any changes to the classic rendering (where we basically draw things manually, without using uxtheme; the classic drawing and uxtheme drawing code paths are almost completely separate from each other).

While testing GetThemeMargins on the dropmarker part to see if there is a way to ask uxtheme if there is a need for a 1px overlap without resorting to hard-coding it for CBP_DROPMARKER_VISTA (no, there does not appear to be), I found something interesting:

As you know, by default, we use CBP_DROPMARKER for uxtheme'ed drop buttons (which is what IE and Chrome uses at all times: in Vista, that's the uncurved gray opaque button), but if the OS is Vista or newer and if there hasn't been a border or background set in CSS (bug 441034), we use CBP_DROPMARKER_VISTA, which overlaps a bit with the parent and gives us that curved look.

In nsUXThemeConstants.h, we have the following:

// Dropdown constants
#define CBP_DROPMARKER       1
[...]
#define CBP_DROPMARKER_VISTA 6

However, if you look at vsstyle.h in the PSDK, we have:

enum COMBOBOXPARTS {
	CP_DROPDOWNBUTTON = 1,
	[...]
	CP_DROPDOWNBUTTONRIGHT = 6,
	CP_DROPDOWNBUTTONLEFT = 7,
	[...]
};

So our CBP_DROPMARKER_VISTA corresponds with CP_DROPDOWNBUTTONRIGHT.  However, I don't think CP_DROPDOWNBUTTONLEFT was intended for use as the substitute for CP_DROPDOWNBUTTONRIGHT in RTL.  My guess is that it is intended for use for custom controls that place the drop button on the opposite side in LTR (since Microsoft has always relied on SetLayout for the LTR->RTL conversion).

Anyway, this is largely irrelevant; I think we should continue using the CP_DROPDOWNBUTTONRIGHT part and rely on the SetLayout call in DrawThemeBGRTLAware for the RTL rendering, but I thought that I would point this out in case someone else noticed it.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 530462 [details] [diff] [review]
Patch (v1)

Ehsan assures me everything looks a-ok on classic w/ the patch as is.

Comment 19

[Kai Liu]

(In reply to comment #18)
> Comment on attachment 530462 [details] [diff] [review] [review]
> Patch (v1)
> 
> Ehsan assures me everything looks a-ok on classic w/ the patch as is.

Hm, we still need to adjust drawing rectangle so that it overlaps with the correct border (see comment 15).

Comment 20

[Kai Liu]

Attachment: overlap the border correctly

...like so.  I just tested this patch, and it works as desired.

As noted in comment 15, the directional drop button, unlike the direction-neutral drop button, should overlap and override its container's borders, and that adjustment takes place around line 1218, not line 1777.

Comment 21

[(no longer active)]

Comment on attachment 531277 [details] [diff] [review]
overlap the border correctly

Out of curiosity, can you please submit a before/after screenshot with your changes?  I'm curious to know what I missed while testing this...

Comment 22

[Kai Liu]

Attachment: LTR, no-overlap, overlapped

(In reply to comment #21)
> Comment on attachment 531277 [details] [diff] [review] [review]
> overlap the border correctly
> 
> Out of curiosity, can you please submit a before/after screenshot with your
> changes?  I'm curious to know what I missed while testing this...

From top to bottom, LTR, RTL without border overlap, RTL with border overlap.

In the middle image, we get an incorrect double-border on the left.

Comment 23

[Kai Liu]

I don't have hg commit access...

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/a466b57361d0

Comment 25

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue and everything seems in order. Setting status to Verified Fixed.