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
Categories
(Core :: Internationalization, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox84 | --- | fixed |
People
(Reporter: Gijs, Assigned: dminor)
References
Details
Attachments
(3 files, 1 obsolete file)
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.
Updated•4 years ago
|
Comment 1•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
(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:
- 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 wayintl.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 byintl.l10n.pseudo = bidi
only if the installed lang pack is RTL). - When testing bugs, QA is used to just set
intl.uidirection = 1
and start testing. Now withintl.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. - I'm worried that this will somewhat regress bug 1672779, but for
intl.l10n.pseudo = bidi
. - 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.
Assignee | ||
Comment 4•4 years ago
|
||
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•4 years ago
|
||
- 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.
- 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.) - :dminor is working on cleanup after bug 1672779 which is actually a wrong solution to the problem. This conversation is part of that.
- 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•4 years ago
|
||
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.
Assignee | ||
Comment 7•4 years ago
|
||
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.
Assignee | ||
Comment 8•4 years ago
|
||
Depends on D96232
Assignee | ||
Comment 9•4 years ago
|
||
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
Assignee | ||
Comment 10•4 years ago
|
||
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
Assignee | ||
Comment 11•4 years ago
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
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•4 years ago
|
||
Comment 14•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ffa0d674900
https://hg.mozilla.org/mozilla-central/rev/ade217772368
https://hg.mozilla.org/mozilla-central/rev/d19d58218d3c
Description
•