Consider dropping old-attribute cleanup

[zbraniecki]

In FluentDOM we currently attempt to clean up the attributes from the element we're translating.

That has two consequences:

1. It requires additional work to be performed on each element before we can apply simple translations

One of the reasons we're able to perform quite complex logic like DOM Overlays without much performance cost is that we actually perform that on a small subset of elements.

An example of Preferences where we handle several thousands localizable elements at startup, but really only 10-20 of them have DOM Overlays is a good example.

Having to collect attributes, check them against a whitelist on each and every one of them seems to be a potentially high cost for when most nodes just set the textContent.

I'll test the perf impact and report back in this thread.

2. It may add a hard to discover unexpected bugs

Imagine an element which the developer populated with an attribute that happens to be on our list, but he didn't intend for it to be localizable:

<key data-l10n-id="foo" keyCode="VK_LEFT"/>

foo =
    .key = F

Now, this will remove keyCode from the <key> without developer expecting it probably. We'd have to ask them to consult and remember about the list of attributes we keep to avoid that. It may also be rather hard to debug.

But it gets even worse. Imagine that we in the future alter the list of we have - adding a new attribute will suddenly remove it from all places where it has been used, in a non-localizable manner, before.

There's also no escape path. We do not offer a developer a way to say "please, do not localize attribute X".

Lastly, it is inconsistent with what we do for text content. If a message does not provide a value, we will not attempt to "clean it up in case there's a value in another language". Which is pretty lucky, because otherwise we'd remove the whole content of <window> element in XUL, but it is inconsistent with an attempt to clean it up for the same reason in attributes.

I'm also not very convinced by the "leaking between translations" argument in total. We do not allow for localizations to differ in which attributes they localize and we don't seem to have a use case where we'd like to allow one locale to translate placeholder and another not to.

In which case the list of current attributes on the message is a list of localizable attributes of the current element and we can do for attributes what we do for values.

[Pike]

You're talking about

fluent.js/fluent-dom/src/overlay.js

Lines 77 to 83 in 8ed4e4b

	// Remove localizable attributes which may have been set by a previous
	// translation.
	for (const attr of Array.from(targetElement.attributes)) {
	if (isAttrNameLocalizable(attr.name, targetElement, explicitlyAllowed)) {
	targetElement.removeAttribute(attr.name);
	}
	}

?

Yes, I think that can go away, at least in a gecko context where we know that we ran compare-locales.

[stasm]

If there's an easy way to do this in Gecko-specifc build of fluent-dom, then it might be ok to do it. Otherwise, I'm against changing the guts of fluent-dom because of compare-locales.

I also don't agree with the rationale.

Having to collect attributes, check them against a whitelist on each and every one of them seems to be a potentially high cost for when most nodes just set the textContent.

I would like to see any numbers supporting this claim. I highly doubt that iterating over a mostly-empty array has any noticeable cost.

Imagine an element which the developer populated with an attribute that happens to be on our list, but he didn't intend for it to be localizable:

This would be better solved by somehow disallowing attr localization via data-l10n-attrs. We already do exactly this in fluent-react.

Lastly, it is inconsistent with what we do for text content. If a message does not provide a value, we will not attempt to "clean it up in case there's a value in another language". Which is pretty lucky, because otherwise we'd remove the whole content of element in XUL, but it is inconsistent with an attempt to clean it up for the same reason in attributes.

I think this is different bug. We should provide a way for developers to protect the contents of the element, maybe via data-l10n-novalue or something like that. Note that I think we should do this even though we're already protecting against this scenario in compare-locales.

I'm also not very convinced by the "leaking between translations" argument in total. We do not allow for localizations to differ in which attributes they localize and we don't seem to have a use case where we'd like to allow one locale to translate placeholder and another not to.

We don't allow it by means of compare-locales checks. fluent-dom being a runtime localization solution, it makes sense to provide guards in the runtime rather than rely on static analysis tools on buildtime.