Merge pull request #153 from sillsdev/rtl

fix: report unknown script reading direction instead of guessing left-to-right

Author: hatton

components/language-chooser/common/find-language/findLanguageInterfaces.spec.ts

@@ -0,0 +1,135 @@
+import { describe, expect, it } from "vitest";
+import { iso15924 } from "iso-15924";
+import { isRTLScript } from "./findLanguageInterfaces";
+
+describe("isRTLScript", () => {
+  it("reports true for right-to-left scripts", () => {
+    expect(isRTLScript("Arab")).toBe(true);
+    expect(isRTLScript("Hebr")).toBe(true);
+    expect(isRTLScript("Thaa")).toBe(true);
+    expect(isRTLScript("Nkoo")).toBe(true);
+    expect(isRTLScript("Adlm")).toBe(true);
+  });
+
+  it("reports false for left-to-right scripts", () => {
+    expect(isRTLScript("Latn")).toBe(false);
+    expect(isRTLScript("Cyrl")).toBe(false);
+    expect(isRTLScript("Hans")).toBe(false);
+    expect(isRTLScript("Deva")).toBe(false);
+    expect(isRTLScript("Ethi")).toBe(false);
+  });
+
+  it("reports true for RTL scripts whose direction the runtime's ICU may not know yet", () => {
+    // CLDR release-48-2 marks Sidetic RTL=YES, but Node 22's ICU reports it as
+    // left-to-right. This assertion may start passing for the ordinary reason
+    // once the host ICU picks up Unicode 16, at which point the pin for it in
+    // RTL_SCRIPTS_UNKNOWN_TO_OLDER_ICU can be dropped.
+    expect(isRTLScript("Sidt")).toBe(true);
+  });
+
+  it("takes the direction of the parent script for variant codes", () => {
+    // Neither Intl nor CLDR has data for these, but a Nastaliq Arabic document
+    // is still Arabic and Western Syriac is still Syriac. Intl reports all of
+    // them as left-to-right. amw-Syrj (Western Neo-Aramaic) is reachable from a
+    // search, making it the one genuinely user-facing fix here.
+    expect(isRTLScript("Aran")).toBe(true); // Arabic (Nastaliq variant)
+    expect(isRTLScript("Syre")).toBe(true); // Syriac (Estrangelo variant)
+    expect(isRTLScript("Syrj")).toBe(true); // Syriac (Western variant)
+    expect(isRTLScript("Syrn")).toBe(true); // Syriac (Eastern variant)
+    expect(isRTLScript("Phlv")).toBe(true); // Book Pahlavi
+
+    // Variants of left-to-right scripts must stay left-to-right, not become
+    // unknown just because there is no data for the variant code itself.
+    expect(isRTLScript("Cyrs")).toBe(false); // Cyrillic (Old Church Slavonic)
+    expect(isRTLScript("Latf")).toBe(false); // Latin (Fraktur variant)
+    expect(isRTLScript("Latg")).toBe(false); // Latin (Gaelic variant)
+    expect(isRTLScript("Hans")).toBe(false); // Han (Simplified variant)
+    expect(isRTLScript("Hant")).toBe(false); // Han (Traditional variant)
+  });
+
+  it("does not let a placeholder inherit a direction from its parent", () => {
+    // Zsye is "Symbols (Emoji variant)", so the variant derivation would map it
+    // to Zsym. Placeholders must keep their own answer instead.
+    expect(isRTLScript("Zsye")).toBeUndefined();
+  });
+
+  it("does not invent a right-to-left direction for left-to-right scripts", () => {
+    // Todhri is explicitly RTL=NO in CLDR scriptMetadata and Intl agrees.
+    // sq-Todr is reachable from a search, so wrongly pinning this as RTL would
+    // misrender real Albanian text. Guards against re-adding it as an override.
+    expect(isRTLScript("Todr")).toBe(false);
+    // Egyptian hieroglyphs are left-to-right per Unicode's Bidi_Class data.
+    expect(isRTLScript("Egyp")).toBe(false);
+  });
+
+  it("keeps a usable direction for Braille", () => {
+    // CLDR marks Braille RTL=UNKNOWN because it is script agnostic, but
+    // Braille is read left to right, and 148 languages in our data offer it.
+    expect(isRTLScript("Brai")).toBe(false);
+  });
+
+  it("reports unknown for placeholder script codes", () => {
+    // Zxxx covers the sign languages in our data: not merely unknown
+    // direction, but no written form at all.
+    expect(isRTLScript("Zxxx")).toBeUndefined();
+    expect(isRTLScript("Zzzz")).toBeUndefined();
+    expect(isRTLScript("Zyyy")).toBeUndefined();
+    expect(isRTLScript("Zinh")).toBeUndefined();
+    expect(isRTLScript("Zmth")).toBeUndefined();
+    expect(isRTLScript("Zsym")).toBeUndefined();
+    expect(isRTLScript("Zsye")).toBeUndefined();
+  });
+
+  it("reports unknown for private use script codes", () => {
+    expect(isRTLScript("Qaaa")).toBeUndefined();
+    expect(isRTLScript("Qaap")).toBeUndefined();
+    expect(isRTLScript("Qabx")).toBeUndefined();
+  });
+
+  it("reports unknown for codes that are not registered scripts", () => {
+    // Well formed but unregistered. Intl answers "ltr" for these, which is a
+    // guess rather than information.
+    expect(isRTLScript("Xyzw")).toBeUndefined();
+    expect(isRTLScript("Qzzz")).toBeUndefined();
+  });
+
+  it("reports unknown for empty or malformed codes", () => {
+    expect(isRTLScript("")).toBeUndefined();
+    expect(isRTLScript("xyz")).toBeUndefined();
+    expect(isRTLScript("Latn-x")).toBeUndefined();
+    expect(isRTLScript("not a script code")).toBeUndefined();
+  });
+
+  it("is not case sensitive", () => {
+    expect(isRTLScript("arab")).toBe(true);
+    expect(isRTLScript("ARAB")).toBe(true);
+    expect(isRTLScript("latn")).toBe(false);
+    expect(isRTLScript("zxxx")).toBeUndefined();
+  });
+
+  // Guards the placeholder and variant handling against accidentally
+  // suppressing a real direction: whenever the runtime's own ICU data says a
+  // script is right-to-left, we must answer true. Note this deliberately tests
+  // for "not true" rather than "false" — returning undefined suppresses a real
+  // direction just as effectively as returning false does.
+  it("never suppresses a right-to-left direction the runtime reports", () => {
+    const suppressed = iso15924
+      .map(({ code }) => code)
+      .filter((code) => {
+        let intlSaysRtl = false;
+        try {
+          const locale = new Intl.Locale(`und-${code}`);
+          const info =
+            locale.getTextInfo?.() ??
+            (locale as unknown as { textInfo?: { direction?: string } })
+              .textInfo;
+          intlSaysRtl = info?.direction === "rtl";
+        } catch {
+          return false;
+        }
+        return intlSaysRtl && isRTLScript(code) !== true;
+      });
+
+    expect(suppressed).toEqual([]);
+  });
+});

