Closed Bug 998917 Opened 11 years ago Closed 10 years ago

Keyboard keys are reversed in RTL locales

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g-v2.1 verified)

Status:
VERIFIED FIXED
Milestone:
2.0 S6 (18july)
Tracking Flags:
Tracking Status
b2g-v2.1 --- verified

People

(Reporter: arky, Assigned: rudyl)

References

Details

Attachments

(5 files, 1 obsolete file)

English-Keyboard-in-Arabic-Locale.jpg
516.53 KB, image/jpeg
Details
2014-07-02-09-31-04.png
55.96 KB, image/png
Details
WIP
46 bytes, text/x-github-pull-request
nefzaoui
: feedback+
stas
: feedback-
Details | Review
Patch V1
46 bytes, text/x-github-pull-request
stas
: review+
Details | Review
verify_video
1.73 MB, video/mp4
Details
The standard QWERTY English keyboard characters are reversed on phones with RTL (Arabic) locales. See attached screenshot for more information.
Actually all layouts are reversed which shouldn't happen at all. I will be looking into it. :)
Assignee: nobody → nefzaoui.ahmed
Summary: QWERTY keyboard keys are reversed in RTL locales → Keyboard keys are reversed in RTL locales
Thanks for looking into this :)
Attached file PR to Github (obsolete) —
Rudy, may I get this reviewed? I'm curious to see what review comes out of this. Also asking Kaze for feedback about it since it involves l10n.js Guys? Thanks
Attachment #8411014 - Flags: review?(rlu)
Attachment #8411014 - Flags: feedback?(fabien)
Comment on attachment 8411014 [details] [review] PR to Github Hi, Thanks for looking into this. If simply setting document.dir as "ltr" could fix this issue, I would suggest we do this directly in keyboard code, no need to include l10n.js for this purpose. Let me know if I take anything wrong. Thanks.
Attachment #8411014 - Flags: review?(rlu)
(In reply to Rudy Lu [:rudyl] from comment #4) > Comment on attachment 8411014 [details] [review] > PR to Github > > Hi, > > Thanks for looking into this. > If simply setting document.dir as "ltr" could fix this issue, I would > suggest we do this directly in keyboard code, no need to include l10n.js for > this purpose. > > Let me know if I take anything wrong. > Thanks. Concerning just adding document.documentElement.dir = "ltr" in Keyboard code: At the very moment you click on "Arabic" in languages list in Settings here's what (as far as I understand) happens 1) dir is default to LTR 2) Choosing Arabic, so l10n sets dir = "RTL" 3) Then comes our document.documentElement.dir = "ltr" so this sets it back to LTR I am absolutely no expert in anything related to performance, but I think putting a condition within l10n code will reduce the number of times our dir value changes from 2 to 1 by 1) dir is default to LTR 2) l10n checks for a variable's value in App X and decide to either keep it LTR or set it to RTL I may be very wrong anyway but I like to know what you think about this?
Flags: needinfo?(rlu)
Comment on attachment 8411014 [details] [review] PR to Github Sorry that I really took something wrong when writing my previous comment, that we already had l10n.js included in our keyboard code. (But in the end, we might be able to remove that, that is another story). However, I don't think it is good to make l10n code depends on a global variable like this. What I wanted to say is, What could be wrong, if we always set document.direction = "ltr" in keyboard? That will make modifying l10n.js not necessary, right?
Attachment #8411014 - Flags: feedback-
Flags: needinfo?(rlu)
Attached image 2014-07-02-09-31-04.png —
I can repro this on 2.0, 2014-07-02 build.
Comment on attachment 8411014 [details] [review] PR to Github Flagging Omega on UX review to ensure "QWERTY flipping" issue is fixed.
Attachment #8411014 - Flags: ui-review?(ofeng)
Comment on attachment 8411014 [details] [review] PR to Github Looks good from UX POV.
Attachment #8411014 - Flags: ui-review?(ofeng) → ui-review+
Still looking for a smoother way to approach this based on comment 6.
On Firefox desktop, we have a hardcoded list of RTL locales <http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#1529> and when we want to decide which direction to show the UI in, we check the active locale against that list. Can we perhaps do something very similar here?
Attached file WIP —
This patch is to counter propose that we add a flag to <html>, "data-l10n-ignore-dir", and don't assign dir by l10n.js, if that flag is "true". I have not added any tests for this patch, because I'd like to get feedback from l10n experts first.
Attachment #8455157 - Flags: feedback?(nefzaoui.ahmed)
Comment on attachment 8455157 [details] [review] WIP Stas, Could you please take a look at this proposal? Thank you.
Attachment #8455157 - Flags: feedback?(stas)
Comment on attachment 8455157 [details] [review] WIP Great! Perfect way to handle this bug! However I wonder if we could make the word RTL fit into the attribute? To make it explicitly clear that this page will *not* be RTL'ed? We'll also use this solution for Dialer app :)
Attachment #8455157 - Flags: feedback?(nefzaoui.ahmed) → feedback+
Attachment #8411014 - Attachment is obsolete: true
Attachment #8411014 - Flags: feedback?(fabien)
Comment on attachment 8455157 [details] [review] WIP Hi Rudy, thanks for the patch. I'm reluctant to extend the data-l10n-* API with data-l10n-ignore-dir="true". f- for now; if my idea from below doesn't make sense, I'll be happy to discuss this addition again. I wonder if instead you could simply defined dir="ltr" somewhere deeper in the DOM tree, on <body> or a <div> which holds the key elements. This way no special-casing would be necessary and we could also get more granularity of control over which elements change direction on language change, and which don't. For instance, in the future we might have text menus which pop up when a key is pressed and held, or we could display the name of the current language at the bottom of the space key etc. etc., and we'd want these elements to change direction of the content inside.
Attachment #8455157 - Flags: feedback?(stas) → feedback-
Attached file Patch V1 —
Stas, Thanks, advice taken. Could you please help review this is good enough?
Attachment #8455961 - Flags: review?(stas)
Comment on attachment 8455961 [details] [review] Patch V1 Thanks, Rudy. I've tested this patch and it appears to work well. r=me.
Attachment #8455961 - Flags: review?(stas) → review+
Landed to Gaia master, https://github.com/mozilla-b2g/gaia/commit/eb04b0228011a34b8c45d1fc675a6b9b7965b79d -- Stas, Ahmed, Thanks for your help on this issue.
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g-v2.1: --- → fixed
Resolution: --- → FIXED
Whiteboard: [p=1]
Assignee: nefzaoui.ahmed → rlu
Target Milestone: --- → 2.0 S6 (18july)
status-b2g-v2.1: fixed → verified
Attached video verify_video —
This issue has been verified successfully on Flame v2.1 STR: 1. Launch Settings app. 2. Change the system language into Arabic. 3. Back to home and launch Browser app. 4. Tap on address bar. **The layout of keyboard display correctly. See attachment: verify_video.MP4 Reproducing rate: 0/5 Flame 2.1 versions: Gaia-Rev ccb49abe412c978a4045f0c75abff534372716c4 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22 Build-ID 20141201001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141201.034405 FW-Date Mon Dec 1 03:44:15 EST 2014 Bootloader L1TC00011880
Status: RESOLVED → VERIFIED
Mass Edit: adding the [rtl-meta]
Whiteboard: [p=1] → [rtl-meta]
QA Whiteboard: [rtl-impact]
Whiteboard: [rtl-meta]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: