task(fxa-settings): Revert localized content in key download file #15437
Conversation
packages/fxa-settings/src/components/ButtonDownloadRecoveryKey/index.tsx
Outdated
Show resolved
Hide resolved
| // Non-latin text (e.g., Hebrew) may be displayed as jibberish due to incorrect encoding detection. | ||
| // In addition, localized strings contain important directionality markers that are hidden on Mac | ||
| // but visible on many other devices. On Apple devices, these directionality markers get copied with the key, | ||
| // and the key is rejected as invalid during password reset. |
There was a problem hiding this comment.
What do these markers look like? Is it something we can strip on submit like we do spaces? Doesn't help with the comment about Hebrew though...
What do our localized plaintext emails look like, are they not experiencing these issues? If they aren't, the fix (not in this PR) might be to repurpose our Localizer class to be either used here (sounds like Reino was already thinking about using it client-side), or the logic needed from it pulled and used. We probably need a similar ticket filed if our emails experience the same thing.
There was a problem hiding this comment.
The markers are rendered differently depending on the device/application. Sometimes invisible, sometimes square boxes, sometimes little symbols with FSI/PDI letters.
We could strip them out on submit or leave the key out of the localized string to resolve FXA-7571 , but we're left with the issue that there are directionality markers around other bits of text in this download file including brand terms, and that doesn't look great on most devices/applications except Apple's FXA-7564.
Our plain text emails do include directionality markers, but they're hidden in maildev. Not sure if they might be visible in other email clients. The confirmation codes are fine functionality-wise because they're on their own line and not included as a variable in a localized string --> no directionality marker(s) around the emailed codes.
There was a problem hiding this comment.
Well, that is a tricky problem. I'm just reiterating the ticket here but looks like Windows is not respecting the directionality markers at all if I'm seeing the screenshots correctly; English shows the unicode control characters and renders LTR (probably default) and RTL languages show the unicode control characters and are still rendering LTR.
Notepad is notorious for not displaying unicode correctly, but it's strange it's also occurring in Notepad++... If we don't use PDFs (unsure if that would solve this), this is not ideal but I wonder if Windows would interpret "overrides" or "embedding" better than "isolates"? I'm not sure if we can parse the text and replace [U+2068] with \u202E (not 100% sure that's how it'd need to be in code) or if that would need to happen on the l10n side somehow. 🤔
There was a problem hiding this comment.
TLDR; Getting encoding right on all devices/apps is very tricky. For the time being, I've reverted the default download to "key only" but included a few options that we can test in staging with a param. Also filed a spike to investigate options https://mozilla-hub.atlassian.net/browse/FXA-7790 because this testing was growing beyond the scope of this ticket.
Windows is not respecting the directionality markers at all
The directionality markers are useful when the text contains a mix of LTR and RTL text, but it won't specify if the file as a whole should be displayed in RTL. If the Windows OS is set to Hebrew (for example), text in Notepad will be displayed in RTL by default (or so I've understood). Switching between RTL and LTR is manual if the text direction does not match the OS direction (this is also true for Mac).
English shows the unicode control characters
The FSI/PDI characters can be optionally shown/hidden in Notepad with app settings, we just can't assume that the characters will be hidden by default.
If we don't use PDFs (unsure if that would solve this)
Switching to PDF will have it's own set of problems, because then we'll have to conditionally apply styling incl. directionality markers to the PDF that we're generating on the fly, based on the user's locale (BUT at least we would have more control over the output... maybe. PDFs can also have problems with encoding http://www.rdpslides.com/psfaq/ENCODING.PDF)
I wonder if Windows would interpret "overrides" or "embedding" better than "isolates"? I'm not sure if we can parse the text and replace
[U+2068]with\u202E(not 100% sure that's how it'd need to be in code) or if that would need to happen on the l10n side somehow. 🤔
I'm not sure if Windows might interpret those other characters more easily, but the trouble with "overrides" and "embedding" is that they can have spillover effect on surrounding text. The Unicode standards recommend using "isolates".
| // and the key is rejected as invalid during password reset. | ||
|
|
||
| // Not tested - could possibly force recognition of UTF-8 encoding by adding BOM at the beginning of the blob | ||
| const BOM = new Uint8Array([0xef, 0xbb, 0xbf]); |
There was a problem hiding this comment.
Does BOM mean anything in particular? Also if you want to test this in staging (if we're not planning to go to prod right now anyway), we could potentially enable this with a parameter instead of setting keyWithContext to false.
There was a problem hiding this comment.
BOM stands for "byte order mark". I've read that it can be used to identify the encoding of a text file, but I'm not sure if it would resolve the issue where some text is jibberish on Android.
https://learn.microsoft.com/en-us/globalization/encoding/byte-order-mark
Good call on enabling with a parameter so we can test if it makes a difference on stage.
There was a problem hiding this comment.
@vpomerleau Ah, TIL. Have you tried specifying charset in the type or maybe there's another way to force utf-8? Like, type: 'text/plain;charset=utf-8'. Not sure if that'll work though.
You could maybe also try TextEncoder to convert each string to utf-8.
| # "Key" here refers to the term "account recovery key" | ||
| recovery-key-file-key-value-v2 = Key: { $recoveryKeyValue } | ||
| # "Key" here refers to the term "account recovery key", a randomly generated code in latin characters | ||
| recovery-key-file-key-value-v3 = Key: |
There was a problem hiding this comment.
@bcolsson With this PR, the download file only contains the key unless we add a query parameter to the URL (on local and stage only). With the query param enabled, we can test the detailed file with the key removed from the localized string.
There was a problem hiding this comment.
Is in latin characters accurate? It can contain numbers 0-9 and any letters with the exception of I, L, O, and U. Probably not useful to say it's base32 Crockford 😝, maybe just a 32-character generated code?
There was a problem hiding this comment.
base32 Crockford might be too specific here, but I've clarified the comment 👍
|
Found an old Android device I can use for testing - switching back to draft to test @LZoog's ideas. |
Because: * Localized plain text file is not rendering as expected on Android and Windows. * Hidden directionality characters get copied with key, resulting in the key being marked as invalid during password reset. * Reversal is temporary to prevent this issue from blocking deployment while next steps are determined. This commit: * Serve key only in the file download. * Add an optional query param to test downloading a detailed file 1) with BOM (byte-order-mark), 2) with charset defined, or 3) with TextEncoder * Add stories to download the 3 test options Closes #FXA-7688
Because
This pull request
Issue that this pull request solves
Closes: #FXA-7688
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.