Skip to content

✨ [Story localization] Localize system layer async #37870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mszylkowski merged 47 commits into from
Mar 22, 2022
Merged

✨ [Story localization] Localize system layer async #37870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
6aabe91
Added tasts
mszylkowski Dec 21, 2021
dc1fa87
Undo
mszylkowski Dec 21, 2021
d707e5b
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Dec 28, 2021
0e3df75
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Dec 28, 2021
a3f7bd0
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 4, 2022
8c5119c
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 6, 2022
77f8435
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 10, 2022
16e712e
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 19, 2022
8088eb7
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 25, 2022
3858b2a
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 28, 2022
e12d2f6
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 31, 2022
2a29105
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Feb 1, 2022
a5b781f
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Feb 10, 2022
b8006ee
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Mar 8, 2022
a04f6d2
Added logic and tests
mszylkowski Mar 8, 2022
9c4f9b9
Updated tests with correct version
mszylkowski Mar 8, 2022
07e3ca5
Updated comment
mszylkowski Mar 9, 2022
879fa46
Catch error in JSON inlined
mszylkowski Mar 9, 2022
5dfb927
Merge branch 'stringjson_inlined' of github.com:mszylkowski/amphtml i…
mszylkowski Mar 9, 2022
bc9c7ed
Removed empty lines
mszylkowski Mar 9, 2022
e53661a
Changed attribute to amp-localization
mszylkowski Mar 9, 2022
55274f6
Added json fetching for testing
mszylkowski Mar 9, 2022
595c683
Remove .only
mszylkowski Mar 9, 2022
ed4ecaf
Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…
mszylkowski Mar 10, 2022
5b923de
Install localization strings if inside experiment
mszylkowski Mar 10, 2022
6f8c1ca
Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…
mszylkowski Mar 10, 2022
1d419c3
Added tests
mszylkowski Mar 10, 2022
72070cf
Moved test to inside describes
mszylkowski Mar 10, 2022
1361273
Remove unused function
mszylkowski Mar 10, 2022
5b49fff
Remove unused import
mszylkowski Mar 10, 2022
b293243
Reverted index.js
mszylkowski Mar 10, 2022
ec2c791
Added dep check
mszylkowski Mar 10, 2022
3a5e409
Made tests more explicit
mszylkowski Mar 14, 2022
b3b7b17
Catching unknown language
mszylkowski Mar 14, 2022
53eb702
Localize system layer + localization service changes
mszylkowski Mar 14, 2022
ac27177
Install all bundles at the same time
mszylkowski Mar 15, 2022
d6cea31
Fixed tests with installing all bundles
mszylkowski Mar 15, 2022
0900d28
Fixed comments from gmajoulet
mszylkowski Mar 15, 2022
59ebcb5
Remove unused fetchStub const, test with performance service
mszylkowski Mar 15, 2022
613800f
Merge branch 'TEST_fetchstrings_controlled' of github.com:mszylkowski…
mszylkowski Mar 15, 2022
4dc1328
Revert observable change
mszylkowski Mar 17, 2022
8129374
Use object.keys instead of entries
mszylkowski Mar 17, 2022
193502b
Fixed iteration
mszylkowski Mar 17, 2022
f617514
Merge branch 'main' of github.com:ampproject/amphtml into localizeAsy…
mszylkowski Mar 17, 2022
4315bae
Use localizeTemplate
mszylkowski Mar 18, 2022
e22b1a9
Updated tests
mszylkowski Mar 18, 2022
8488149
Remove .only
mszylkowski Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 25 additions & 24 deletions extensions/amp-story-auto-ads/0.1/story-ad-localization.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,29 +110,30 @@ export class StoryAdLocalization {
(s) => `[${s} one two]`
);

