1673054 - Improve API surface of mozIntl and the locale service and decide how directionality-impacting prefs (intl.uidirection and intl.l10n.pseudo) should impact API results

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[:Gijs (he/him)]

Before the patch from bug 1672779, when using intlUtils or mozIntl / Services.intl, results for getLocaleInfo(...).direction were always correct for the underlying language, but ignored these prefs (ie intl.uidirection and intl.l10n.pseudo).

After the patch, the prefs override the result of the API - but they do so even when the request pertains to locales that aren't the current application locale. This likely isn't a super big deal because those prefs are meant for testing anyway, but it could end up being problematic in some situations when testing mixed-directionality content, for instance.

All the while, Services.locale.isAppLocaleRTL can answer the same question (and does get adjusted by the aforementioned prefs), but that cannot be used from UA widgets and other unprivileged code.

It's also not great that we have to duplicate the logic for these prefs into both mozIntl.jsm and the locale service.

So straightening this story out would be a good idea.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

We also have bug 1635561 - many of the features we landed in mozIntl are now part of ECMA402 and are in SpiderMonkey.

I imagine this may be a good first step in the cleanup route.

Comment 2

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/jobs?repo=try&revision=dabe7af5799dbb0d7ddc9833651fbf6b743b032c

Comment 3

[Itiel]

(In reply to Dan Minor [:dminor] from comment #2)

Try run here: https://treeherder.mozilla.org/jobs?repo=try&revision=dabe7af5799dbb0d7ddc9833651fbf6b743b032c

Some concerns about this:

1. In order to debug RTL bugs, I have to first mirror the UI, which it'll have to be by setting intl.l10n.pseudo = bidi from now on. Due to unrelated reasons, I'm sometimes required to clobber. Because the way intl.l10n.pseudo = bidi works is to also flip the English characters, I'll also have to install a Hebrew lang pack everytime so I'd be able to navigate the UI (I can't read flipped English lol).
   Not a huge deal, but a bummer.
   The next issue is more serious if my assumption on what the patch does is right (which is: Mirror the UI by intl.l10n.pseudo = bidi only if the installed lang pack is RTL).
2. When testing bugs, QA is used to just set intl.uidirection = 1 and start testing. Now with intl.l10n.pseudo = bidi as the default, if QA tests Nightly without adding an RTL lang pack (which is what they do IIRC when given a Try build to test), most of the UI will work, but UI that checks for lang directionality will still display that UI as LTR.
3. I'm worried that this will somewhat regress bug 1672779, but for intl.l10n.pseudo = bidi.
4. Apparently there are Persian users that use the Persian lang pack but with LTR layout as they think RTL UI should be the same as LTR UI. How will this affect them?

Feel free to ignore #2 and #3 if I'm reading the patch wrong.

Comment 4

[Dan Minor [:dminor]]

After Zibi raised some concerns in #i18n, I'll be redoing this to expose the existing version of isAppLocaleRTL from LocaleService instead of adding a new one.

Bug 1492587 added the code to check webExposedLocales to the datepicker UI widgets because of concerns about the localized date picker being fingerprintable. I'd like to switch callers over to isAppLocaleRTL to determine if the widget UI should be rendered RTL so we can remove the pref overrides from getLocaleInfo and isLocaleRTL.

This would mean that the text direction would be the app text direction instead of the direction from webExposedLocales (or privacy.spoof_english if it is set). This is fine, so long as the text direction of the UI widgets isn't fingerprintable. I don't see how the videocontrol work being done in Bug 1666637 would be fingerprintable, but I'm not as sure about the date picker. It seems like the concerns in Bug 1492587 with the date picker were about the element width varying with locale due to text length.

If we can't switch callers over to IsAppLocaleRTL because of fingerprinting concerns, we'd lose the ability to easily test the direction of the UI widgets through prefs.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

1. You don't need to install Hebrew lang pack. You can just use pseudo on en-US self build or nightly. Clobber doesn't affect it except of flipping the pref.
2. That's incorrect. If QA will set pseudo=bidi, they'll test RTL and compared to ui.direction they'll actually test RTL layout, string and internationalization just as a real RTL translation would (so, dates, times, numbers etc.)
3. :dminor is working on cleanup after bug 1672779 which is actually a wrong solution to the problem. This conversation is part of that.
4. It will remove their ability to use Persian lang pack as LTR. I really don't think this is something we should support

After chatting with Itiel on Matrix, my conclusion is that we would alleviate most of their concerns if we added a third strategy to Pseudolocalization which would use accented characters just like pseudo=accented does. We may also want to avoid doubling the vowels to make it easier to read. For that mode I also recommend accenting only vowels.

I suggest to name this strategy bidi-accented.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

After some more conversations, :dminor offered to take a look at improving readability of pseudo, so maybe we won't need a new one, just improved algo for the ones we have.

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1673054 - Add isAppLocaleRTL to mozIMozIntl; r=gijs!,zbraniecki!

This adds an isAppLocaleRTL method that calls the corresponding method in
LocaleService. This will make it possible to migrate callers away from
getLocaleInfo for the current locale when attempting to determine whether
to render UI components RTL. Calling getLocaleInfo for this purpose is
problematic because it is difficult to determine whether preference overrides
should be applied.

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1673054 - Add isAppLocaleRTL to IntlUtils; r=gijs!,zbraniecki!

Depends on D96232

Comment 9

[Dan Minor [:dminor]]

Attachment: Bug 1673054 - Convert callers to use isAppLocaleRTL; r=gijs!,zbraniecki!

This moves callers that are using getLocaleInfo to determine the current locale
to render widgets to use the new isAppLocaleRTL method. This will allow us to remove
pref overrides from getLocaleInfo.

Depends on D96233

Comment 10

[Dan Minor [:dminor]]

Attachment: Bug 1673054 - Migrate uses of intl.uidirection to intl.l10n.pseudo; r=gijs!,zbraniecki!

This also removes overrides from methods like LocaleService::IsLocaleRTL or
IntlService.getLocaleInfo, because it doesn't really make sense to override the
result of checking an arbitrary locale, the relevant use case is overriding the
result for the current app locale.

Depends on D96234

Comment 11

[Dan Minor [:dminor]]

I'm not planning to land anything until we improve the readability of the bidi pseudo-locale, but I thought I'd push my patches up in case anyone has feedback in the mean time.

Try run here: https://treeherder.mozilla.org/jobs?repo=try&revision=6bab36b10d3e11caf969af865aff61d0765c22c8

Comment 12

[Dan Minor [:dminor]]

I've filed Bug 1676137 to track removing the intl.uidirection pref completely. I'll still move our test cases to use the bidi pseudolocale as part of this bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ffa0d674900
Add isAppLocaleRTL to IntlUtils; r=Gijs,zbraniecki,emilio
https://hg.mozilla.org/integration/autoland/rev/ade217772368
Convert callers to use isAppLocaleRTL; r=Gijs,zbraniecki
https://hg.mozilla.org/integration/autoland/rev/d19d58218d3c
Migrate uses of intl.uidirection to intl.l10n.pseudo; r=Gijs,zbraniecki

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9ffa0d674900
https://hg.mozilla.org/mozilla-central/rev/ade217772368
https://hg.mozilla.org/mozilla-central/rev/d19d58218d3c