fix(compiler): support i18n interpolated only attribute bindings

[petebacondarwin]

While fully dynamic bound properties (and attributes) cannot be marked for localization, properties that only contain interpolation can.

This commit ensure that attribute bindings that only contain interpolation can also be marked for localization.

Closes #43260

[AndrewKushnir]

Looks great, thanks Pete 👍

Just left a couple comments + it might be great to add an acceptance test as well (packages/core/test/acceptance/i18n_spec.ts).

[petebacondarwin]

@AndrewKushnir - I added the tests and comments - PTAL - and can you run a presubmit?

[AndrewKushnir]

@petebacondarwin FYI I've left a couple comments and started a presubmit (I'd also need to verify extraction mechanisms separately).

[AndrewKushnir]

@petebacondarwin FYI presubmits went well and extra testing was also successful, so this PR is in a good-to-go state from g3 perspective. Thank you.

[AndrewKushnir]

This PR was merged into the repository by commit bba0a87.

[AndrewKushnir]

Reopened due to the rollback, see #43882.
We'll need to run a global presubmit for this change before we try to land it next time.

[AndrewKushnir]

Next steps: @AndrewKushnir to investigate in g3 further and come up with a test case that we can use to avoid regressions.

[AndrewKushnir]

@petebacondarwin I've performed the necessary investigation and found the origin of the problem. The template that can be used for repro:

<div i18n-attr.aria-label attr.aria-label="hello {{name}}"></div>

It's important to have an attribute that doesn't directly translate to a property. The underlying problem is that the i18nAttributes instruction that we generate actually sets a property on an element, not an attribute (taking an advantage that attribute translations were not supported by the compiler), see:

angular/packages/core/src/render3/i18n/i18n_apply.ts

Lines 279 to 296 in a91fe6d

	case I18nUpdateOpCode.Attr:
	const propName = updateOpCodes[++j] as string;
	const sanitizeFn = updateOpCodes[++j] as SanitizerFn | null;
	const tNodeOrTagName = tView.data[nodeIndex] as TNode | string;
	ngDevMode && assertDefined(tNodeOrTagName, 'Experting TNode or string');
	if (typeof tNodeOrTagName === 'string') {
	// IF we don't have a `TNode`, then we are an element in ICU (as ICU content does
	// not have TNode), in which case we know that there are no directives, and hence
	// we use attribute setting.
	setElementAttribute(
	lView[RENDERER], lView[nodeIndex], null, tNodeOrTagName, propName, value,
	sanitizeFn);
	} else {
	elementPropertyInternal(
	tView, tNodeOrTagName, lView, propName, value, lView[RENDERER], sanitizeFn,
	false);
	}
	break;

We end up calling elementPropertyInternal. It worked for the test case that we had, since:

const div = document.createElement('div');
div.title = 'abc';
div.getAttribute('title'); // outputs 'abc'

But for attributes like aria-label, that would not work (we'd just set a property, which would actually be incorrect).

It looks like the mentioned code in the i18n_apply.ts should have a clear indication when we should set an attribute vs a property for an element (in addition to the current if check that we have there). We might consider introducing 2 instructions: i18nAttributes and i18nProperties, which would use different OpCodes, so that the code in the i18n_apply.ts can differentiate between attributes and properties.

[AndrewKushnir]

FYI we also have a code that maps attribute names that don't correspond to their element property names:

https://github.com/angular/angular/blob/master/packages/core/src/render3/instructions/shared.ts#L1004

So we can potentially consider expanding the logic in the i18n_apply.ts (or the shared.ts) to have some extra mapping logic, but it would be either too heavy from code size perspective (if we just put 1:1 mapping for ARIA and other attributes) or too slow (if we try to apply some transform to an attribute name).

[amakhrov]

Should we reopen the original issue then (#43260)?
It currently looks like it's resolved, which is misleading (I actually ended up running through the changelog, and then looking at the PRs again, just to learn the fix has been reverted)

[AndrewKushnir]

Thanks for the comment @amakhrov, I've reopened the mentioned issue.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}