this.localizationService_
.registerLocalizedStringBundle('default', LocalizedStringsEn)
.registerLocalizedStringBundle('ar', LocalizedStringsAr)
.registerLocalizedStringBundle('de', LocalizedStringsDe)
.registerLocalizedStringBundle('en', LocalizedStringsEn)
.registerLocalizedStringBundle('en-GB', LocalizedStringsEnGb)
.registerLocalizedStringBundle('es', LocalizedStringsEs)
.registerLocalizedStringBundle('es-419', LocalizedStringsEs419)
.registerLocalizedStringBundle('fr', LocalizedStringsFr)
.registerLocalizedStringBundle('hi', LocalizedStringsHi)
.registerLocalizedStringBundle('id', LocalizedStringsId)
.registerLocalizedStringBundle('it', LocalizedStringsIt)
.registerLocalizedStringBundle('ja', LocalizedStringsJa)
.registerLocalizedStringBundle('ko', LocalizedStringsKo)
.registerLocalizedStringBundle('nl', LocalizedStringsNl)
.registerLocalizedStringBundle('no', LocalizedStringsNo)
.registerLocalizedStringBundle('pt-PT', LocalizedStringsPtPt)
.registerLocalizedStringBundle('pt-BR', LocalizedStringsPtBr)
.registerLocalizedStringBundle('ru', LocalizedStringsRu)
.registerLocalizedStringBundle('tr', LocalizedStringsTr)
.registerLocalizedStringBundle('vi', LocalizedStringsVi)
.registerLocalizedStringBundle('zh-cn', LocalizedStringsZhCn)
.registerLocalizedStringBundle('zh-TW', LocalizedStringsZhTw)
.registerLocalizedStringBundle('en-xa', enXaPseudoLocaleBundle);
this.localizationService_.registerLocalizedStringBundles({
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Mar 17, 2022

Choose a reason for hiding this comment

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

Are we lazy loading auto-ads strings as a follow up? Where is this tracked?
How many kbs are removed from the critical render path by doing so?

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Mar 18, 2022

Choose a reason for hiding this comment

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

This change is part of the change in signature of registerLocalizationStringBundles to receive a map of languageCode: bundle. We can make the auto-ads async at some point but it's not the goal of this effort now (can be a stretch goal); we could add the strings to the amp-story JSONs or a new set of JSONs, because this architecture allows for both, though it will require more work on the infra side. The auto-ads strings are not in the critical render path of the story so we can analyze it later.

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Mar 18, 2022

Choose a reason for hiding this comment

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

The answer to this is in the FAQ section of the I2I as well. I added it in the description of the PR so it's easier to follow.

'default': LocalizedStringsEn,
'ar': LocalizedStringsAr,
'de': LocalizedStringsDe,
'en': LocalizedStringsEn,
'en-GB': LocalizedStringsEnGb,
'es': LocalizedStringsEs,
'es-419': LocalizedStringsEs419,
'fr': LocalizedStringsFr,
'hi': LocalizedStringsHi,
'id': LocalizedStringsId,
'it': LocalizedStringsIt,
'ja': LocalizedStringsJa,
'ko': LocalizedStringsKo,
'nl': LocalizedStringsNl,
'no': LocalizedStringsNo,
'pt-PT': LocalizedStringsPtPt,
'pt-BR': LocalizedStringsPtBr,
'ru': LocalizedStringsRu,
'tr': LocalizedStringsTr,
'vi': LocalizedStringsVi,
'zh-cn': LocalizedStringsZhCn,
'zh-TW': LocalizedStringsZhTw,
'en-xa': enXaPseudoLocaleBundle,
});
}
}
32 changes: 32 additions & 0 deletions extensions/amp-story/1.0/amp-story-localization-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import {getWin} from '#core/window';

import {Services} from '#service';
import {LocalizationService} from '#service/localization';

