1064617 - (email-rtl) [META] [RTL] Email

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Stephany Wilkes]

This is the meta bug for RTL support in the Email app. The release target for full Gaia (all apps) RTL support is 2.2.

Comment 2

[James Burke [:jrburke]]

Some logistical notes for dev, applies to all rtl bugs, so commenting about it here:

1) There is a good thread on dev-gaia about some mechanics involved in doing the changes:

https://groups.google.com/forum/#!topic/mozilla.dev.gaia/GzIIlTUKvAg

2) I am hoping we can do a bulk change for email for at least this initial set of rtl bugs, and after we land bug 1005446, since that one changes a bunch of files. I am not marking it as a hard dependency, since if we do get pull requests for these bugs before it lands, I would want to take them, and I handle merging them into that other pull request. I am hopeful we can land that bug this week, it is at the point of formal review.

Comment 3

[James Burke [:jrburke]]

Attachment: Chevrons left aligned in email account settings

I have a first pass at the RTL support for email. You can install this app zip via the web IDE (unzip it by double clicking in your file system first):

http://jrburke.com/work/gaia/email-rtl-1.zip

Some notes: I suggest setting up the email account, particularly if it is a gmail account, before switching the phone's locale to the fake "Mirrored English" one. I think our weird, fictional locale gives some servers some trouble.

So it is best if you set up the account and fetch the emails you want to look at before switching the locale. I don't know if it was the locale thing in particular or other phone problems, but I had some trouble going back and forth in accounts, and then getting network operations to the email servers to work. Restarting the phone, in a real locale seemed to help. I do not expect these problem with a real RTL locale.

You can enable the fake locales by going to Settings, Developer, and check the "Psuedo-localization" checkbox. Then you can go to Settings, Language, and select "Mirrored English" which gives an approximation of an RTL language.

Some notes on what is implemented so far:

* Even though the locale is set to "Mirrored English", text input seems to be normal English, so you do not get the full feel of text entry. I would like to have a native Arabic speaker/reader try to input text to confirm we have the inputs wired up correctly. I believe I do (try typing an @ sign in some of the fields, and you can see it jump to the left), but there could be some tweaks needed.

* Similar for in search: when we show matches, we color highlight the match in the search results. It would be good to know from a person who uses a real RTL language that it looks OK, not something we can simulate with the fake locale hack.

* I believe I still have to fix some issues with zooming in the message reader, so if you see weirdness there, it is a known issue.

* I left the chevrons in the settings screens, as they seemed to light up well with other form/select box controls that end up on the left. 

The attachment for this comment shows what that looks like. However, I know this was a tricky UX question. I can remove them if you like, but just trying out an option so that you can see it in practice, and in relation to the other form fields. It is easy to remove or even right-align if you want to try those things.

Once we work out the final story for the chevrons, then I think this baseline implementation is good enough that I would like to land it, then focus on filing bugs for specific cleanup issues.

However, I will wait for ux-review+ and need-info OK from product before asking review on this first round of changes. The dev work is in this branch:

https://github.com/jrburke/gaia/compare/bug1064617-email-rtl

Comment 4

[Juwei Huang[:juwei]]

Comment on attachment 8522580 [details]
Chevrons left aligned in email account settings

Hi James, nice work!!!!
I just tried the patch u provided.
Apart from the issues you mentioned on last comment, it looks overall great!
As far as I know, framework team has started to come out a new idea for chevron.
So we can wait for their updates.

Comment 5

[James Burke [:jrburke]]

I did an update to the zip, I believe I did some things incorrectly in the last one:

* used an attribute selector instead of a psuedo selector (as an end user you would not have seen a difference vs. the first zip).
* I removed dir="auto" from user input form elements, where I needed to make sure they all had it, as well as adding it to some of the display areas where the text came from user input.

I updated those items, and this is the latest zip:

http://jrburke.com/work/gaia/email-rtl-2.zip

