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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: clarify ivy breaking change for ngFor and <select> with animations and no trackBy #35896

Closed
rsheptolut opened this issue Mar 6, 2020 · 17 comments
Assignees
Labels
area: core Issues related to the framework runtime
Milestone

Comments

@rsheptolut
Copy link

rsheptolut commented Mar 6, 2020

🐞 bug report

Affected Package

The issue seems to be related to package @angular/platform-browser

Is this a regression?

Yes, the previous version in which this bug was not present was: v8.* (8.2.14)

Description

Upgrading to Angular v9 broke the functionality of <select> for us. The *ngFor directive we use for populating it with <option> elements now all of the sudden re-renders items if you simply refresh the collection with new instances, even if their contents are the same. This resets the selected option, which is not expected and not desirable. This wasn't happening in v8. This isn't happening in v9 either, unless BrowserAnimationsModule is imported by the app-module. This also can be worked around by using trackBy, but for that you'd need to track down all the problematic ngFor directives after upgrading to v9.

🔬 Minimal Reproduction

Click here for v9 repro repository
Click here to see the same v9 repro app in action
Same demo working without problems in Angular v8 - although StackBlitz doesn't have this problem with v9 either (not sure why), but at least helps illustrate how it was working before

The v9 repro is based on ng new of Angular CLI 9.0.5. If building yourself, run locally with ng serve, because if imported into StackBlitz, the issue can't be reproduced.

  1. Choose an item in any drop down (the two are linked and simply illustrate the trackBy difference)
  2. Notice that the first button adds a new item but the selected option doesn't get reset.
  3. Clicking the second button though resets the selected option in the first drop down.

🔥 Exception or Error



 None

🌍 Your Environment

Angular Version:


Angular CLI: 9.0.5
Node: 10.16.3
OS: win32 x64

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

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.5
@angular-devkit/build-angular     0.900.5
@angular-devkit/build-optimizer   0.900.5
@angular-devkit/build-webpack     0.900.5
@angular-devkit/core              9.0.5
@angular-devkit/schematics        9.0.5
@ngtools/webpack                  9.0.5
@schematics/angular               9.0.5
@schematics/update                0.900.5
rxjs                              6.5.4
typescript                        3.7.5
webpack                           4.41.2

Anything else relevant?
Happens only in Angular v9, when BrowserAnimationsModule is imported into AppModule and ngFor is not set up with trackBy.

@C0ZEN
Copy link

C0ZEN commented Mar 6, 2020

I could not tell you if this is really a bug or a feature of the v9 nevertheless the trackBy feature is dedicated to avoid re-rendering the DOM to improve the performances.
It seems to me that you should have used a trackBy and your issue is in fact, a bad design from your project.

Even if Angular considers it as a bug, you should really use the trackBy anytime it is possible.

@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Mar 6, 2020
@ngbot ngbot bot added this to the needsTriage milestone Mar 6, 2020
@kara kara added area: animations and removed area: core Issues related to the framework runtime labels Mar 11, 2020
@matsko matsko added freq3: high regression Indicates than the issue relates to something that worked in a previous version type: bug/fix labels Mar 12, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 12, 2020
@xonuzofa
Copy link

xonuzofa commented Apr 28, 2020

This is still happening on Angular 9.1.3. Without using trackBy, when assigning a new list to the *ngFor list in a select, it sets the select back to the default value '', but the form contains the correct id and the formControlName is not working with select correctly.

Also figured out BrowserAnimationsModule is the cause of this when adding to AppModule

@atscott
Copy link
Contributor

atscott commented May 8, 2020

This appears to be related to the breaking change related to insertion order (reference):

Embedded views (such as ones created by *ngFor) are now inserted in front of anchor DOM comment node (e.g. ) rather than behind it as was the case previously. In most cases this does not have any impact on rendered DOM. In some cases (such as animations delaying the removal of an embedded view) any new embedded views will be inserted after the embedded view being animated away. This difference only last while the animation is active, and might alter the visual appearance of the animation. Once the animation is finished the resulting rendered DOM is identical to that rendered with View Engine.

In the stackblitz example, you can see that when Ivy is disabled, the new items are added to the front of the list (add a break on: subtree modifications to the dom). If you go to the settings in stackblitz and check the box to "Enable Ivy", you'll see that new items are added to the end of the list. To clarify, both Ivy and View Engine are re-rendering the items because you don't have trackBy. The difference is the location of the newly inserted items.
Because BrowserAnimationsModule queues up the removal of the old items in order to animate them, you will briefly have duplicate items in the list if you don't use trackBy. In the Ivy example, since the old items being removed are above the new items, when they're removed, the browser sees this as the selected item being removed.
As a quick fix, I would recommend using trackBy to avoid this issue.

@matsko matsko added area: core Issues related to the framework runtime triage #1 type: confusing and removed area: animations type: bug/fix labels May 27, 2020
@ngbot ngbot bot modified the milestones: Backlog, needsTriage May 27, 2020
@matsko
Copy link
Contributor

matsko commented May 27, 2020

@rsheptolut unfortunately the only way forward with this is to use trackBy. This API allows for the behavior to stay the same despite the internals of the framework changing.

@matsko matsko closed this as completed May 27, 2020
@xonuzofa
Copy link

How is this a valid solution to suggest using trackBy? BrowserAnimationsModule breaks selects in Angular 9, this is new breaking changes. Selects are used widely throughout all web applications. trackBy has never been a necessity before, but all of a sudden is.

I have 15 applications using Angular 9 that all inherited this issue and have no way to fix this without tremendous overhaul of all applications by adding trackBy functions on every single *ngFor.

Angular team should not expect people who rely on Angular to rework their entire source code and have temporary work arounds when they break huge components of html!

@C0ZEN
Copy link

C0ZEN commented May 27, 2020

RTFM. trackBy is one of the famous step forward the optimization of the DOM and has always been a very recommend step since as long as I aware of the ngFor directive. Let me prove it to you simply by linking the very first ngx documentation about the ngFor showing in EACH example the trackBy. Hell maybe they should have made it required but there is always someone to complain.
Yes there is a breaking change, yes this is unfortunate and yes the lack of communication is very bad but keep in mind that this is a feature to enhance your application.

@petebacondarwin
Copy link
Member

@matsko and @atscott - I wonder if it could be possible to identify such cases and provide a migration to help?

theodorejb added a commit to theodorejb/angular that referenced this issue Jan 15, 2021
…alue

Given the following component:

    @component({
      selector: 'ng-model-select-form',
      template: `
        <select [(ngModel)]="selectedCity">
          <option *ngFor="let c of cities" [ngValue]="c">{{c.name}}</option>
        </select>
      `
    })
    class NgModelSelectForm {
      selectedCity: {name: string} | null = null;
      cities: {name: string}[] = [];
    }

Previously, when values were pushed into the cities array after a
delay (e.g. from an API request), IE 11 and Safari (tested in v11-v13)
would display the first city in the dropdown as being selected, despite
the `selectedCity` property still being null. This broke the form for
end users, because the UI state didn't match the component state. In
some cases this could completely prevent users from submitting a form.

The problem occurs because Angular sets the select element value
before appending an option's child text node to the DOM. Once the
text node is appended, Safari and IE 11 ignore the previously set
select value and instead mark the first option element as selected.

This commit fixes the issue by setting the select value again in the
`AfterViewChecked` hook. `NgZone.onStable` is used to wait for changes
to be applied when using `BrowserAnimationsModule`. This results in
correct behavior across all supported browsers (an option is only
selected when its value matches that of the select element).

