-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(tab-bar): prevent keyboard controller memory leak on rapid mount/unmount #30906
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
Changes from all commits
13b441d
6ea7cef
9c10dbe
4d19d31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ import type { TabBarChangedEventDetail } from './tab-bar-interface'; | |||||||||||||||||
| }) | ||||||||||||||||||
| export class TabBar implements ComponentInterface { | ||||||||||||||||||
| private keyboardCtrl: KeyboardController | null = null; | ||||||||||||||||||
| private keyboardCtrlPromise: Promise<KeyboardController> | null = null; | ||||||||||||||||||
| private didLoad = false; | ||||||||||||||||||
|
|
||||||||||||||||||
| @Element() el!: HTMLElement; | ||||||||||||||||||
|
|
@@ -88,7 +89,7 @@ export class TabBar implements ComponentInterface { | |||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async connectedCallback() { | ||||||||||||||||||
| this.keyboardCtrl = await createKeyboardController(async (keyboardOpen, waitForResize) => { | ||||||||||||||||||
| const promise = createKeyboardController(async (keyboardOpen, waitForResize) => { | ||||||||||||||||||
| /** | ||||||||||||||||||
| * If the keyboard is hiding, then we need to wait | ||||||||||||||||||
| * for the webview to resize. Otherwise, the tab bar | ||||||||||||||||||
|
|
@@ -100,11 +101,32 @@ export class TabBar implements ComponentInterface { | |||||||||||||||||
|
|
||||||||||||||||||
| this.keyboardVisible = keyboardOpen; // trigger re-render by updating state | ||||||||||||||||||
| }); | ||||||||||||||||||
| this.keyboardCtrlPromise = promise; | ||||||||||||||||||
|
|
||||||||||||||||||
| const keyboardCtrl = await promise; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Only assign if this is still the current promise. | ||||||||||||||||||
| * Otherwise, a new connectedCallback has started or | ||||||||||||||||||
| * disconnectedCallback was called, so destroy this instance. | ||||||||||||||||||
| */ | ||||||||||||||||||
| if (this.keyboardCtrlPromise === promise) { | ||||||||||||||||||
| this.keyboardCtrl = keyboardCtrl; | ||||||||||||||||||
| this.keyboardCtrlPromise = null; | ||||||||||||||||||
| } else { | ||||||||||||||||||
| keyboardCtrl.destroy(); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| disconnectedCallback() { | ||||||||||||||||||
| if (this.keyboardCtrlPromise) { | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this doesn't need to be done on this PR, but I feel like we do this same code in so many places that we should consider making it a utility.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this particular pattern is only specifically done in tab bar and footer (unless I'm missing something?), so I'm not sure if that's worth extracting right now
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mostly meant the code that is destroying like this: ionic-framework/core/src/components/select/select.tsx Lines 376 to 379 in 1c89cf0
We also disconnect in a lot of places: ionic-framework/core/src/components/select/select.tsx Lines 382 to 385 in 1c89cf0
It seems like we could make this into a utility or something, but that's not an issue for your PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make a ticket to circle back on adding the utility.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thetaPC Done! This will be FW-7030 |
||||||||||||||||||
| this.keyboardCtrlPromise.then((ctrl) => ctrl.destroy()); | ||||||||||||||||||
| this.keyboardCtrlPromise = null; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (this.keyboardCtrl) { | ||||||||||||||||||
| this.keyboardCtrl.destroy(); | ||||||||||||||||||
| this.keyboardCtrl = null; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.