You should not notice much difference from the first zip, except that now:

* text ellipsis truncation is correct for LTR text viewed in a RTL language used for the UI.
* text input starts off on the left now instead of the right, but not sure if that is because I only have a US keyboard, need a native speaker with correct keyboard to try it. But it should fix the type of issue noted in bug 1064643, if using LTR input.

Comment 6

[James Burke [:jrburke]]

Aaannnd... version 3:

http://jrburke.com/work/gaia/email-rtl-3.zip

I was aggressive with the dir="auto" and applied it to the rootBodyNode in message_reader, so that the user intent for the message body was reflected, but this causes havoc with the iframe_shims calculations. So for now I removed dir="auto" and expect to have a fix later for all the iframe_shims related issues, including image loading in a separate bug, which I will attach as a blocking bug for this bug.

I expect the dir="auto" is the correct thing for the user intent, we just need to work out some other issues with HTML body display first, and I want to do that as targeted fixes after landing this first pass.

Comment 7

[James Burke [:jrburke]]

Attachment: GitHub pull request

Going to go ahead and start review on this so we can land the first pass at this. 

By landing this changeset, it does not mean this bug is "fixed", but rather we can start opening targeted bugs to fix the more isolated issues, like bug 1099449, instead of the more global issues.

Comment 8

[Stephany Wilkes]

Sorry - clearing the ni? for me since Juwei has this. Overall looks great except for the targeted issues you already mention. THANK YOU! Very nice work.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8523293 [details] [review]
GitHub pull request

r=asuth, ideally with the click logic for RTL fixed in this patch.  Otherwise please spin-off a bug for that (which maybe gets resolved by your html display experiments anyways?).

Comment 10

[James Burke [:jrburke]]

There definitely still needs to be some email body cleanup work, tracked in bug 1099449. The hope is to land this bigger baseline support patch, but not close this bug until we get all follow-on bugs fixed up.

Comment 11

[James Burke [:jrburke]]

Baseline support changeset in Gaia master:
https://github.com/mozilla-b2g/gaia/commit/28a8edaafa3a73c7fc837bd0b543681cb46ed15d

from pull request:
https://github.com/mozilla-b2g/gaia/pull/26162

Still keeping this open since a meta bug, and will get to scrubbing dependent bugs as the next step for this bug.

Comment 12

[Jacob Shen]

[Tracking Requested - why for this release]:

Comment 13

[Stephany Wilkes]

Hi James - Let me know how close the full patch is. Bhavana are evaluating for 2.2 inclusion and timing, not for "full RTL support in 2.2" but for incremental RTL improvements.

Comment 14

[James Burke [:jrburke]]

Stephany: I just scrubbed through the dependent bugs on this ticket. I did a fix for bug 1087066, and I believe the bug about direction in inputs needs a real RTL keyboard to test the full fix. So that leaves these two bugs that I know need work:

* Bug 1099449 - [email] rtl: properly display message bodies: This is for HTML email, plain text emails should be fine.
* Bug 1087051 - Cannot view image in email when language is set to Arabic: This one may just fall out of the fix that is done for bug 1099449, but if not, will likely be related. Keeping it open for now as a separate bug.

So the big missing piece is proper functioning of HTML body display in RTL. I would also not be surprised if we have some smaller nit fixes once we have real RTL users doing some testing with a real RTL keyboard installed.

The changes done for this ticket so far should be safe to carry in 2.2, should not affect LTR users, and I would not want to back them out for 2.2. 

I would like to just get to fixing the above HTML body bugs in the 2.2 timeframe, just so we can mentally feel this is ready for real user testing, but may not make it depending on holiday schedules and if other things in 2.2 get scheduled that steal time.

Comment 15

[Peter Bylenga [:PBylenga]]

Adding whiteboard tag [rtl-impact]

Comment 16

[Peter Bylenga [:PBylenga]]

Mass Edit: adding the [rtl-meta]