Closed Bug 998481 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 + verified
firefox31 + verified

People

(Reporter: nohamelin, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: rtl, Whiteboard: [Australis:P3])

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 306980, 960258
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
Attached patch Patch v1 (obsolete) — Splinter Review
Tested on OS X and Windows 7 - looks pretty good. Testing on Ubuntu next.
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: 6 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+
See Also: → 1005855
Flags: in-testsuite?
Keywords: verifyme
(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
Flags: in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.