Expand Down Expand Up @@ -31,3 +33,33 @@ export function getLocalizationService(element) {
export function localize(context, key) {
return getLocalizationService(context).getLocalizedString(key);
}

/**
* Adds the localized strings onto the template as aria labels or text contents.
* Uses `i-amphtml-i18n-text-content` or `i-amphtml-i18n-aria-label` attributes.
* @param {!Element} template
* @param {!Element} context
* @return {!Promise}
*/
export function localizeTemplate(template, context) {
const localizationService = getLocalizationService(context);
const vsync = Services.vsyncFor(getWin(context));
const promises = [];
template.querySelectorAll('[i-amphtml-i18n-aria-label]').forEach((el) => {
promises.push(
localizationService
.getLocalizedStringAsync(el.getAttribute('i-amphtml-i18n-aria-label'))
.then((str) => el.setAttribute('aria-label', str))
);
el.removeAttribute('i-amphtml-i18n-aria-label');
});
template.querySelectorAll('[i-amphtml-i18n-text-content]').forEach((el) => {
promises.push(
localizationService
.getLocalizedStringAsync(el.getAttribute('i-amphtml-i18n-text-content'))
.then((str) => vsync.mutatePromise(() => (el.textContent = str)))
);
el.removeAttribute('i-amphtml-i18n-text-content');
});
return Promise.all(promises);
}
54 changes: 24 additions & 30 deletions extensions/amp-story/1.0/amp-story-system-layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {LocalizedStringId_Enum} from '#service/localization/strings';

import {dev} from '#utils/log';

import {localize} from './amp-story-localization-service';
import {localizeTemplate} from './amp-story-localization-service';
import {
Action,
StateProperty,
Expand Down Expand Up @@ -97,80 +97,72 @@ const renderSystemLayerElement = (element, children) => (
<div class="i-amphtml-story-has-new-page-notification-container">
<div class="i-amphtml-story-has-new-page-text-wrapper">
<span class="i-amphtml-story-has-new-page-circle-icon" />
<div class="i-amphtml-story-has-new-page-text">
{localize(
element,
<div
class="i-amphtml-story-has-new-page-text"
i-amphtml-i18n-text-content={
LocalizedStringId_Enum.AMP_STORY_HAS_NEW_PAGE_TEXT
)}
</div>
}
></div>
</div>
</div>
<div class="i-amphtml-story-system-layer-buttons">
<div
role="button"
class={INFO_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_INFO_BUTTON_LABEL
)}
}
/>
<div class="i-amphtml-story-sound-display">
<button
class={UNMUTE_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_AUDIO_UNMUTE_BUTTON_LABEL
)}
}
/>
<button
class={MUTE_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_AUDIO_MUTE_BUTTON_LABEL
)}
}
/>
</div>
<div class="i-amphtml-paused-display">
<button
class={PAUSE_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_PAUSE_BUTTON_LABEL
)}
}
/>
<button
class={PLAY_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_PLAY_BUTTON_LABEL
)}
}
/>
</div>
<button
class={
SKIP_TO_NEXT_CLASS +
' i-amphtml-story-ui-hide-button i-amphtml-story-button'
}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_SKIP_TO_NEXT_BUTTON_LABEL
)}
}
/>
<button
class={SHARE_CLASS + ' i-amphtml-story-button'}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_SHARE_BUTTON_LABEL
)}
}
/>
<button
class={
CLOSE_CLASS + ' i-amphtml-story-ui-hide-button i-amphtml-story-button'
}
aria-label={localize(
element,
i-amphtml-i18n-aria-label={
LocalizedStringId_Enum.AMP_STORY_CLOSE_BUTTON_LABEL
)}
}
/>
</div>
<div class="i-amphtml-story-system-layer-buttons-start-position" />
Expand Down Expand Up @@ -279,6 +271,8 @@ export class SystemLayer {
this.parentEl_,
this.progressBar_.build(initialPageId)
);
localizeTemplate(this.systemLayerEl_, this.parentEl_);