This also fixes an issue in all browsers caused by delayed element
removal when using the animations module, where select dropdowns would
display an option that didn't match the ngModel after removing the
selected option. This occurred because when a selected option is
removed, Angular changes the select element value before actually
removing the option from the DOM. Then when the option is finally
removed from the DOM, the browser would change the select value to
that of the first option, even though it didn't match the ngModel.

Fixes angular#14505, fixes angular#18430, fixes angular#35896.
theodorejb added a commit to theodorejb/angular that referenced this issue Feb 23, 2021
…alue

Given the following component:

    @component({
      selector: 'ng-model-select-form',
      template: `
        <select [(ngModel)]="selectedCity">
          <option *ngFor="let c of cities" [ngValue]="c">{{c.name}}</option>
        </select>
      `
    })
    class NgModelSelectForm {
      selectedCity: {name: string} | null = null;
      cities: {name: string}[] = [];
    }

Previously, when values were pushed into the cities array after a
delay (e.g. from an API request), IE 11 and Safari (tested in v11-v13)
would display the first city in the dropdown as being selected, despite
the `selectedCity` property still being null. This broke the form for
end users, because the UI state didn't match the component state. In
some cases this could completely prevent users from submitting a form.

The problem occurs because Angular sets the select element value
before appending an option's child text node to the DOM. Once the
text node is appended, Safari and IE 11 ignore the previously set
select value and instead mark the first option element as selected.

This commit fixes the issue by setting the select value again in the
`AfterViewChecked` hook. `NgZone.onStable` is used to wait for changes
to be applied when using `BrowserAnimationsModule`. This results in
correct behavior across all supported browsers (an option is only
selected when its value matches that of the select element).

This also fixes an issue in all browsers caused by delayed element
removal when using the animations module, where select dropdowns would
display an option that didn't match the ngModel after removing the
selected option. This occurred because when a selected option is
removed, Angular changes the select element value before actually
removing the option from the DOM. Then when the option is finally
removed from the DOM, the browser would change the select value to
that of the first option, even though it didn't match the ngModel.

Fixes angular#14505, fixes angular#18430, fixes angular#35896.
theodorejb added a commit to theodorejb/angular that referenced this issue Mar 10, 2021
…value

Given the following component:

    @component({
      selector: 'ng-model-select-form',
      template: `
        <select [(ngModel)]="selectedCity">
          <option *ngFor="let c of cities" [ngValue]="c">{{c.name}}</option>
        </select>
      `
    })
    class NgModelSelectForm {
      selectedCity: {name: string} | null = null;
      cities: {name: string}[] = [];
    }

Previously, when values were pushed into the cities array after a
delay (e.g. from an API request), IE 11 and Safari (tested in v11-v13)
would display the first city in the dropdown as being selected, despite
the `selectedCity` property still being null. This broke the form for
end users, because the UI state didn't match the component state. In
some cases this could completely prevent users from submitting a form.

The problem occurs because Angular sets the select element value
before appending an option's child text node to the DOM. Once the
text node is appended, Safari and IE 11 ignore the previously set
select value and instead mark the first option element as selected.

This commit fixes the issue by setting the select value again in the
`AfterViewChecked` hook. `NgZone.onStable` is used to wait for changes
to be applied when using `BrowserAnimationsModule`. This results in
correct behavior across all supported browsers (an option is only
selected when its value matches that of the select element).

This also fixes an issue in all browsers caused by delayed element
removal when using the animations module, where select dropdowns would
display an option that didn't match the ngModel after removing the
selected option. This occurred because when a selected option is
removed, Angular changes the select element value before actually
removing the option from the DOM. Then when the option is finally
removed from the DOM, the browser would change the select value to
that of the first option, even though it didn't match the ngModel.

Fixes angular#14505, fixes angular#18430, fixes angular#35896.
theodorejb added a commit to theodorejb/angular that referenced this issue Mar 14, 2021
…value

Given the following component:

    @component({
      selector: 'ng-model-select-form',
      template: `
        <select [(ngModel)]="selectedCity">
          <option *ngFor="let c of cities" [ngValue]="c">{{c.name}}</option>
        </select>
      `
    })
    class NgModelSelectForm {
      selectedCity: {name: string} | null = null;
      cities: {name: string}[] = [];
    }

