Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Attribute fallback i18n strings with lang attribute (#7323)
Browse files Browse the repository at this point in the history
* add lang attribute to fallback translations

Signed-off-by: Kerry Archibald <kerrya@element.io>

* readability improvement

Signed-off-by: Kerry Archibald <kerrya@element.io>

* split _t and _tDom

Signed-off-by: Kerry <kerry@Kerrys-MBP.fritz.box>

* use tDom in HomePage

Signed-off-by: Kerry Archibald <kerrya@element.io>

* lint

Signed-off-by: Kerry Archibald <kerrya@element.io>

* bump matrix-web-i18n

Signed-off-by: Kerry Archibald <kerrya@element.io>
  • Loading branch information
Kerry authored Jan 5, 2022
1 parent ea7ac45 commit 7f13a1b
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 132 deletions.
12 changes: 11 additions & 1 deletion __mocks__/browser-request.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
const en = require("../src/i18n/strings/en_EN");
const de = require("../src/i18n/strings/de_DE");
const lv = {
"Save": "Saglabāt",
};

// Mock the browser-request for the languageHandler tests to return
// Fake languages.json containing references to en_EN and de_DE
// Fake languages.json containing references to en_EN, de_DE and lv
// en_EN.json
// de_DE.json
// lv.json - mock version with few translations, used to test fallback translation
module.exports = jest.fn((opts, cb) => {
const url = opts.url || opts.uri;
if (url && url.endsWith("languages.json")) {
Expand All @@ -17,11 +21,17 @@ module.exports = jest.fn((opts, cb) => {
"fileName": "de_DE.json",
"label": "German",
},
"lv": {
"fileName": "lv.json",
"label": "Latvian"
}
}));
} else if (url && url.endsWith("en_EN.json")) {
cb(undefined, {status: 200}, JSON.stringify(en));
} else if (url && url.endsWith("de_DE.json")) {
cb(undefined, {status: 200}, JSON.stringify(de));
} else if (url && url.endsWith("lv.json")) {
cb(undefined, {status: 200}, JSON.stringify(lv));
} else {
cb(true, {status: 404}, "");
}
Expand Down
20 changes: 10 additions & 10 deletions src/components/structures/HomePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { useContext, useState } from "react";

import AutoHideScrollbar from './AutoHideScrollbar';
import { getHomePageUrl } from "../../utils/pages";
import { _t } from "../../languageHandler";
import { _tDom } from "../../languageHandler";
import SdkConfig from "../../SdkConfig";
import * as sdk from "../../index";
import dis from "../../dispatcher/dispatcher";
Expand Down Expand Up @@ -72,8 +72,8 @@ const UserWelcomeTop = () => {
return <div>
<MiniAvatarUploader
hasAvatar={!!ownProfile.avatarUrl}
hasAvatarLabel={_t("Great, that'll help people know it's you")}
noAvatarLabel={_t("Add a photo so people know it's you.")}
hasAvatarLabel={_tDom("Great, that'll help people know it's you")}
noAvatarLabel={_tDom("Add a photo so people know it's you.")}
setAvatarUrl={url => cli.setAvatarUrl(url)}
>
<BaseAvatar
Expand All @@ -86,8 +86,8 @@ const UserWelcomeTop = () => {
/>
</MiniAvatarUploader>

<h1>{ _t("Welcome %(name)s", { name: ownProfile.displayName }) }</h1>
<h4>{ _t("Now, let's help you get started") }</h4>
<h1>{ _tDom("Welcome %(name)s", { name: ownProfile.displayName }) }</h1>
<h4>{ _tDom("Now, let's help you get started") }</h4>
</div>;
};

Expand All @@ -113,8 +113,8 @@ const HomePage: React.FC<IProps> = ({ justRegistered = false }) => {

introSection = <React.Fragment>
<img src={logoUrl} alt={config.brand} />
<h1>{ _t("Welcome to %(appName)s", { appName: config.brand }) }</h1>
<h4>{ _t("Own your conversations.") }</h4>
<h1>{ _tDom("Welcome to %(appName)s", { appName: config.brand }) }</h1>
<h4>{ _tDom("Own your conversations.") }</h4>
</React.Fragment>;
}

Expand All @@ -123,13 +123,13 @@ const HomePage: React.FC<IProps> = ({ justRegistered = false }) => {
{ introSection }
<div className="mx_HomePage_default_buttons">
<AccessibleButton onClick={onClickSendDm} className="mx_HomePage_button_sendDm">
{ _t("Send a Direct Message") }
{ _tDom("Send a Direct Message") }
</AccessibleButton>
<AccessibleButton onClick={onClickExplore} className="mx_HomePage_button_explore">
{ _t("Explore Public Rooms") }
{ _tDom("Explore Public Rooms") }
</AccessibleButton>
<AccessibleButton onClick={onClickNewRoom} className="mx_HomePage_button_createGroup">
{ _t("Create a Group Chat") }
{ _tDom("Create a Group Chat") }
</AccessibleButton>
</div>
</div>
Expand Down
5 changes: 3 additions & 2 deletions src/components/views/elements/MiniAvatarUploader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ import MatrixClientContext from "../../../contexts/MatrixClientContext";
import { useTimeout } from "../../../hooks/useTimeout";
import Analytics from "../../../Analytics";
import CountlyAnalytics from '../../../CountlyAnalytics';
import { TranslatedString } from '../../../languageHandler';
import RoomContext from "../../../contexts/RoomContext";

export const AVATAR_SIZE = 52;

interface IProps {
hasAvatar: boolean;
noAvatarLabel?: string;
hasAvatarLabel?: string;
noAvatarLabel?: TranslatedString;
hasAvatarLabel?: TranslatedString;
setAvatarUrl(url: string): Promise<unknown>;
}

Expand Down
9 changes: 0 additions & 9 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -2976,15 +2976,6 @@
"Community %(groupId)s not found": "Community %(groupId)s not found",
"This homeserver does not support communities": "This homeserver does not support communities",
"Failed to load %(groupId)s": "Failed to load %(groupId)s",
"Great, that'll help people know it's you": "Great, that'll help people know it's you",
"Add a photo so people know it's you.": "Add a photo so people know it's you.",
"Welcome %(name)s": "Welcome %(name)s",
"Now, let's help you get started": "Now, let's help you get started",
"Welcome to %(appName)s": "Welcome to %(appName)s",
"Own your conversations.": "Own your conversations.",
"Send a Direct Message": "Send a Direct Message",
"Explore Public Rooms": "Explore Public Rooms",
"Create a Group Chat": "Create a Group Chat",
"Upgrade to %(hostSignupBrand)s": "Upgrade to %(hostSignupBrand)s",
"Open dial pad": "Open dial pad",
"Public community": "Public community",
Expand Down
99 changes: 68 additions & 31 deletions src/languageHandler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ const ANNOTATE_STRINGS = false;

// We use english strings as keys, some of which contain full stops
counterpart.setSeparator('|');
// Fall back to English
counterpart.setFallbackLocale('en');

// see `translateWithFallback` for an explanation of fallback handling
const FALLBACK_LOCALE = 'en';

interface ITranslatableError extends Error {
translatedMessage: string;
Expand Down Expand Up @@ -72,20 +73,40 @@ export function _td(s: string): string { // eslint-disable-line @typescript-esli
return s;
}

/**
* to improve screen reader experience translations that are not in the main page language
* eg a translation that fell back to english from another language
* should be wrapped with an appropriate `lang='en'` attribute
* counterpart's `translate` doesn't expose a way to determine if the resulting translation
* is in the target locale or a fallback locale
* for this reason, we do not set a fallback via `counterpart.setFallbackLocale`
* and fallback 'manually' so we can mark fallback strings appropriately
* */
const translateWithFallback = (text: string, options?: object): { translated?: string, isFallback?: boolean } => {
const translated = counterpart.translate(text, options);
if (/^missing translation:/.test(translated)) {
const fallbackTranslated = counterpart.translate(text, { ...options, fallbackLocale: FALLBACK_LOCALE });
return { translated: fallbackTranslated, isFallback: true };
}
return { translated };
};

// Wrapper for counterpart's translation function so that it handles nulls and undefineds properly
// Takes the same arguments as counterpart.translate()
function safeCounterpartTranslate(text: string, options?: object) {
function safeCounterpartTranslate(text: string, variables?: object) {
// Don't do substitutions in counterpart. We handle it ourselves so we can replace with React components
// However, still pass the variables to counterpart so that it can choose the correct plural if count is given
// It is enough to pass the count variable, but in the future counterpart might make use of other information too
const options = { ...variables, interpolate: false };

// Horrible hack to avoid https://github.com/vector-im/element-web/issues/4191
// The interpolation library that counterpart uses does not support undefined/null
// values and instead will throw an error. This is a problem since everywhere else
// in JS land passing undefined/null will simply stringify instead, and when converting
// valid ES6 template strings to i18n strings it's extremely easy to pass undefined/null
// if there are no existing null guards. To avoid this making the app completely inoperable,
// we'll check all the values for undefined/null and stringify them here.
let count;

if (options && typeof options === 'object') {
count = options['count'];
Object.keys(options).forEach((k) => {
if (options[k] === undefined) {
logger.warn("safeCounterpartTranslate called with undefined interpolation name: " + k);
Expand All @@ -97,13 +118,7 @@ function safeCounterpartTranslate(text: string, options?: object) {
}
});
}
let translated = counterpart.translate(text, options);
if (translated === undefined && count !== undefined) {
// counterpart does not do fallback if no pluralisation exists
// in the preferred language, so do it here
translated = counterpart.translate(text, Object.assign({}, options, { locale: 'en' }));
}
return translated;
return translateWithFallback(text, options);
}

type SubstitutionValue = number | string | React.ReactNode | ((sub: string) => React.ReactNode);
Expand All @@ -117,6 +132,20 @@ export type Tags = Record<string, SubstitutionValue>;

export type TranslatedString = string | React.ReactNode;

// For development/testing purposes it is useful to also output the original string
// Don't do that for release versions
const annotateStrings = (result: TranslatedString, translationKey: string): TranslatedString => {
if (!ANNOTATE_STRINGS) {
return result;
}

if (typeof result === 'string') {
return `@@${translationKey}##${result}@@`;
} else {
return <span className='translated-string' data-orig-string={translationKey}>{ result }</span>;
}
};

/*
* Translates text and optionally also replaces XML-ish elements in the text with e.g. React components
* @param {string} text The untranslated text, e.g "click <a>here</a> now to %(foo)s".
Expand All @@ -134,31 +163,39 @@ export type TranslatedString = string | React.ReactNode;
* @return a React <span> component if any non-strings were used in substitutions, otherwise a string
*/
// eslint-next-line @typescript-eslint/naming-convention
// eslint-nexline @typescript-eslint/naming-convention
export function _t(text: string, variables?: IVariables): string;
export function _t(text: string, variables: IVariables, tags: Tags): React.ReactNode;
export function _t(text: string, variables?: IVariables, tags?: Tags): TranslatedString {
// Don't do substitutions in counterpart. We handle it ourselves so we can replace with React components
// However, still pass the variables to counterpart so that it can choose the correct plural if count is given
// It is enough to pass the count variable, but in the future counterpart might make use of other information too
const args = Object.assign({ interpolate: false }, variables);

// The translation returns text so there's no XSS vector here (no unsafe HTML, no code execution)
const translated = safeCounterpartTranslate(text, args);
const { translated } = safeCounterpartTranslate(text, variables);

const substituted = substitute(translated, variables, tags);

// For development/testing purposes it is useful to also output the original string
// Don't do that for release versions
if (ANNOTATE_STRINGS) {
if (typeof substituted === 'string') {
return `@@${text}##${substituted}@@`;
} else {
return <span className='translated-string' data-orig-string={text}>{ substituted }</span>;
}
} else {
return substituted;
}
return annotateStrings(substituted, text);
}

/*
* Wraps normal _t function and adds atttribution for translations that used a fallback locale
* Wraps translations that fell back from active locale to fallback locale with a `<span lang=<fallback locale>>`
* @param {string} text The untranslated text, e.g "click <a>here</a> now to %(foo)s".
* @param {object} variables Variable substitutions, e.g { foo: 'bar' }
* @param {object} tags Tag substitutions e.g. { 'a': (sub) => <a>{sub}</a> }
*
* @return a React <span> component if any non-strings were used in substitutions
* or translation used a fallback locale, otherwise a string
*/
// eslint-next-line @typescript-eslint/naming-convention
export function _tDom(text: string, variables?: IVariables): TranslatedString;
export function _tDom(text: string, variables: IVariables, tags: Tags): React.ReactNode;
export function _tDom(text: string, variables?: IVariables, tags?: Tags): TranslatedString {
// The translation returns text so there's no XSS vector here (no unsafe HTML, no code execution)
const { translated, isFallback } = safeCounterpartTranslate(text, variables);
const substituted = substitute(translated, variables, tags);

// wrap en fallback translation with lang attribute for screen readers
const result = isFallback ? <span lang='en'>{ substituted }</span> : substituted;

return annotateStrings(result, text);
}

/**
Expand Down
79 changes: 0 additions & 79 deletions test/i18n-test/languageHandler-test.js

This file was deleted.

Loading

1 comment on commit 7f13a1b

@DoM1niC
Copy link

@DoM1niC DoM1niC commented on 7f13a1b Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grafik
Counterpart is broken :(

Please sign in to comment.