833401 - RTL support is broken in the Settings app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Fabien Cazenave [:kaze]]

The Settings app used to be perfectly RTL-compatible, it’s not the case any more — far from it:

 • the Back button is missing (should be on the right side)
 • the transitions don’t work in RTL mode any more
 • some elements are mispositioned (e.g. checkboxes, fake <select> boxes…)

I understand that Arabic is not a short-term concern but I want to ensure Gaia remains RTL-compliant. The sooner, the better.

Comment 1

[Fabien Cazenave [:kaze]]

Notes:
 • this could be done on the UX branch;
 • this could be a good “first bug” for contributors — especially the ones who are comfortable with RTL-languages (hint, hint).

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Pull request 7779

Comment 3

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: PR to Github

Here's it. Took me two days to finish :S

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Nice Ahmed!  I can't really comment on much here, but it would be good to have some tests to prevent RTL from regressing again in the future.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Maybe we should write a second bug to ask the a-team for an RTL Gaia-UI test.  Not sure we can verify this kind of breakage in a unit test.  The Gaia-UI test might be tricky, though, if we don't have an RTL locale in the default eng build.

Comment 7

[Fabien Cazenave [:kaze]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Maybe we should write a second bug to ask the a-team for an RTL Gaia-UI
> test.  Not sure we can verify this kind of breakage in a unit test.

do we plan to have reftests?

> The Gaia-UI test might be tricky, though, if we don't have an RTL locale in
> the default eng build.

I insisted a lot that we should keep Arabic in Eng builds, even if the locale is not 100% up-to-date.
So far so good…

Comment 8

[Fabien Cazenave [:kaze]]

Comment on attachment 8365366 [details]
PR to Github

Looks good, thanks Ahmed! I left a few remarks on your PR — mostly coding style nitpicks and a general comment: please use minimalistic rules. I expect RTL-specific rules to only affect left/right positions and background images.

Cleaning the r? flag, please flag me again when you have addressed my comments. Thanks for the good work!

Comment 9

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8365366 [details]
PR to Github

Thank you :)
All addressed.

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 8365366 [details]
PR to Github

Ahmed, this looks really good! I left a couple code-formatting nitpicks on your PR.

Here are the two points that still worry me a bit:
* missing a few padding-[left|right] rules in lists.css;
* the [small|span]:not(.menu-item-desc) selectors in lists.css don’t have any explicit equivalent in LTR mode.

We’ve discussed this on IRC a bit. I’m tempted to land this patch “as is” because it’s already a very significant improvement (which covers much more than just the Settings app!), but let’s get Pavel’s feedback first.

Comment 11

[Fabien Cazenave [:kaze]]

FTR: “32 changed files with 575 additions and 86 deletions”. Impressive work!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I opened bug 965771 to track setting up tests and CC'd some a-team folks.

Comment 13

[Fabien Cazenave [:kaze]]

Great, thanks Ben!

Comment 14

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8365366 [details]
PR to Github

Hey Ahmed,

great work man :) thanks for this one :)

I have just one comment here. W
hen you override the styles for [dir="rtl"] you can use not so hard selectors
(e.g. you can use just `html[dir="rtl"] .pack-checkbox > span:not(.menu-item)` the specificity here is enough)

f+ from me :)

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 8365366 [details]
PR to Github

R=me. There’s a bit of room for improvement, but this patch does not affect any LTR rule and already fixes most of the broken RTL layout. And Pavel is fine with this patch, too. :-)

Thanks a lot Ahmed!

Comment 16

[Fabien Cazenave [:kaze]]

Attachment: screenshot — after patch

Comment 17

[Fabien Cazenave [:kaze]]

Merged on master: https://github.com/mozilla-b2g/gaia/commit/6ac9b952402fe5f593fbba687d434ca0fb64750b

Comment 18

[Gabriele Svelto [:gsvelto]]

I've noticed that a few new images have been added in this bug. Could you please recompress them with the recompression script available in gaia? This brings small savings but after having landed bug 959659 I'm trying to make sure that all our PNG assets are fully compressed.

The new images can be recompressed with the following command from the gaia root (this requires the optipng and advancecomp tools to be installed):

./tools/png_recompress.sh -v shared/style/buttons/images/icons/dialog_rtl.png \
                             shared/style/buttons/images/icons/[e-mail] \
                             shared/style/buttons/images/icons/[e-mail] \
                             shared/style/buttons/images/icons/view_rtl.png \
                             shared/style/buttons/images/icons/[e-mail] \
                             shared/style/buttons/images/icons/[e-mail] \
                             shared/style/input_areas/images/icons/arrow_rtl.png \
                             shared/style/input_areas/images/icons/[e-mail] \
                             shared/style/input_areas/images/icons/[e-mail]

If you encounter any problems ping me and I'll do it myself in a follow-up.

Comment 19

[Ahmed Nefzaoui [:Nefzaoui]]

Just opened Bug 967662 to accomplish this

Comment 20

[sarsenyev]

Verified the issue is fixed on 2.2 and 2.1 Flame

RTL icons are supported in "Settings"


"Flame 2.2

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141202040207
Gaia: 725685831f5336cf007e36d9a812aad689604695
Gecko: 2c9781c3e9b5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Flame 2.1

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141202001201
Gaia: ccb49abe412c978a4045f0c75abff534372716c4
Gecko: 18fb67530b22
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 21

[sarsenyev]

Verified the issue is fixed on 1.4 Flame, since the fix is landed on 02/04


RTL icons and elements are supported in "Settings"



Device: Flame 1.4 (Flame, 319mb, Full Flash)
BuildID: 20141203000201
Gaia: 22c80a708329321a2fdeed4ece019498c0cec90d
Gecko: 429d90dd383c
Version: 30.0 (1.4)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 22

[Peter Bylenga [:PBylenga]]

Mass Edit: adding the [rtl-meta]