✨ [Story localization] Localize system layer async #37870
Conversation
Co-authored-by: Gabriel Majoulet <gmajoulet@google.com>
…nto stringjson_inlined
…trings_controlled
…trings_controlled
…/amphtml into localizeAsyncArias
| .registerLocalizedStringBundle('zh-cn', LocalizedStringsZhCn) | ||
| .registerLocalizedStringBundle('zh-TW', LocalizedStringsZhTw) | ||
| .registerLocalizedStringBundle('en-xa', enXaPseudoLocaleBundle); | ||
| this.localizationService_.registerLocalizedStringBundles({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const localizeSystemLayer = (systemLayer, parentElement) => { | ||
| const localizationService = getLocalizationService(parentElement); | ||
| systemLayer.querySelectorAll('[data-aria-label]').forEach((el) => { | ||
| localizationService.localizeEl( | ||
| el, | ||
| el.getAttribute('data-aria-label'), | ||
| 'aria-label' | ||
| ); | ||
| el.removeAttribute('data-aria-label'); | ||
| }); | ||
| localizationService.localizeEl( | ||
| systemLayer.querySelector('.i-amphtml-story-has-new-page-text'), | ||
| LocalizedStringId_Enum.AMP_STORY_HAS_NEW_PAGE_TEXT | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
We shouldn't have any LocalizedStringId_Enum key in here, this should just be a post processing function using the data provided by developers within templates.
Also, this should be a helper that can be re-used for all other templates in other files.
A few additional questions:
- How will you block rendering on strings that need to be rendered synchronously
- If you add a string asynchronously but it is visible to users, then it needs to run in a mutateElement context
There was a problem hiding this comment.
- Block rendering can happen like so:
localize(template).then(() => {document.appendChild(template)}) - Yes, in that scenario it will likely be a render-blocking localization, which currently happens in a mutate context so we just do that after the promise.
|
Also, can you please improve how you track this feature, and open an issue explaining the change + listing all the files you have to update? Being more transparent will help wit a lot of things, from understanding the scope of this PR and its place in the overall project, to tracking progress. |
|
@gmajoulet there is already an I2I on #37344 that contains that info. There are a few implementation details that are not there, so I added them in a comment. |
Make the system layer localize asynchronously. This means loading the aria-labels and the textContents async.
Why change the signature of
registerLocalizationStringBundles?Every time we add a new localization bundle we need to check if any usages are waiting for that string, so when we want to install the 20 languages bundled with
amp-story, we want to just make one call toregisterLocalizationStringBundles. For that reason the function needs to allow registering many languages at once, and at the end make a call to the observable.When is the observable going to fire? What's it going to fire?
Only when the strings arrive after the components request the strings, the observable will have handlers that use the strings presented (this is when fetching the JSON from the cache). If the strings are registered before that (eg: with an inline JSON), the observer will never be used and the string requests will be resolved immediately.
Drafted on: #37303
Requires fix: #37887
I2I: #37344