Skip to content
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

Jump to bottom

@petebacondarwin fix(core): handle multiple i18n attributes with expression bindings (PATCH) #41912

Closed
wants to merge 1 commit into from
Closed

@petebacondarwin fix(core): handle multiple i18n attributes with expression bindings (PATCH) #41912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 39 additions & 10 deletions packages/core/src/render3/i18n/i18n_parse.ts
View file
Open in desktop
Expand Up @@ -219,7 +219,7 @@ function i18nStartFirstCreatePassProcessTextNode(
const tNode = createTNodeAndAddOpCode(
tView, rootTNode, existingTNodes, lView, createOpCodes, hasBinding ? null : text, false);
if (hasBinding) {
generateBindingUpdateOpCodes(updateOpCodes, text, tNode.index);
generateBindingUpdateOpCodes(updateOpCodes, text, tNode.index, null, 0, null);
}
}

Expand Down Expand Up @@ -251,7 +251,11 @@ export function i18nAttributesFirstPass(tView: TView, index: number, values: str

// i18n attributes that hit this code path are guaranteed to have bindings, because
// the compiler treats static i18n attributes as regular attribute bindings.
generateBindingUpdateOpCodes(updateOpCodes, message, previousElementIndex, attrName);
// Since this may not be the first i18n attribute on this element we need to pass in how
// many previous bindings there have already been.
generateBindingUpdateOpCodes(
updateOpCodes, message, previousElementIndex, attrName, countBindings(updateOpCodes),
null);
}
}
tView.data[index] = updateOpCodes;
Expand All @@ -267,10 +271,12 @@ export function i18nAttributesFirstPass(tView: TView, index: number, values: str
* @param destinationNode Index of the destination node which will receive the binding.
* @param attrName Name of the attribute, if the string belongs to an attribute.
* @param sanitizeFn Sanitization function used to sanitize the string after update, if necessary.
* @param bindingStart The lView index of the next expression that can be bound via an opCode.
* @returns The mask value for these bindings
*/
export function generateBindingUpdateOpCodes(
updateOpCodes: I18nUpdateOpCodes, str: string, destinationNode: number, attrName?: string,
sanitizeFn: SanitizerFn|null = null): number {
function generateBindingUpdateOpCodes(
updateOpCodes: I18nUpdateOpCodes, str: string, destinationNode: number, attrName: string|null,
bindingStart: number, sanitizeFn: SanitizerFn|null): number {
ngDevMode &&
assertGreaterThanOrEqual(
destinationNode, HEADER_OFFSET, 'Index must be in absolute LView offset');
Expand All @@ -289,7 +295,7 @@ export function generateBindingUpdateOpCodes(

if (j & 1) {
// Odd indexes are bindings
const bindingIndex = parseInt(textValue, 10);
const bindingIndex = bindingStart + parseInt(textValue, 10);
updateOpCodes.push(-1 - bindingIndex);
mask = mask | toMaskBit(bindingIndex);
} else if (textValue !== '') {
Expand All @@ -309,6 +315,28 @@ export function generateBindingUpdateOpCodes(
return mask;
}

/**
* Count the number of bindings in the given `opCodes`.
*
* It could be possible to speed this up, by passing the number of bindings found back from
* `generateBindingUpdateOpCodes()` to `i18nAttributesFirstPass()` but this would then require more
* complexity in the code and/or transient objects to be created.
*
* Since this function is only called once when the template is instantiated, is trivial in the
* first instance (since `opCodes` will be an empty array), and it is not common for elements to
* contain multiple i18n bound attributes, it seems like this is a reasonable compromise.
*/
function countBindings(opCodes: I18nUpdateOpCodes): number {
let count = 0;
for (let i = 0; i < opCodes.length; i++) {
const opCode = opCodes[i];
// Bindings are negative numbers.
if (typeof opCode === 'number' && opCode < 0) {
count++;
}
}
return count;
}

/**
* Convert binding index to mask bit.
Expand Down Expand Up @@ -595,12 +623,12 @@ function walkIcuTree(
if (VALID_ATTRS.hasOwnProperty(lowerAttrName)) {
if (URI_ATTRS[lowerAttrName]) {
generateBindingUpdateOpCodes(
update, attr.value, newIndex, attr.name, _sanitizeUrl);
update, attr.value, newIndex, attr.name, 0, _sanitizeUrl);
} else if (SRCSET_ATTRS[lowerAttrName]) {
generateBindingUpdateOpCodes(
update, attr.value, newIndex, attr.name, sanitizeSrcset);
update, attr.value, newIndex, attr.name, 0, sanitizeSrcset);
} else {
generateBindingUpdateOpCodes(update, attr.value, newIndex, attr.name);
generateBindingUpdateOpCodes(update, attr.value, newIndex, attr.name, 0, null);
}
} else {
ngDevMode &&
Expand All @@ -627,7 +655,8 @@ function walkIcuTree(
addCreateNodeAndAppend(create, null, hasBinding ? '' : value, parentIdx, newIndex);
addRemoveNode(remove, newIndex, depth);
if (hasBinding) {
bindingMask = generateBindingUpdateOpCodes(update, value, newIndex) | bindingMask;
bindingMask =
generateBindingUpdateOpCodes(update, value, newIndex, null, 0, null) | bindingMask;
}
break;
case Node.COMMENT_NODE:
Expand Down
14 changes: 10 additions & 4 deletions packages/core/test/acceptance/i18n_spec.ts
View file
Open in desktop
Expand Up @@ -1604,17 +1604,23 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => {
});

it('multiple attributes', () => {
loadTranslations({[computeMsgId('hello {$INTERPOLATION}')]: 'bonjour {$INTERPOLATION}'});
loadTranslations({
[computeMsgId('hello {$INTERPOLATION} - {$INTERPOLATION_1}')]:
'bonjour {$INTERPOLATION} - {$INTERPOLATION_1}',
[computeMsgId('bye {$INTERPOLATION} - {$INTERPOLATION_1}')]:
'au revoir {$INTERPOLATION} - {$INTERPOLATION_1}',
});
const fixture = initWithTemplate(
AppComp,
`<input i18n i18n-title title="hello {{name}}" i18n-placeholder placeholder="hello {{name}}">`);
`<input i18n i18n-title title="hello {{name}} - {{count}}" i18n-placeholder placeholder="bye {{count}} - {{name}}">`);
expect(fixture.nativeElement.innerHTML)
.toEqual(`<input title="bonjour Angular" placeholder="bonjour Angular">`);
.toEqual(`<input title="bonjour Angular - 0" placeholder="au revoir 0 - Angular">`);

fixture.componentRef.instance.name = 'John';
fixture.componentRef.instance.count = 5;
fixture.detectChanges();
expect(fixture.nativeElement.innerHTML)
.toEqual(`<input title="bonjour John" placeholder="bonjour John">`);
.toEqual(`<input title="bonjour John - 5" placeholder="au revoir 5 - John">`);
});

it('on removed elements', () => {
Expand Down
13 changes: 8 additions & 5 deletions packages/core/test/render3/i18n/i18n_spec.ts
View file
Open in desktop
Expand Up @@ -449,9 +449,10 @@ describe('Runtime i18n', () => {
});

it('for multiple attributes', () => {
const message = `Hello �0�!`;
const attrs = ['title', message, 'aria-label', message];
const nbConsts = 2;
const message1 = `Hello �0� - �1�!`;
const message2 = `Bye �0� - �1�!`;
const attrs = ['title', message1, 'aria-label', message2];
const nbConsts = 4;
const index = 1;
const opCodes = getOpCodes(attrs, () => {
ɵɵelementStart(0, 'div');
Expand All @@ -460,8 +461,10 @@ describe('Runtime i18n', () => {
}, undefined, nbConsts, HEADER_OFFSET + index);

expect(opCodes).toEqual(matchDebug([
'if (mask & 0b1) { (lView[20] as Element).setAttribute(\'title\', `Hello ${lView[i-1]}!`); }',
'if (mask & 0b1) { (lView[20] as Element).setAttribute(\'aria-label\', `Hello ${lView[i-1]}!`); }',
`if (mask & 0b11) { (lView[${HEADER_OFFSET + 0}] as Element).` +
'setAttribute(\'title\', `Hello ${lView[i-1]} - ${lView[i-2]}!`); }',
`if (mask & 0b1100) { (lView[${HEADER_OFFSET + 0}] as Element).` +
'setAttribute(\'aria-label\', `Bye ${lView[i-3]} - ${lView[i-4]}!`); }',
]));
});
});
Expand Down