Skip to content

Commit

Permalink
Bug 1673054 - Migrate uses of intl.uidirection to intl.l10n.pseudo; r…
Browse files Browse the repository at this point in the history
…=Gijs,zbraniecki

This also removes pref 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.

Removal of the intl.uidirection pref completely will be done in a separate bug.

Differential Revision: https://phabricator.services.mozilla.com/D96235
  • Loading branch information
dminor committed Nov 9, 2020
1 parent f9d9764 commit 47fd0fe
Show file tree
Hide file tree
Showing 22 changed files with 38 additions and 88 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ add_task(async function testArrowsInPanelMultiView() {

// Test that right/left arrows move in the expected direction for RTL locales.
add_task(async function testArrowsRtl() {
await SpecialPowers.pushPrefEnv({ set: [["intl.uidirection", 1]] });
await SpecialPowers.pushPrefEnv({ set: [["intl.l10n.pseudo", "bidi"]] });
// window.RTL_UI doesn't update in existing windows when this pref is changed,
// so we need to test in a new window.
let win = await BrowserTestUtils.openNewBrowserWindow();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,10 @@ add_task(async function test_sidebarpanels_click() {

for (let test of tests) {
gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
await pushPref("intl.uidirection", 0);
info("Running " + test.desc + " in LTR mode");
await testPlacesPanel(test);
await popPref();

await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
info("Running " + test.desc + " in RTL mode");
await testPlacesPanel(test);
await popPref();
Expand Down
4 changes: 3 additions & 1 deletion browser/components/syncedtabs/SyncedTabsDeckComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ SyncedTabsDeckComponent.prototype = {
// Add app locale change support for HTML sidebar
Services.obs.addObserver(this, "intl:app-locales-changed");
Services.prefs.addObserver("intl.uidirection", this);
Services.prefs.addObserver("intl.l10n.pseudo", this);
this.updateDir();

// Go ahead and trigger sync
Expand All @@ -125,6 +126,7 @@ SyncedTabsDeckComponent.prototype = {
Services.obs.removeObserver(this, UIState.ON_UPDATE);
Services.obs.removeObserver(this, "intl:app-locales-changed");
Services.prefs.removeObserver("intl.uidirection", this);
Services.prefs.removeObserver("intl.l10n.pseudo", this);
this._deckView.destroy();
},

Expand All @@ -141,7 +143,7 @@ SyncedTabsDeckComponent.prototype = {
this.updateDir();
break;
case "nsPref:changed":
if (data == "intl.uidirection") {
if (data == "intl.uidirection" || data == "intl.l10n.pseudo") {
this.updateDir();
}
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ add_task(async function testObserver() {
"locale change triggers UI direction update"
);

Services.prefs.setIntPref("intl.uidirection", 1);
Services.prefs.setStringPref("intl.l10n.pseudo", "bidi");

Assert.equal(
component.updateDir.callCount,
Expand Down
1 change: 0 additions & 1 deletion chrome/nsIChromeRegistry.idl
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ interface nsIChromeRegistry : nsISupports
interface nsIXULChromeRegistry : nsIChromeRegistry
{
// Get whether the default writing direction of the locale is RTL
// (or may be overridden by intl.uidirection pref)
boolean isLocaleRTL(in ACString package);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ requestLongerTimeout(4);

// Test that DevTools panels are rendered in "rtl" (right-to-left) in the Browser Toolbox.
add_task(async function() {
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");

const ToolboxTask = await initBrowserToolboxTask();
await ToolboxTask.importFunctions({});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ add_task(async function() {
CHROME_URL_ROOT + "current-time-scrubber_head.js",
this
);
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
// eslint-disable-next-line no-undef
await testCurrentTimeScrubber(true);
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ add_task(async function() {
CHROME_URL_ROOT + "keyframes-graph_keyframe-marker_head.js",
this
);
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
// eslint-disable-next-line no-undef
await testKeyframesGraphKeyframesMarker();
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ add_task(async function() {
CHROME_URL_ROOT + "summary-graph_delay-sign_head.js",
this
);
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
// eslint-disable-next-line no-undef
await testSummaryGraphDelaySign();
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ add_task(async function() {
CHROME_URL_ROOT + "summary-graph_end-delay-sign_head.js",
this
);
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
await testSummaryGraphEndDelaySign();
});
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ add_task(async function() {
await inspectorResized;

info("Testing transitions ltr");
await pushPref("intl.uidirection", 0);
await pushPref("intl.l10n.pseudo", "");
await testBreadcrumbTransitions(hostWindow, inspector);

info("Testing transitions rtl");
await pushPref("intl.uidirection", 1);
await pushPref("intl.l10n.pseudo", "bidi");
await testBreadcrumbTransitions(hostWindow, inspector);

hostWindow.resizeTo(originalWidth, originalHeight);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ add_task(async function() {
});

async function testForGivenDir(dir) {
await pushPref("intl.uidirection", dir === "rtl" ? 1 : -1);
if (dir === "rtl") {
await pushPref("intl.l10n.pseudo", "bidi");
} else {
await pushPref("intl.l10n.pseudo", "");
}

// Reset visibleColumns so we only get the default ones
// and not all that are set in head.js
Expand Down
2 changes: 0 additions & 2 deletions devtools/docs/contributing/css.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,6 @@ The appropriate value for the `dir` attribute will then be set when the toolbox

The recommended workflow to test RTL on DevTools is to use the [Force RTL extension](https://addons.mozilla.org/en-US/firefox/addon/force-rtl/). After changing the direction using Force RTL, you should restart DevTools to make sure all modules apply the new direction. A future version of Force RTL will be able to update dynamically all DevTools documents.<!--TODO: update when the fate of this addon/webextension is known-->

Going to `about:config` and setting `intl.uidirection.en` to rtl is not recommended, and will always require to re-open DevTools to have any impact.

## Toggles

Sometimes you have a style that you want to turn on and off. For example a tree twisty (a expand-collapse arrow), a tab background, etc.
Expand Down
4 changes: 1 addition & 3 deletions intl/l10n/docs/fluent/tutorial.rst
Original file line number Diff line number Diff line change
Expand Up @@ -621,9 +621,7 @@ The three classes of potential problems that this can help with are:
- Bidi adaptation.
For many developers, testing the UI in right-to-left mode is hard. Mozilla
offers a pref :js:`intl.uidirection` which switches the direction of the layout,
but that doesn't expose problems related to right-to-left text.
For many developers, testing the UI in right-to-left mode is hard.
Pseudolocalization shows how a right-to-left locale will look like.
To turn on pseudolocalization, add a new string pref :js:`intl.l10n.pseudo` and
Expand Down
10 changes: 5 additions & 5 deletions intl/locale/LocaleService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,18 @@ void LocaleService::LocalesChanged() {
}

bool LocaleService::IsLocaleRTL(const nsACString& aLocale) {
return unic_langid_is_rtl(&aLocale);
}

bool LocaleService::IsAppLocaleRTL() {
// First, let's check if there's a manual override
// preference for directionality set.
int pref = Preferences::GetInt("intl.uidirection", -1);
if (pref >= 0) {
return (pref > 0);
}

return unic_langid_is_rtl(&aLocale);
}

bool LocaleService::IsAppLocaleRTL() {
// First, check if there is a pseudo locale `bidi` set.
// Next, check if there is a pseudo locale `bidi` set.
nsAutoCString pseudoLocale;
if (NS_SUCCEEDED(Preferences::GetCString("intl.l10n.pseudo", pseudoLocale))) {
if (pseudoLocale.EqualsLiteral("bidi")) {
Expand Down
2 changes: 0 additions & 2 deletions intl/locale/LocaleService.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,6 @@ class LocaleService final : public mozILocaleService,

/**
* Returns whether the locale is RTL.
*
* This method respects the `intl.uidirection` pref override.
*/
static bool IsLocaleRTL(const nsACString& aLocale);

Expand Down
12 changes: 9 additions & 3 deletions intl/locale/tests/gtest/TestLocaleService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,13 @@ TEST(Intl_Locale_LocaleService, GetDefaultLocale)

TEST(Intl_Locale_LocaleService, IsAppLocaleRTL)
{
// For now we can only test if the method doesn't crash.
LocaleService::GetInstance()->IsAppLocaleRTL();
ASSERT_TRUE(true);
mozilla::Preferences::SetCString("intl.l10n.pseudo", "bidi");
ASSERT_TRUE(LocaleService::GetInstance()->IsAppLocaleRTL());
mozilla::Preferences::ClearUser("intl.l10n.pseudo");

mozilla::Preferences::SetInt("intl.uidirection", 0);
ASSERT_FALSE(LocaleService::GetInstance()->IsAppLocaleRTL());
mozilla::Preferences::SetInt("intl.uidirection", 1);
ASSERT_TRUE(LocaleService::GetInstance()->IsAppLocaleRTL());
mozilla::Preferences::SetInt("intl.uidirection", -1);
}
1 change: 0 additions & 1 deletion intl/locale/tests/unit/test_localeService.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ add_test(function test_isAppLocaleRTL_pseudo() {

localeService.availableLocales = ["en-US"];
localeService.requestedLocales = ["en-US"];
Services.prefs.setIntPref("intl.uidirection", -1);
Services.prefs.setCharPref("intl.l10n.pseudo", "");

Assert.ok(localeService.isAppLocaleRTL === false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,14 @@ async function test_i18n_css(options = {}) {

// We don't currently have a good way to mock this.
if (false) {
const DIR = "intl.uidirection";
const DIR = "intl.l10n.pseudo";

// We don't wind up actually switching the chrome registry locale, since we
// don't have a chrome package for Hebrew. So just override it, and force
// RTL directionality.
const origReqLocales = Services.locale.requestedLocales;
Services.locale.requestedLocales = ["he"];
Preferences.set(DIR, 1);
Preferences.set(DIR, "bidi");

css = await fetch(baseURL + "locale.css");
equal(
Expand Down
25 changes: 1 addition & 24 deletions toolkit/components/mozintl/mozIntl.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
const { XPCOMUtils } = ChromeUtils.import(
"resource://gre/modules/XPCOMUtils.jsm"
);

const mozIntlHelper = Cc["@mozilla.org/mozintlhelper;1"].getService(
Ci.mozIMozIntlHelper
Expand Down Expand Up @@ -768,20 +765,6 @@ class MozIntl {
constructor() {
this._cache = {};
Services.obs.addObserver(this, "intl:app-locales-changed", true);
XPCOMUtils.defineLazyPreferenceGetter(
this,
"_forcedDir",
"intl.uidirection",
-1,
() => this.observe()
);
XPCOMUtils.defineLazyPreferenceGetter(
this,
"_pseudo",
"intl.l10n.pseudo",
"",
() => this.observe()
);
}

observe() {
Expand Down Expand Up @@ -810,13 +793,7 @@ class MozIntl {
mozIntlHelper.addGetLocaleInfo(this._cache);
}

let info = this._cache.getLocaleInfo(getLocales(locales), ...args);
if (this._pseudo == "bidi") {
info.direction = "rtl";
} else if (this._forcedDir != -1) {
info.direction = this._forcedDir == 1 ? "rtl" : "ltr";
}
return info;
return this._cache.getLocaleInfo(getLocales(locales), ...args);
}

getAvailableLocaleDisplayNames(type) {
Expand Down
29 changes: 0 additions & 29 deletions toolkit/components/mozintl/test/test_mozintl.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ function run_test() {
test_methods_presence();
test_methods_calling();
test_constructors();
test_direction_adaptation();
test_rtf_formatBestUnit();

ok(true);
Expand Down Expand Up @@ -59,34 +58,6 @@ function test_constructors() {
});
}

function test_direction_adaptation() {
{
let { direction } = Services.intl.getLocaleInfo("en-US");
equal(direction, "ltr", "Should get ltr for English.");
}
Services.prefs.setIntPref("intl.uidirection", 1);
{
let { direction } = Services.intl.getLocaleInfo("en-US");
equal(direction, "rtl", "Should now get rtl for English.");
}
Services.prefs.clearUserPref("intl.uidirection");
{
let { direction } = Services.intl.getLocaleInfo("en-US");
equal(direction, "ltr", "Should get ltr for English again.");
}

Services.prefs.setCharPref("intl.l10n.pseudo", "bidi");
{
let { direction } = Services.intl.getLocaleInfo("en-US");
equal(direction, "rtl", "Should get rtl after enabling pseudolocale.");
}
Services.prefs.clearUserPref("intl.l10n.pseudo");
{
let { direction } = Services.intl.getLocaleInfo("en-US");
equal(direction, "ltr", "Should finally have ltr for English again.");
}
}

function testRTFBestUnit(anchor, value, expected) {
let rtf = new Services.intl.RelativeTimeFormat("en-US");
deepEqual(rtf.formatBestUnit(new Date(value), { now: anchor }), expected);
Expand Down
4 changes: 2 additions & 2 deletions toolkit/content/tests/chrome/test_bug437844.xhtml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=348233
SimpleTest.waitForExplicitFinish();

let prefs = SpecialPowers.Services.prefs.nsIPrefBranch;
prefs.setIntPref("intl.uidirection", 1);
prefs.setCharPref("intl.l10n.pseudo", "bidi");

let rootDir = getRootDirectory(window.location.href);
let manifest = rootDir + "rtlchrome/rtl.manifest";
Expand Down Expand Up @@ -76,7 +76,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=348233
is(frame.contentDocument.body.dir, "rtl", "file:// listings should be RTL in RTL locales");

cleanupFunc();
prefs.clearUserPref("intl.uidirection");
prefs.clearUserPref("intl.l10n.pseudo");
SimpleTest.finish();
});
document.documentElement.appendChild(frame);
Expand Down

0 comments on commit 47fd0fe

Please sign in to comment.