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

fix(VirtualScroll): cancel raf on destroy #13107

Closed
wants to merge 189 commits into from
Closed

fix(VirtualScroll): cancel raf on destroy #13107

wants to merge 189 commits into from

Conversation

masimplo
Copy link
Contributor

Short description of what this resolves:

Cancel any scheduled requestAnimationFrame when virtualScroll component is destroyed to prevent accessing cleaned up collections and causing cannot access x of undefined errors.

Changes proposed in this pull request:

Ionic Version: 3.x

Fixes: #

brandyscarney and others added 30 commits June 12, 2017 13:51
fixes #9699
fixes #11484
fixes #11389
fixes #11325
fixes #11291
fixes #10828
fixes #11291
fixes #10393
fixes #10257
fixes #9434
fixes #8933
fixes #7178
fixes #7047
fixes #10552
fixes #10393
fixes #10183
fixes #10187
fixes #10852
fixes #11578
When `Tabs` are nested within each other, the highlight can get
misaligned. This prevents that by ensuring the affected
`.tab-highlight` is a direct child of the targeted `Tabs`.
…tabs

* wip

* wip

* progress

* wippy skippy

* getting there

* all tests passing except goBack

* unit tests pass again boi

* goBack tests pass

* great success

* the good stuff
kensodemann and others added 22 commits September 29, 2017 17:41
…s exceed the screen (#13049)

* fix(action-sheet): add ability to scroll buttons in an action sheet

* fix(action-sheet): add scroll to buttons for all modes

* fix(select): add support for more than 6 options in action-sheet

* style(sass): fix linter error

* fix(action-sheet): add flex-shrink to all modes and fix border bleed

also adds variables for md and wp mode padding and removes unused
$action-sheet-md-group-margin-bottom var

* style(sass): fix linter errors

* refactor(action-sheet): remove duplicated overflow styles

* fix(action-sheet): get scroll working properly without cancel button

* fix(action-sheet): remove pointer events from wrapper
Squashed commit of the following:

commit 3497d40
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Wed Sep 20 10:52:39 2017 +0300

    Fix virtualScroll spec

commit c6a2a97
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Wed Sep 20 10:51:58 2017 +0300

    Fix virtualScroll e2e infiniteScroll test

commit 6eaf7d1
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Wed Sep 20 09:40:01 2017 +0300

    Use const enums to reduce generated code

commit 564ef41
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Tue Sep 19 16:12:01 2017 +0300

    Recalculated nodes once more on scrollEnd

commit bfa44b9
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 19:10:55 2017 +0300

    Remove needClean param from populateNodeData

commit 84d402d
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 19:06:06 2017 +0300

    Remove console.warn

commit 07e15d7
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 17:50:14 2017 +0300

    Simplify adjustRendered function

commit 9b3b0db
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 17:14:39 2017 +0300

    Use recordSize consistently

commit c8d5e09
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 16:27:09 2017 +0300

    Fix top and bottom cells special cases

commit acec8ef
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 13:26:33 2017 +0300

    Update existing nodes by matching cells

commit 8590b9b
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Mon Sep 18 13:25:00 2017 +0300

    Stop clearing DOM in slow path

commit e06d757
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 17:20:18 2017 +0300

    Return records from VirtualScroll getter

commit a13184f
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 15:12:31 2017 +0300

    Change comment

commit 75aed51
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 15:01:17 2017 +0300

    Stop looking for available nodes after one is found

commit ebef282
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:49:36 2017 +0300

    Change TemplateType consts to enum

commit 21a7ce9
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:45:47 2017 +0300

    Stop searching nodes if cell is already rendered

commit 7d3b7c4
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:36:11 2017 +0300

    Make debug logs consistent

commit a716e58
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:33:29 2017 +0300

    Simplify changes calculation

commit ad9c6d9
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:22:41 2017 +0300

    Simplify addCell function

commit 8e5774a
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:17:03 2017 +0300

    Throw errors not strings

commit fdd9bd1
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:12:39 2017 +0300

    Initialize differ with trackByFunction as ngForOf does

commit 4cf1f39
Author: Michael Asimakopoulos <m.asimakopoulos@gmail.com>
Date:   Fri Sep 15 14:08:11 2017 +0300

    Convert queue consts to enum
* fix(overlay): onWillDismiss is called as expected

fixes #11702

* Fix condition
* refactor(OpaqueToken): ready for Angular 4.2

* Create app-root.ts

* Create config.ts

* Create url-serializer.ts

* Create platform-registry.ts

* Create module-loader.ts
* style(): better list design for ios

* fix(): fix lint issues

* fix(): more ios11 updates

* fix(): use env and constant for now

* fix(footer): user correct value for padding

* fix(): remove extra padding on title

* fix(): reorder imports to pass lint
@manucorporat manucorporat self-requested a review October 12, 2017 23:34
@manucorporat
Copy link
Contributor

Going to merge this after 3.8.0 is released!

@Kevin-Hamilton
Copy link

Kevin-Hamilton commented Oct 21, 2017

I don't think this commit sufficiently addresses the issue.

DomController.read() and DomController.write() both queue their callbacks to be called on the next requestAnimationFrame. I believe it is possible that after scheduling that callback and before the callback occurs, the cleanup function ngOnDestroy() could get called and null out variables that the callback functions assume to be in scope.

So to be complete, I think every _dom.read() and _dom.write() callback would need to check whether ngOnDestroy has been called.

See: https://github.com/ionic-team/ionic/blob/master/src/platform/dom-controller.ts#L109

@Ionitron
Copy link
Collaborator

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!

@Ionitron Ionitron closed this Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.