1746748 - Introduce multi-setAttributes and immediate mode

Status: NEW

(Core :: Internationalization: Localization, enhancement)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Based on the conversation in bug 1737951, this bug is about introduction of two API extensions intended for DOMLocalization:

1. Multi-setAttributes:

interface ElementL10nInfo {
  element: Element,
  id?: string;
  args?: Record<string, string | number>
}

document.l10n.setManyAttributes(ElementL10nInfo[]); // Name up for bikeshed

2. Immediate mode

document.l10n.setAttributes(elem, id, args, { immediate: true }); 
document.l10n.setManyAttributes(ElementL10nInfo[], { immediate: true }); 

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

WDYT?

Comment 2

[Eemeli Aro [:eemeli]]

Is there a reason to not overload the existing function?

document.l10n.setAttributes(elem, id, args, { immediate: true }); 
document.l10n.setAttributes(ElementL10nInfo[], { immediate: true });

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Is there a reason to not overload the existing function?

No, we can do it. I didn't suggest it because the signature feels a bit weird. If we were to shift it to:

document.l10n.setAttributes({elem, id, args}, { immediate: true});

so: ElementL10nInfo[] | ElemL10nInfo as the first element, that would be more consistent, but I don't want to shift all use cases around the codebase.

What do you prefer?

Comment 4

[Eemeli Aro [:eemeli]]

I would prefer not to breaking existing APIs unless there's a very good reason, and I don't think being arguably more consistent on this would be that.

To continue with the bikeshedding, another option here would be to keep the current setAttributes() exactly as-is, and add a second method like:

document.l10n.setAttributesImmediately(ElementL10nInfo[]); // not a great fan of the name, though

For the current setAttributes(), there is no real penalty from calling it multiple times. But for the immediate variant, getting a single list means that the observer needs to be toggled off & on only once. Having a separate method for it that requires array input might direct users towards good practices more easily.

Comment 5

[Erik Nordin [:nordzilla]]

Finally caught up on this conversation and the one in Bug 1737951.

In the spirit of the naming conventions like generateBundles() and generateBundlesSync(), how about:

document.l10n.setAttributesSync(ElementL10nInfo[]);

It seems to be a recurring theme to have contexts in which there is a desire to distinguish between the sync and async versions with different function calls; I wouldn't mind normalizing the naming convention that someAction() is, by default, async, and that there will often be a someActionSync() variant.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

The issue with that is that unfortunately the operation is not going to be sync, at least not yet.

The immediate mode is only shortcutting the step of coalescing pending elements localization into a single animation frame. Instead of waiting for it, the immediate mode kicks off l10n immediatelly, but that that is still async.

Localization class has a sync and async mode, but only one way "upgrade" path from sync to async.

This means that you can have a sync localization, or async localization and the sync localization can be updated to async (this happens when browser.xhtml is first loaded, the initial l10n is sync, then we upgrade and from firstPaint it's async).

Once the class is in async state, we can't revert it, so no sync calls can be executed, which means that all l10n calls are async.

We can revisit it (there are some interesting implications on cache handling for cases where a pending async I/O is followed by a sync I/O request for the same resource), but it'd be much deeper in the guts of fluent-fallback.

For this bug, I'd like to introduce a mode that bypasses the coalescing, and incentivizes engineers to batch updates to l10n DOM that trigger it.

Comment 7

[Erik Nordin [:nordzilla]]

I see. I misunderstood. But that does clear things up. Thank you for clarifying.