Previously, when values were pushed into the cities array after a
delay (e.g. from an API request), IE 11 and Safari (tested in v11-v13)
would display the first city in the dropdown as being selected, despite
the `selectedCity` property still being null. This broke the form for
end users, because the UI state didn't match the component state. In
some cases this could completely prevent users from submitting a form.

The problem occurs because Angular sets the select element value
before appending an option's child text node to the DOM. Once the
text node is appended, Safari and IE 11 ignore the previously set
select value and instead mark the first option element as selected.

This commit fixes the issue by setting the select value again in the
`AfterViewChecked` hook. `NgZone.onStable` is used to wait for changes
to be applied when using `BrowserAnimationsModule`. This results in
correct behavior across all supported browsers (an option is only
selected when its value matches that of the select element).

This also fixes an issue in all browsers caused by delayed element
removal when using the animations module, where select dropdowns would
display an option that didn't match the ngModel after removing the
selected option. This occurred because when a selected option is
removed, Angular changes the select element value before actually
removing the option from the DOM. Then when the option is finally
removed from the DOM, the browser would change the select value to
that of the first option, even though it didn't match the ngModel.

Fixes angular#14505, fixes angular#18430, fixes angular#35896.
@petebacondarwin
Copy link
Member

It looks like this is a bug that requires a code fix, so removing the comp:docs label.

@petebacondarwin petebacondarwin changed the title Upgrading to v9 makes *ngFor re-render items, breaking <select> for us docs: clarify ivy breaking change for ngFor and <select> with animations and no trackBy May 24, 2021
@petebacondarwin petebacondarwin added comp: docs and removed P2 The issue is important to a large percentage of users, with a workaround freq3: high regression Indicates than the issue relates to something that worked in a previous version triage #1 type: confusing labels May 24, 2021
@ngbot ngbot bot modified the milestones: Backlog, needsTriage May 24, 2021
@petebacondarwin
Copy link
Member

Having discussed offline with @atscott - I now understand that this is part of how Angular works (by design) and it just happens that the browser didn't notice with View Engine, while it becomes obvious under Ivy.

Therefore this specific case needs to be listed more clearly in our Ivy breaking changes so that people can more quickly resolve their problem.

IgorMinar added a commit to IgorMinar/angular that referenced this issue May 26, 2021
An internal change in Ivy has surfaced issues in previosly broken code. This change adds a note to the
Ivy compatibility guide as well as the TrackByFunction api docs.

Fixes angular#35896
@IgorMinar IgorMinar self-assigned this May 26, 2021
IgorMinar added a commit to IgorMinar/angular that referenced this issue May 27, 2021
An internal change in Ivy has surfaced issues in previosly broken code. This change adds a note to the
Ivy compatibility guide as well as the TrackByFunction api docs.

Fixes angular#35896
AndrewKushnir pushed a commit that referenced this issue May 27, 2021
…ide (#42338)

An internal change in Ivy has surfaced issues in previosly broken code. This change adds a note to the
Ivy compatibility guide as well as the TrackByFunction api docs.

Fixes #35896

PR Close #42338
umairhm pushed a commit to umairhm/angular that referenced this issue May 28, 2021
…ide (angular#42338)

An internal change in Ivy has surfaced issues in previosly broken code. This change adds a note to the
Ivy compatibility guide as well as the TrackByFunction api docs.

Fixes angular#35896

PR Close angular#42338
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this issue Jun 4, 2021
…ide (angular#42338)

An internal change in Ivy has surfaced issues in previosly broken code. This change adds a note to the
Ivy compatibility guide as well as the TrackByFunction api docs.

Fixes angular#35896

PR Close angular#42338
@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 Jun 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime
Projects
None yet
Development

No branches or pull requests