1177350 - [RTL][Email] The plaintext inline forward auto-generated header block results in awkward rearrangement of parentheses and arrow braces. (ex: '(GMT+0800(CST' and ' "<t" <[e-mail]" ')

Status: RESOLVED DUPLICATE of bug 1157727

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

Description

[Sue]

Attachment: Forward_screenshot1.png

[1.Description]:
[[RTL][Flame v2.2&v3.0][N5 v2.2&v3.0][Email]Forward any received mail, on the forwarding compose view, the 'GMT+0800(CST)' displays as '(GMT+0800(CST'.
See attachment: Forward_screenshot1.png

[2.Testing Steps]: 
Prerequisite: Set system language as Arabic.
1. Log in an E-mail account.
2. Launch E-mail and tap a received mail to enter it.
3. Tap the fifth icon on bottom.
4. Select "Forward" item.
5. Observe the Forwarding compose view.

[3.Expected Result]: 
5. The compose view is displayed as right-aligned and the 'GMT+0800(CST)' should display as 'GMT+0800(CST)'.

[4.Actual Result]: 
5. The compose view is displayed as right-aligned, but the 'GMT+0800(CST)' displays as '(GMT+0800(CST'.

[5.Reproduction build]: 
Device: Flame 2.2 (affected)
Build ID               20150624162504
Gaia Revision          1f8981d7872e3c0053571c26fb3edaf401844d75
Gaia Date              2015-06-19 13:22:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2f8b845e5fa3
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150624.195750
Firmware Date          Wed Jun 24 19:58:02 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 (affected)
Build ID               20150624080416
Gaia Revision          eb0d4aefa62b20420d6fa0642515a110daca5d97
Gaia Date              2015-06-24 01:48:14
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/4cdc1a95a672
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150624.153831
Firmware Date          Wed Jun 24 15:38:44 EDT 2015
Bootloader             L1TC000118D0

Device: N5 2.2 (affected)
Build ID               20150624162504
Gaia Revision          1f8981d7872e3c0053571c26fb3edaf401844d75
Gaia Date              2015-06-19 13:22:30
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2f8b845e5fa3
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150624.195506
Firmware Date          Wed Jun 24 19:55:22 EDT 2015
Bootloader             HHZ12f

Device: N5 3.0 (affected)
Build ID               20150624010204
Gaia Revision          311c4e59936a407e64509f54fecb440d8a78e3c8
Gaia Date              2015-06-20 20:21:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/be81b8d6fae9
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150624.043116
Firmware Date          Wed Jun 24 04:31:35 EDT 2015
Bootloader             HHZ12f

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

Comment 1

[Dylan Oliver [:doliver]]

[Blocking Requested - why for this release]:

Comment 2

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

I assume this is fallout from the platform RTL magic since the "Date:" string is in an RTL language and it's all on one line.  I wonder if we should be inserting special unicode delimiters in our produced string.  I'm going to ask for guidance on dev-gaia since I'm going to board an airplane soon.

Comment 3

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

Hm, actually it looks like our backend logic may be particularly RTL-naive.  The logic looks like this:

  textMsg += l10n_forward_header_labels['date'] + ': ' + new Date(date) + '\n';

I'm going to look at how Thunderbird handles this; maybe we should be leaving more of this up to the string authors or maybe we need to be inject unicode delimiting characters to prevent the automatic logic from coming into play.

Comment 4

[Dylan Oliver [:doliver]]

Blocking for continued RTL support.

Comment 5

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

Okay, so I dug into what Thunderbird does.  It is not actually smarter than us.

In the event of forwarding, mime_insert_normal_headers (https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#770) or its friends are called, and for each header it basically:
- Does a straight-forward header => localized string for that header lookup.
  - These are numeric constants #defined in https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMimeStringResources.h where for example MIME_MHTML_TO is 1012.
  - MimeGetNamedString does this by looking in chrome://messenger/locale/mime.properties for lines like "1012=To"
  - Examples:
    - ar: http://hg.mozilla.org/l10n-central/ar/file/79e47abcafe7/mail/chrome/messenger/mime.properties
    - es-ES: http://hg.mozilla.org/l10n-central/es-ES/file/600b838b359d/mail/chrome/messenger/mime.properties
- mime_intl_insert_message_header_1 (https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#550) gets invoked to do string injection:
  - If plaintext, it inserts a newline followed by that translated string followed by ": " followed by the string contents of the header after header decoding.  Note that for things like Date we had parsed the date up and are re-stringifying it by using the Date object's toString which adds the timezone and the parentheses which creates a problem right there.  (The bug 1177351 variant of this for the `"display name" <braced@email>` scenario is not a place where Thunderbird lucks out though.
  - If HTML, it uses the following snippets:
#define MIME_HEADER_TABLE             "<TABLE CELLPADDING=0 CELLSPACING=0 BORDER=0 class=\"moz-email-headers-table\">"
#define HEADER_START_JUNK             "<TR><TH VALIGN=BASELINE ALIGN=RIGHT NOWRAP>"
#define HEADER_MIDDLE_JUNK            ": </TH><TD>"
#define HEADER_END_JUNK               "</TD></TR>"
and </TABLE>

So the main thing going on is the table means that the RTL label lives in its own little box where automatic rules can only impact the colon while the non-RTL email addresses and date then live in their own box where the RTL rules don't apply.


Unfortunately, since we do want to be able to forward plaintext messages without performing a plaintext upgrade, the table isn't actually good enough.  Happily there is something we can do, although it is potentially horrible for interop reasons.  Per http://www.unicode.org/reports/tr9/ section 2, Unicode 6.3 introduced "explicit directional isolates" (section 2.4), where we can use FSI and PDI roughly like we could use <bdi></bdi>/<bdi dir="auto"></bdi>.  Specifically, these magical characters cause the thing we surround with FSI (First Strong Isolate) and PDI (Pop Directional Isolate) which can avoid the algorithm deciding to mirror things and what not.

For most headers, I think we would FSI/PDI the header value as a whole.  For the `"display name" <email@address>` scenario, I think we would want to wrap the display name for sure.  Based on section 6.3, I think this means `"FSIdisplay namePDI".  Because of our probable lack of IDN support and the state of email in the wild, we expect the mail address to probably be LTR-strong and result in things looking reasonable.  But if there's any RTL characters in the email address itself, maybe wacky stuff happens.  It doesn't help that the arrow-bracing the email address is really a MIME representation artifact.

The Gecko state of things here is:
- We're not fully unicode 6.3 bidi-algorithm compliant yet; that's bug 922963
- However, we do have isolates because bug 1157726 was fixed
- Bug 1157727 is about implementing paired brackets (http://www.unicode.org/reports/tr9/#Paired_Brackets) which seems like it would actually make this problem not happen at all if it was implemented.  Which is frustrating.

So it seems like we have a few options:
1) Yeah, FSI/PDI!  Wooo!  But only when we're in an RTL locale since embedding unicode control characters into all forwards we generate is asking for trouble.

2) Do nothing, it's crazy to embed FSI/PDI which were only introduced in Unicode 6.3 but where Unicode 6.3 also makes this problem magically not happen (maybe).  And maybe this will fix itself in time for v2.5.  (I asked for a ballpark estimate on bug 1157727.)

3) Do nothing in the plaintext case, but do specialize the HTML case to generate tables (with some extra structure for the display names so that they get implicit bdi contexts from their markup somehow?).  Hope that in general everyone is replying to an HTML-formatted message.

4) Make this the locale's problem and require the RTL strings to just embed themselves in an isolate.  (I think explicit embeddings and overrides may still cause contamination?)  This seems likely to be a band-aid in the sense that locales seem unlikely to get this right the first time until a bug gets filed.

5) Change how we structure these header things so it's basically a free-form replace where the locale string is like: "To: {payload}", "Date: {payload}" and it's still the locale's problem but there's less unknown stuff going on from the localizer's perspective.  This also seems lie it won't happen correctly the first time and the ability to do free-form stuff breaks existing email convention and automated incoming message analysis to the limited extent it's possible.

6) Sorta like 1, but use explicit LTR encodes.

7) Oh hey, we actually use contenteditable for our plaintext editing too, so we can actually use appropriate bdi-aware markup that we can strip on persistence.  Unfortunately this won't roundtrip through drafts and so is no good.

8) Ask some RTL people.

I would lean towards 3 (with the hoping aspect of 2) where we leave this bug open for tracking the platform fix, and then make sure we have a different explicit enhancement bug to generate the HTML table with this called out as a win.

Comment 7

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

(In reply to Andrew Sutherland [:asuth] from comment #5)
> 2) Do nothing, it's crazy to embed FSI/PDI which were only introduced in
> Unicode 6.3 but where Unicode 6.3 also makes this problem magically not
> happen (maybe).  And maybe this will fix itself in time for v2.5.  (I asked
> for a ballpark estimate on bug 1157727.)

The ballpark estimate from bug 1157727 comment 2 is "End of September."

I think this means we can wait it out and in the event it turns out it slips, we can do one of these other things.  Marking the dependency.

Comment 9

[Lancy(Leave from Mozilla)]

Attachment: AriesKK_v2.5.png

This issue can be reproduced on latest AriesKK v2.5 build by the same STR in comment 0.
See attachment:AriesKK_v2.5.png.
Reproducing rate:10/10


Device:AriesKK v2.5 build
Build ID               20150814002719
Gaia Revision          39b121515ab8a8c3ea07f26d3ba1dd792e90217c
Gaia Date              2015-08-13 18:25:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/4e883591bb5dff021c108d3e30198a99547eed1e
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150813.235024
Firmware Date          Thu Aug 13 23:50:32 UTC 2015
Bootloader             s1

Comment 10

[Dylan Oliver [:doliver]]

Looks like a straight duplicate of bug 1157727, we can re-open if this is not fixed when that is resolved.