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

Duplicate text when using ICU expression in list without 'other' #38073

Closed
sodaDreamer opened this issue Jul 15, 2020 · 2 comments
Closed

Duplicate text when using ICU expression in list without 'other' #38073

sodaDreamer opened this issue Jul 15, 2020 · 2 comments
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

@sodaDreamer
Copy link

sodaDreamer commented Jul 15, 2020

馃悶 bug report

Affected Package

The issue is caused by package @angular/....

Is this a regression?

Pretty sure this wasn't an issue in Angular 7. Noticed the issue after updating to Angular 9.

Description

When using an ICU select expression inside a list, if you do not include the "other" placeholder, then subsequent changes to an item in the list causes the replacement text to display twice.

Issue #34018 suggests that the "other" case should actually be required but this is now no longer enforced. It might be worth reinstating the warning if this is deemed not to be a bug.

馃敩 Minimal Reproduction

https://stackblitz.com/edit/angular-ivy-cuoymq?file=src%2Fapp%2Fapp.component.html

馃實 Your Environment

Angular Version:


Angular CLI: 9.1.10
Node: 12.18.2
OS: win32 x64

Angular: 9.1.11
... animations, common, compiler, compiler-cli, core, forms
... language-service, localize, platform-browser
... platform-browser-dynamic, router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.901.10
@angular-devkit/build-angular     0.901.10
@angular-devkit/build-optimizer   0.901.10
@angular-devkit/build-webpack     0.901.10
@angular-devkit/core              9.1.10
@angular-devkit/schematics        9.1.10
@angular/cli                      9.1.10
@ngtools/webpack                  9.1.10
@schematics/angular               9.1.10
@schematics/update                0.901.10
rxjs                              6.6.0
typescript                        3.8.3
webpack                           4.42.0

@ngbot ngbot bot added this to the needsTriage milestone Jul 15, 2020
@petebacondarwin petebacondarwin added core: DOM rendering state: confirmed type: bug/fix freq1: low regression Indicates than the issue relates to something that worked in a previous version labels Jul 15, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 16, 2020
@mhevery
Copy link
Contributor

mhevery commented Jul 17, 2020

First let me say thank you for creating simple and easy to follow reproduction with text explaining what to click and what is about to happen. KUDOS!

Yes, this seems to be a bug. While ICU may require other I don't see a reason why that should affect the runtime algorithm.

crisbeto added a commit to crisbeto/angular that referenced this issue Jul 18, 2020
Fixes an issue where the previous value of an ICU expression wasn't being
removed, if it didn't have an `other` parameter and it's the last value inside
a template. The issue seems to be due to the fact that we were resetting
the index of the active expression if we didn't manage to match a value
to it which prevented the previous node from being cleaned up on the
next template run.

Fixes angular#38073.
crisbeto added a commit to crisbeto/angular that referenced this issue Jul 18, 2020
Fixes an issue where the previous value of an ICU expression wasn't being
removed, if it didn't have an `other` parameter and it's the last value inside
a template. The issue seems to be due to the fact that we were resetting
the index of the active expression if we didn't manage to match a value
to it which prevented the previous node from being cleaned up on the
next template run.

Also cleans up an optional parameter in one of the internal functions and
adds stronger typing to the i18n unit tests.

Fixes angular#38073.
@crisbeto crisbeto self-assigned this Jul 18, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Jul 20, 2020
Fixes an issue where the previous value of an ICU expression wasn't being
removed, if it didn't have an `other` parameter and it's the last value inside
a template. The issue is due to the `createDynamicNodeAtIndex` function
which sets a new node at a particular index inside the `LView`, but it doesn't
account for any node that may have existed there beforehand. Since we've
overwritten the reference, we don't have a way of removing the node from
the DOM anymore.

Also cleans up an optional parameter in one of the internal functions and
adds stronger typing to the i18n unit tests.

Fixes angular#38073.

fixup! fix(core): ICU expression previous value not removed inside template

fixup! fix(core): ICU expression previous value not removed inside template
crisbeto added a commit to crisbeto/angular that referenced this issue Jul 20, 2020
Fixes an issue where the previous value of an ICU expression wasn't being
removed, if it didn't have an `other` parameter and it's the last value inside
a template. The issue is due to the `createDynamicNodeAtIndex` function
which sets a new node at a particular index inside the `LView`, but it doesn't
account for any node that may have existed there beforehand. Since we've
overwritten the reference, we don't have a way of removing the node from
the DOM anymore.

Also cleans up an optional parameter in one of the internal functions and
adds stronger typing to the i18n unit tests.

Fixes angular#38073.
crisbeto added a commit to crisbeto/angular that referenced this issue Aug 9, 2020
Fixes an issue where the previous value of an ICU expression wasn't being
removed, if it didn't have an `other` parameter and it's the last value inside
a template. The issue is due to the `createDynamicNodeAtIndex` function
which sets a new node at a particular index inside the `LView`, but it doesn't
account for any node that may have existed there beforehand. Since we've
overwritten the reference, we don't have a way of removing the node from
the DOM anymore.

Also cleans up an optional parameter in one of the internal functions and
adds stronger typing to the i18n unit tests.

Fixes angular#38073.
@crisbeto crisbeto removed their assignment Aug 11, 2020
@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Oct 1, 2020
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

Successfully merging a pull request may close this issue.

7 participants