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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ivy] Nested ICU throws Failed to execute 'appendChild' on 'Node' #37021

Closed
HitkoDev opened this issue May 8, 2020 · 9 comments
Closed

[Ivy] Nested ICU throws Failed to execute 'appendChild' on 'Node' #37021

HitkoDev opened this issue May 8, 2020 · 9 comments
Assignees
Labels
area: i18n freq1: low P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: confirmed type: bug/fix
Milestone

Comments

@HitkoDev
Copy link

HitkoDev commented May 8, 2020

馃悶 bug report

Affected Package

The issue is caused by package @angular/core, @angular/compiler

Is this a regression?

Yes, the previous version in which this bug was not present was: before Ivy

Description

Using nested ICU expressions in Ivy results in render errors

馃敩 Minimal Reproduction

https://stackblitz.com/edit/angular-issue-repro2-qr9dqx

馃敟 Exception or Error

ERROR DOMException: Failed to execute 'appendChild' on 'Node': This node type does not support this method.
    at DefaultDomRenderer2.appendChild (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/platform-browser@9.1.6/__ivy_ngcc__/bundles/platform-browser.umd.js:909:20)
    at nativeAppendChild (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:9806:22)
    at nativeAppendOrInsertBefore (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:9817:13)
    at appendChild (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:9884:17)
    at appendI18nNode (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:24289:9)
    at readCreateOpCodes (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:24500:29)
    at readUpdateOpCodes (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:24637:41)
    at readUpdateOpCodes (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:24646:41)
    at Object.傻傻i18nApply (https://angular-issue-repro2-qr9dqx.stackblitz.io/turbo_modules/@angular/core@9.1.6/__ivy_ngcc__/bundles/core.umd.js:24814:13)
    at AppComponent_div_2_Template (https://angular-issue-repro2-qr9dqx.stackblitz.io/~/src/app/app.component.ts:27:8)

馃實 Your Environment

Angular Version:

Angular: 9.1.4
@ngbot ngbot bot modified the milestone: needsTriage May 9, 2020
@AndrewKushnir AndrewKushnir self-assigned this May 9, 2020
@HitkoDev
Copy link
Author

HitkoDev commented May 9, 2020

It seems with nested ICU getRenderParent returns a comment node, which in turn throws this error as comment nodes can't have children.

@AndrewKushnir AndrewKushnir added freq1: low regression Indicates than the issue relates to something that worked in a previous version type: bug/fix labels May 12, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog May 12, 2020
@AndrewKushnir
Copy link
Contributor

Hi @HitkoDev, thanks for reporting the issue, we were able to reproduce it. Initial investigation revealed that the problem happens only when ICU cases have different shape, i.e. one of them has a nested ICU and another one does not. As a temporary workaround (to get unblocked), try to update the code to use *ngSwitch instead of an ICU in that particular scenario. Thank you.

@HitkoDev
Copy link
Author

HitkoDev commented May 12, 2020

Thanks, I've already done that as a temporary fix. It did, however, increase translation complexity in a few places, but I'll take it until a proper fix is available.

@gizm0bill
Copy link

I got it fixed adding a zero-width space ​ before the nested expression

@HitkoDev
Copy link
Author

HitkoDev commented May 29, 2020

I got it fixed adding a zero-width space ​ before the nested expression

I've noticed the same thing, however it only works in this particular case because an extra text element offsets the index by 1 which happens to make compiler resolve the correct target. With slightly different ICU expressions that solution no longer works.

@gizm0bill
Copy link

I see @HitkoDev ... Do you mind sharing an example so I can check my code for that kind of case?

@HitkoDev
Copy link
Author

HitkoDev commented May 29, 2020

I don't have that code anymore (I don't recall which ICU in my app that fix broke, and I've since rewritten some of them to avoid this problem), but a combination of nested ICUs and HTML tags in ICU sometimes gave me

<div>
  <span>foo<span>bar</span></span>
</div>

instead of

<div>
  <span>foo</span>
  <span>bar</span>
</div>

It still got an incorrect getRenderParent, however thanks to this fix the returned parent was now a HTML element so it didn't broke completely, it just appended the result to the wrong parent.

@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Oct 1, 2020
@AndrewKushnir
Copy link
Contributor

FYI, this problem should be resolved by #39003.

mhevery added a commit to mhevery/angular that referenced this issue Oct 9, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 12, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 13, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 15, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 16, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 16, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 17, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 20, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 20, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 20, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 20, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 21, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 21, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 21, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 21, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
mhevery added a commit to mhevery/angular that referenced this issue Oct 21, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes angular#37021
closes angular#38144
closes angular#38073
alxhub pushed a commit that referenced this issue Oct 22, 2020
Before this refactoring/fix the ICU would store the current selected
index in `TView`. This is incorrect, since if ICU is in `ngFor` it will
cause issues in some circumstances. This refactoring properly moves the
state to `LView`.

closes #37021
closes #38144
closes #38073

PR Close #39233
@alxhub alxhub closed this as completed in ca11ef2 Oct 22, 2020
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: i18n freq1: low P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version state: confirmed type: bug/fix
Projects
None yet
Development

No branches or pull requests

4 participants