components/language-chooser/common/find-language/findLanguageInterfaces.ts

@@ -1,3 +1,5 @@
+import { iso15924 } from "iso-15924";
+
 export interface IRegion {
   name: string;
   code: string;
@@ -6,6 +8,11 @@ export interface IRegion {
 export interface IScript {
   code: string;
   name: string;
+  // true = right-to-left, false = left-to-right, undefined = we don't know.
+  // Undefined is a real and meaningful state: see isRTLScript below for the
+  // cases that produce it. Consumers that need a hard boolean should decide
+  // their own fallback (`script.isRtl ?? false`) rather than assume we
+  // determined the direction to be left-to-right.
   isRtl?: boolean;
   languageNameInScript?: string;
 }
@@ -58,6 +65,114 @@ export interface IOrthography {
   customDetails?: ICustomizableLanguageDetails;
 }
 
+// ISO 15924 codes which are placeholders rather than actual scripts, so
+// reading direction is either unknown or not applicable. Zxxx in particular
+// covers 165 languages in our data (mostly sign languages), which have no
+// written form and therefore no reading direction at all. Intl reports all of
+// these as "ltr", which is a fabricated answer rather than a real one.
+const SCRIPT_CODES_WITH_NO_DIRECTION = new Set([
+  "Zinh", // inherited
+  "Zmth", // mathematical notation
+  "Zsye", // symbols (emoji variant)
+  "Zsym", // symbols
+  "Zxxx", // unwritten
+  "Zyyy", // undetermined
+  "Zzzz", // uncoded
+]);
+
+// Scripts a runtime's ICU build may not know are right-to-left yet. This is
+// deliberately NOT a mirror of CLDR's RTL list: Intl agrees with CLDR on 178
+// of the 179 scripts CLDR has an explicit verdict for, so duplicating that
+// list would add a second source of truth to maintain for no benefit. Only
+// genuine gaps belong here, and entries should be deleted as ICU catches up.
+//
+// Verified against field 6 (RTL) of CLDR release-48-2 scriptMetadata.txt:
+// https://github.com/unicode-org/cldr/blob/release-48-2/common/properties/scriptMetadata.txt
+const RTL_SCRIPTS_UNKNOWN_TO_OLDER_ICU = new Set([
+  // Sidetic, added in Unicode 16. Node 22 reports it as left-to-right.
+  "Sidt",
+]);
+
+const ISO_15924_CODES = new Set(iso15924.map((script) => script.code));
+
+// ISO 15924 states variant relationships in its own script names: Aran is
+// "Arabic (Nastaliq variant)", Syrj is "Syriac (Western variant)", and so on.
+// Neither Intl nor CLDR carries direction data for those variant codes, but a
+// Nastaliq Arabic document is still Arabic, so we take the parent's direction.
+//
+// This is derived from the registry rather than hand-listed so that a variant
+// code added upstream is picked up when the iso-15924 dependency is bumped,
+// and so nobody has to trust a transcribed table. It currently resolves:
+//   Aran -> Arab, Syre/Syrj/Syrn -> Syrc  (these four change the answer)
+//   Cyrs -> Cyrl, Latf/Latg -> Latn, Hans/Hant -> Hani  (same answer either way)
+const SCRIPT_CODE_VARIANT_PARENTS: ReadonlyMap<string, string> = (() => {
+  const codesByScriptName = new Map<string, string>();
+  for (const { code, name, pva } of iso15924) {
+    codesByScriptName.set(name.toLowerCase(), code);
+    if (pva) codesByScriptName.set(pva.toLowerCase().replace(/_/g, " "), code);
+  }
+
+  const parents = new Map<string, string>();
+  for (const { code, name } of iso15924) {
+    // Matches "<parent script name> (<qualifier> variant)".
+    const match = name.match(/^(.+?)\s*\([^)]*variant[^)]*\)$/i);
+    if (!match) continue;
+    const parent = codesByScriptName.get(match[1].trim().toLowerCase());
+    // A placeholder keeps its own "no direction" answer; Zsye is "Symbols
+    // (Emoji variant)" and must not inherit anything from Zsym.
+    if (
+      parent &&
+      parent !== code &&
+      !SCRIPT_CODES_WITH_NO_DIRECTION.has(code)
+    ) {
+      parents.set(code, parent);
+    }
+  }
+
+  // The one variant relationship the registry does not put in a name: ISO 15924
+  // lists Phli "Inscriptional Pahlavi" and Phlp "Psalter Pahlavi" (both RTL per
+  // CLDR) beside Phlv "Book Pahlavi", with nothing tying them together
+  // mechanically. Reachable only by typing a tag by hand, never from a search.
+  parents.set("Phlv", "Phli");
+
+  return parents;
+})();
+
+// ISO 15924 reserves Qaaa through Qabx for private use. The registry only
+// lists the two endpoints, so we range check instead of looking them up.
+function isPrivateUseScriptCode(titleCaseCode: string): boolean {
+  return (
+    /^Qa[ab][a-z]$/.test(titleCaseCode) &&
+    titleCaseCode >= "Qaaa" &&
+    titleCaseCode <= "Qabx"
+  );
+}
+
+// Script codes are conventionally title case (e.g. "Arab"), but tags that a
+// user typed by hand may not be.
+function toTitleCase(scriptCode: string): string {
+  return scriptCode.charAt(0).toUpperCase() + scriptCode.slice(1).toLowerCase();
+}
+
+// Determines a script's reading direction, or undefined if we cannot know it.
+//
+// Returning undefined rather than false matters because "we know this script
+// is left-to-right" and "we have no idea" call for different handling: a
+// consumer storing a writing system's direction can leave an existing setting
+// (or a user's own choice) alone instead of silently overwriting it with a
+// guess. We report undefined for placeholder script codes, private use codes,
+// and anything that isn't a real ISO 15924 script.
+//
+// Intl remains the authority for the actual left/right answer. It reports
+// "ltr" for every script it has no real data on, which keeps a useful answer
+// for the obscure tail (Tengwar, Mayan hieroglyphs, Braille and so on) at the
+// cost of trusting a default we cannot verify. Because of that fallback, the
+// answer for a very new script can differ between ICU builds; only outright
+// gaps are pinned above. Egyptian demotic (Egyd) and hieratic (Egyh) are the
+// known weak spots: both were normally written right to left, but they are
+// unencoded and no machine-readable source states a direction, so rather than
+// assert one we let them fall through and report left-to-right.
+//
 // Intl.Locale takes in a bcp47 tag, but here we are giving it
 // the tag und-{insert script code}, where the und means no
 // specified language, so that the rtl attribute will be based
