Australis: checked icon for items in panel displayed on wrong side with RTL locales

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
11 months ago

People

(Reporter: nohamelin, Assigned: mconley)

Tracking

(Blocks 1 bug, {rtl})

Trunk
Firefox 31
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
For RTL locales (arabic and hebrew, at least), the check icon for items of type "radio" and "checkbox" should be displayed in the right side. When the respective label is sufficiently long, the icon overlaps the text.


It seems similar to bug 297790, but I can't duplicate many of the cases mentioned there; the solutions will be different, in any case.
Reporter

Updated

5 years ago
Blocks: 306980, 960258

Comment 1

5 years ago
Blake, I *think* this shouldn't be too hard a fix, want to have a look? (IIRC these checkmarks are background images, and we'll basically just want to position them with the same right offset on RTL as they have left offset on LTR; you can detect this case with the :-moz-locale-dir(rtl) pseudoselector, and use the "force rtl" add-on ( https://addons.mozilla.org/en-us/firefox/addon/force-rtl/ ) to test whether things work correctly.
Flags: needinfo?(bwinton)
Keywords: rtl
Whiteboard: [Australis:P3]
I'ld like to, but I'm at a conference for most of this week, so I probably won't get to it particularly soon…  But I'll leave the tab open, in case it's still undone when I get some time…
Flags: needinfo?(bwinton)
Lemme see if I can pump out a patch for this one.
Assignee: nobody → mconley
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Posted patch Patch v1 (obsolete) — Splinter Review
Tested on OS X and Windows 7 - looks pretty good. Testing on Ubuntu next.
Assignee

Updated

5 years ago
Attachment #8409814 - Flags: review?(jaws)
Attachment #8409814 - Flags: review?(jaws) → review+
Comment on attachment 8409814 [details] [diff] [review]
Patch v1

Review of attachment 8409814 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/customizableui/panelUIOverlay.css
@@ +87,5 @@
>  .subviewbutton {
>    -moz-padding-start: 18px;
>  }
>  
> +.subviewbutton[checked="true"]:-moz-locale-dir(ltr) {

nit: not really necessary to specify the direction here, you can take LTR as the default case.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +1040,5 @@
> +  background: url("chrome://global/skin/menu/shared-menu-check.png") 11px 11px no-repeat transparent;
> +}
> +
> +.subviewbutton[checked="true"]:-moz-locale-dir(ltr) {
> +  background-position: center left 7px;

nit: same here, you can move this rule up to the block above.
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Comment on attachment 8409814 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8409814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/customizableui/panelUIOverlay.css
> @@ +87,5 @@
> >  .subviewbutton {
> >    -moz-padding-start: 18px;
> >  }
> >  
> > +.subviewbutton[checked="true"]:-moz-locale-dir(ltr) {
> 
> nit: not really necessary to specify the direction here, you can take LTR as
> the default case.
> 
> ::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
> @@ +1040,5 @@
> > +  background: url("chrome://global/skin/menu/shared-menu-check.png") 11px 11px no-repeat transparent;
> > +}
> > +
> > +.subviewbutton[checked="true"]:-moz-locale-dir(ltr) {
> > +  background-position: center left 7px;
> 
> nit: same here, you can move this rule up to the block above.

Right-o - I'll make those corrections and push. Thanks jaws / mikedeboer!
Attachment #8409814 - Attachment is obsolete: true
Attachment #8410295 - Flags: review+
Attachment #8410295 - Flags: feedback+
remote:   https://hg.mozilla.org/integration/fx-team/rev/6bdd0bb387d6
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6bdd0bb387d6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Comment on attachment 8410295 [details] [diff] [review]
Patch v1.1 (r+'d by jaws, feedback from mikedeboer)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

Users in RTL locales will find that Australis-styled popups with checkmark-able items (like the items in the Character Encoding menu) have their checkmarks on the wrong side.


Testing completed (on m-c, etc.): 

Locally tested on Windows, Ubuntu, OS X. A few nights on m-c.


Risk to taking this patch (and alternatives if risky): 

Very low. CSS only.


String or IDL/UUID changes made by this patch:

None.
Attachment #8410295 - Flags: approval-mozilla-aurora?
Attachment #8410295 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
See Also: → 1005855
Flags: in-testsuite?
Keywords: verifyme

Comment 12

5 years ago
(CSS-only, so I wouldn't know how to even test this, so testsuite-)
Flags: in-testsuite? → in-testsuite-
Flags: in-qa-testsuite?
The check mark is now displayed on the right side for the RTL locales.

Tested using the Arabic and Hebrew builds of Firefox 30 (build ID: 20140605174243) and Firefox 31 beta 6 (build ID: 20140630185627) on Windows 7 64bit, Windows 8.1 32bit, Ubuntu 13.04 and Mac OS X 10.8.5.

Also verified with the en-US builds by manually creating a "intl.uidirection.en" string and setting it's value to "rtl".
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

11 months ago
Flags: in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.