Right-to-left CSS

[mapmeld]

Long time in the works, and finally here - right-to-left CSS (and a little JS) for MetaMask #5436

I recently met a blockchain teacher at Re-Coded's coworking space in Erbil, and he is interested in translating MetaMask into Persian (and maybe Kurdish later). On my side I offered to add RTL stylesheets. Here's what it looks like with the translation (which will be in a separate PR)

CSS notes:

• Most changes are wrapped in [dir='rtl'] & { } blocks , this means their internal changes (swapping left and right) happen only if an upper element has set dir="rtl"
• In some cases I have added direction: ltr; to CSS; this will not change any current views but maintains consistency displaying token names, seed phrases, negative numbers, and other LTR content

JS notes:

• When the app launches and when you change languages, RTL settings should be updated by adding dir="rtl" or dir="auto" to the top-level div
• I think there are one or two places where I added dir="auto" to a text field such as account name; this will accommodate typing LTR or RTL languages into that text field
• An array of locales anticipates Arabic, Dhivehi, Hebrew, Persian, Hebrew, and Kurdish as RTL languages. If someone adds another RTL language we would need to update the list there

[mapmeld]

Added one more commit to fix the 'From' and 'To' on transactions, as previously they showed the endings of the crypto addresses

This is different from the other changes as I put the 'From' and 'To' labels in a new <span>. No changes will be visible to users outside of RTL languages

[mapmeld]

I looked into adding an RTL-CSS plugin for Gulp to automate this. The existing plugin creates an alternate .rtl.css file, and changes which CSS file is loaded... I feel like it would be difficult to swap CSS files based on language

[whymarrh]

@mapmeld first, thanks a bunch for this! RTL support is important but we've neglected it, unfortunately. Would you ming rebasing this on the latest develop? That should also fix the test-deps CI job. Once we get back to a green build I'll be happy to give this a more thorough review.

[mapmeld]

@whymarrh Thanks! Rebased so it lines up with current develop branch

[mapmeld]

@whymarrh also - I started making a smaller branch which would use gulp-rtlcss, but I'm not sure how to switch between index.css and index-rtl.css in the way CSS works in MetaMask: https://github.com/mapmeld/metamask-extension/tree/rtlcss_module

[whymarrh]

Two quick comments, otherwise these code changes look pretty good to me

[mapmeld]

@whymarrh I agree with the changes 👍

[Gudahtt]

We might be better off using predefined messages instead of taking this approach. These predefined messages can be referenced directly from CSS, and can be obtained in JavaScript using i18n.getMessage (see here).

The internationalization API appears to be well supported on the browsers we support, and it should make this easier to maintain going forward. We could also reference these predefined messages directly from the manifest, letting us use rlt-specific or locale-specific HTML if need be later on.

[whymarrh]

We might be better off using predefined messages instead of taking this approach.

Where would we use the predefined messages? In the CSS? Or in the HTML on some attr?

[Gudahtt]

I don't believe these messages work in HTML. They're only accessible in JavaScript via i18n.getMessage, and in CSS (where they just... magically work, somehow? 🤷‍♂ ).

For example, the textDirection value derived here from the hard-coded list of 5 rtl locales could instead be obtained from i18n.getMessage('@@bidi_dir'). That places the onus on the browser to determine which locales should be right-to-left, and it automatically supports any new locales in the future.

For most of these CSS changes, we could use @@bidi_start_edge or @@bidi_end_edge in place of left or right. That would let us use a single CSS rule that supports either ltr or rtl without special-casing either. This is explained further in the MDN Internationalization article.

My impression from reading the MDN and the Chrome extension docs is that this is the approach they're recommending for internationalizing extensions. I'm not entirely clear on what this gains us over the current state of this PR though. Getting rid of that hard-coded list of rtl locales seems like a clear win, and an easy change to make, but the CSS changes would be quite substantial.

Maybe the only benefit would be in having less CSS rules (leading to marginally faster parsing+execution)? Who knows. I don't feel strongly about it at this point anyway. I thought it was an interesting solution though.

[mapmeld]

This might be the ideal way to set dir="ltr" or "rtl", rather than having JS figuring it out from our locale. But I wonder if it updates immediately when we change the MetaMask preferred language? If there's interest I could test this.

[Gudahtt]

Excellent point - it looks like my suggestions wouldn't work after all. The browser.i18n APIs use the browser locale, which I suspect it isn't possible to change from within the extension. In order to use the predefined messages as I suggested, we'd also have to remove the ability to change languages from our Settings page, and direct users to change the locale of their browser instead.

Oh well. Disregard my previous suggestions 😄. Maybe we can try that sometime in the future.

[Gudahtt]

So I guess the last consideration is whether to go with this, or something along the lines of your gulp-rtlcss branch. The RTLCSS author has a fairly compelling wiki article explaining the advantages of creating a separate CSS file. The unintuitive specificity in particular is a bit concerning - those sort of errors are hard to spot in review.

There's a lot to like about the RTLCSS approach. It seems like it'd be easier to maintain. I'll experiment tomorrow with switching between the two CSS files. I want to make sure we minimize the performance impact. Assuming we can get that to work, I think that would be a better solution.

[Gudahtt]

This ended up being a bit more challenging than anticipated. I have something that sorta works here, but it isn't quite finished yet. I'll try to find time to finish it and/or explain it in the next few days.

[mapmeld]

@Gudahtt I re-synced with the develop branch. If you want to talk about the branch on Gitter, I sent you a message there

[Gudahtt]

I believe I've gotten the stylesheet switching to work: https://github.com/Gudahtt/metamask-extension/tree/rtlcss_module

That branch is based upon your gulp-rtlcss branch. Let me know what you think! If it seems OK to you, you could reset this branch to point at that commit and we could go from there.

[mapmeld]

Replacing with a switching stylesheet PR in a moment