@@ -71,15 +186,47 @@ export interface IOrthography {
 // .maximize will return the Arabic script for uz-AF. We always want the
 // isRtl setting to match its IScript in every case, which can accomplish
 // with und-{script}.
-export function isRTLScript(scriptCode: string): boolean {
+export function isRTLScript(scriptCode: string): boolean | undefined {
+  if (!scriptCode) {
+    return undefined;
+  }
+  const code = toTitleCase(scriptCode);
+
+  if (
+    SCRIPT_CODES_WITH_NO_DIRECTION.has(code) ||
+    isPrivateUseScriptCode(code) ||
+    // A well formed but unregistered code such as "Xyzw" is not a script we
+    // know anything about, even though Intl will confidently answer "ltr".
+    !ISO_15924_CODES.has(code)
+  ) {
+    return undefined;
+  }
+
+  // A variant code carries the direction of the script it is a variant of.
+  const effectiveCode = SCRIPT_CODE_VARIANT_PARENTS.get(code) ?? code;
+
+  // No registry entry currently pairs a real script with a placeholder parent,
+  // but if one ever appears the variant must inherit "no direction" rather than
+  // fall through to the Intl default below.
+  if (SCRIPT_CODES_WITH_NO_DIRECTION.has(effectiveCode)) {
+    return undefined;
+  }
+
+  if (RTL_SCRIPTS_UNKNOWN_TO_OLDER_ICU.has(effectiveCode)) {
+    return true;
+  }
+
   try {
-    const locale = new Intl.Locale(`und-${scriptCode}`);
+    const locale = new Intl.Locale(`und-${effectiveCode}`);
     // getTextInfo is the standardized property; textInfo is the older name
     const info = locale.getTextInfo?.() ?? (locale as any).textInfo;
-    return info?.direction === "rtl";
+    if (info?.direction !== "rtl" && info?.direction !== "ltr") {
+      return undefined;
+    }
+    return info.direction === "rtl";
   } catch {
-    // An unrecognized/malformed script code makes Intl.Locale throw. Such a
-    // script has no known RTL direction, so treat it as not RTL.
-    return false;
+    // A malformed script code makes Intl.Locale throw, leaving us with no
+    // direction information for it.
+    return undefined;
   }
 }

components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts

@@ -316,8 +316,15 @@ export function useLanguageChooserViewModel(
 // Returns a copy of the script with its reading direction (isRtl) populated,
 // so consumers receive the direction as part of the selected orthography.
 // Mirrors the behavior of the React useLanguageChooser hook.
+// When the direction is unknown (see isRTLScript) we leave isRtl off entirely
+// rather than claiming left-to-right.
 function scriptWithReadingDirection(script: IScript): IScript {
-  return { ...script, isRtl: isRTLScript(script.code) };
+  const isRtl = isRTLScript(script.code);
+  if (isRtl === undefined) {
+    const { isRtl: _unused, ...scriptWithoutDirection } = script;
+    return scriptWithoutDirection;
+  }
+  return { ...script, isRtl };
 }
 
 function hasValidDisplayName(selection: IOrthography) {

components/language-chooser/common/language-chooser-controller/test/language-chooser.spec.ts

@@ -382,6 +382,22 @@ describe("selected script", () => {
 
     expect(test.viewModel.selectedScript.value?.isRtl).toBe(true);
   });
+
+  it("should leave isRtl unset for a script with no reading direction", () => {
+    // Zxxx means "unwritten", which is how sign languages are tagged. There is
+    // no reading direction to report, so we must not claim left-to-right.
+    const signLanguage: ILanguage = {
+      ...WaataLanguage,
+      scripts: [{ code: "Zxxx", name: "Code for unwritten documents" }],
+    };
+    const test = new TestHelper({ initialLanguages: [signLanguage] });
+
+    test.viewModel.listedLanguages.value[0].isSelected.requestUpdate(true);
+
+    expect(test.viewModel.selectedScript.value?.code).toBe("Zxxx");
+    expect(test.viewModel.selectedScript.value?.isRtl).toBeUndefined();
+    expect(test.viewModel.selectedScript.value).not.toHaveProperty("isRtl");
+  });
 });
 
 describe("creating unlisted language", () => {
@@ -717,10 +733,31 @@ describe("customize language modal", () => {
       },
     });
 
+    // "abc" is not a registered ISO 15924 script, so its reading direction is
+    // unknown and isRtl is left unset. toEqual ignores properties whose value
+    // is undefined, so assert the absence of the key explicitly as well.
     expect(t.viewModel.selectedScript.value).toEqual({
       code: "abc",
       name: "ABC Script",
-      isRtl: false,
+    });
+    expect(t.viewModel.selectedScript.value).not.toHaveProperty("isRtl");
+  });
+
+  it("sets script with reading direction on submit", () => {
+    const t = new TestHelper({ initialLanguages: [NorthernUzbekLanguage] });
+    t.viewModel.listedLanguages.value[0].isSelected.requestUpdate(true);
+
+    t.viewModel.submitCustomizeLanguageModal({
+      script: {
+        code: "Arab",
+        name: "Arabic",
+      },
+    });
+
+    expect(t.viewModel.selectedScript.value).toEqual({
+      code: "Arab",
+      name: "Arabic",
+      isRtl: true,
     });
   });