// Make the share button link to the current document to make sure
// embedded STAMPs always have a back-link to themselves, and to make
// gestures like right-clicks work.
Expand Down
66 changes: 34 additions & 32 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2464,33 +2464,34 @@ export class AmpStory extends AMP.BaseElement {
'story-remote-localization'
);
} else {
getLocalizationService(this.element)
.registerLocalizedStringBundle('default', LocalizedStringsEn)
.registerLocalizedStringBundle('ar', LocalizedStringsAr)
.registerLocalizedStringBundle('de', LocalizedStringsDe)
.registerLocalizedStringBundle('en', LocalizedStringsEn)
.registerLocalizedStringBundle('en-GB', LocalizedStringsEnGb)
.registerLocalizedStringBundle('es', LocalizedStringsEs)
.registerLocalizedStringBundle('es-419', LocalizedStringsEs419)
.registerLocalizedStringBundle('fr', LocalizedStringsFr)
.registerLocalizedStringBundle('hi', LocalizedStringsHi)
.registerLocalizedStringBundle('id', LocalizedStringsId)
.registerLocalizedStringBundle('it', LocalizedStringsIt)
.registerLocalizedStringBundle('ja', LocalizedStringsJa)
.registerLocalizedStringBundle('ko', LocalizedStringsKo)
.registerLocalizedStringBundle('nl', LocalizedStringsNl)
.registerLocalizedStringBundle('no', LocalizedStringsNo)
.registerLocalizedStringBundle('pt-PT', LocalizedStringsPtPt)
.registerLocalizedStringBundle('pt-BR', LocalizedStringsPtBr)
.registerLocalizedStringBundle('ru', LocalizedStringsRu)
.registerLocalizedStringBundle('tr', LocalizedStringsTr)
.registerLocalizedStringBundle('vi', LocalizedStringsVi)
.registerLocalizedStringBundle('zh-CN', LocalizedStringsZhCn)
.registerLocalizedStringBundle('zh-TW', LocalizedStringsZhTw)
.registerLocalizedStringBundle(
'en-xa',
createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`)
);
getLocalizationService(this.element).registerLocalizedStringBundles({
'default': LocalizedStringsEn,
'ar': LocalizedStringsAr,
'de': LocalizedStringsDe,
'en': LocalizedStringsEn,
'en-GB': LocalizedStringsEnGb,
'es': LocalizedStringsEs,
'es-419': LocalizedStringsEs419,
'fr': LocalizedStringsFr,
'hi': LocalizedStringsHi,
'id': LocalizedStringsId,
'it': LocalizedStringsIt,
'ja': LocalizedStringsJa,
'ko': LocalizedStringsKo,
'nl': LocalizedStringsNl,
'no': LocalizedStringsNo,
'pt-PT': LocalizedStringsPtPt,
'pt-BR': LocalizedStringsPtBr,
'ru': LocalizedStringsRu,
'tr': LocalizedStringsTr,
'vi': LocalizedStringsVi,
'zh-CN': LocalizedStringsZhCn,
'zh-TW': LocalizedStringsZhTw,
'en-xa': createPseudoLocale(
LocalizedStringsEn,
(s) => `[${s} one two]`
),
});
}
}

Expand All @@ -2516,10 +2517,9 @@ export class AmpStory extends AMP.BaseElement {
return false;
}
const localizationService = getLocalizationService(this.element);
localizationService.registerLocalizedStringBundle(
languageCode,
stringsOrNull
);
localizationService.registerLocalizedStringBundles({
[languageCode]: stringsOrNull,
});
return true;
}

Expand All @@ -2541,7 +2541,9 @@ export class AmpStory extends AMP.BaseElement {
.fetchJson(localizationUrl, {prerenderSafe: true})
.then((res) => res.json())
.then((json) =>
localizationService.registerLocalizedStringBundle(languageCode, json)
localizationService.registerLocalizedStringBundles({
[languageCode]: json,
})
)
.catch((err) => {
devError(TAG, err, 'Bundle not found for language ' + languageCode);
Expand Down
Loading