<select> appearance on RTL

VERIFIED FIXED in mozilla6

Status

()

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

People

(Reporter: ebrahim, Assigned: Ehsan)

Tracking

({rtl})

Trunk
mozilla6
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
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
Version: unspecified → 4.0 Branch

Comment 2

6 years ago
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.
Status: UNCONFIRMED → NEW
Component: Theme → Widget: Win32
Ever confirmed: true
Keywords: rtl
Product: Firefox → Core
QA Contact: theme → win32
Version: 4.0 Branch → Trunk
(Reporter)

Comment 3

6 years ago
Created attachment 530067 [details]
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

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

Comment 5

6 years ago
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

6 years ago
(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".
(Reporter)

Comment 7

6 years ago
Oh, okay :)
Hmm, at first I wanted to blame this on bug 643945, but I backed out that patch locally and this bug is still present...
Created attachment 530461 [details] [diff] [review]
Patch (v1)

I wish I knew of a good way of testing this... :/
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #530461 - Flags: review?(jmathies)
Created attachment 530462 [details] [diff] [review]
Patch (v1)

Ah, I wish some day I remember to set tab expansion settings on every editor on every machine I sit at!
Attachment #530461 - Attachment is obsolete: true
Attachment #530461 - Flags: review?(jmathies)
Attachment #530462 - Flags: review?(jmathies)

Comment 11

6 years ago
(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

6 years ago
(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

6 years ago
(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

6 years ago
(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

6 years ago
(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
(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

6 years ago
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

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

Ehsan assures me everything looks a-ok on classic w/ the patch as is.
Attachment #530462 - Flags: review?(jmathies) → review+

Comment 19

6 years ago
(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

6 years ago
Created attachment 531277 [details] [diff] [review]
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.
Attachment #531277 - Flags: review?(jmathies)
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

6 years ago
Created attachment 531728 [details]
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.

Updated

6 years ago
Attachment #531277 - Flags: review?(jmathies) → review+

Comment 23

6 years ago
I don't have hg commit access...
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a466b57361d0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6

Comment 25

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.