From 5fabba203bfd807c178fe49e8bf387c646bb082e Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Tue, 13 Sep 2016 22:16:52 +0200 Subject: [PATCH] fix(menu): open/close race condition fixes #7629 #8001 --- src/components/menu/menu-gestures.ts | 19 ++-- src/components/menu/menu.ts | 132 +++++++++++++-------------- src/gestures/slide-gesture.ts | 47 ++++++---- src/navigation/swipe-back.ts | 2 +- 4 files changed, 107 insertions(+), 93 deletions(-) diff --git a/src/components/menu/menu-gestures.ts b/src/components/menu/menu-gestures.ts index b9b277c32a2..f1db9b90d1d 100644 --- a/src/components/menu/menu-gestures.ts +++ b/src/components/menu/menu-gestures.ts @@ -24,7 +24,7 @@ export class MenuContentGesture extends SlideEdgeGesture { canStart(ev: any): boolean { let menu = this.menu; - if (!menu.enabled || !menu.swipeEnabled) { + if (!menu.canSwipe()) { return false; } if (menu.isOpen) { @@ -36,7 +36,7 @@ export class MenuContentGesture extends SlideEdgeGesture { } // Set CSS, then wait one frame for it to apply before sliding starts - onSlideBeforeStart(slide: SlideData, ev: any) { + onSlideBeforeStart(ev: any) { console.debug('menu gesture, onSlideBeforeStart', this.menu.side); this.menu.swipeStart(); } @@ -44,7 +44,15 @@ export class MenuContentGesture extends SlideEdgeGesture { onSlide(slide: SlideData, ev: any) { let z = (this.menu.side === 'right' ? slide.min : slide.max); let stepValue = (slide.distance / z); - console.debug('menu gesture, onSlide', this.menu.side, 'distance', slide.distance, 'min', slide.min, 'max', slide.max, 'z', z, 'stepValue', stepValue); + + console.debug( + 'menu gesture, onSlide', this.menu.side, + 'distance', slide.distance, + 'min', slide.min, + 'max', slide.max, + 'z', z, + 'stepValue', stepValue); + ev.preventDefault(); this.menu.swipeProgress(stepValue); } @@ -70,28 +78,25 @@ export class MenuContentGesture extends SlideEdgeGesture { 'shouldCompleteLeft', shouldCompleteLeft, 'shouldCompleteRight', shouldCompleteRight, 'currentStepValue', currentStepValue); + this.menu.swipeEnd(shouldCompleteLeft, shouldCompleteRight, currentStepValue); } getElementStartPos(slide: SlideData, ev: any) { if (this.menu.side === 'right') { - // right menu return this.menu.isOpen ? slide.min : slide.max; } - // left menu return this.menu.isOpen ? slide.max : slide.min; } getSlideBoundaries(): {min: number, max: number} { if (this.menu.side === 'right') { - // right menu return { min: -this.menu.width(), max: 0 }; } - // left menu return { min: 0, diff --git a/src/components/menu/menu.ts b/src/components/menu/menu.ts index 63e20f66e5d..4f4114f8fdf 100644 --- a/src/components/menu/menu.ts +++ b/src/components/menu/menu.ts @@ -186,16 +186,17 @@ import { GestureController } from '../../gestures/gesture-controller'; encapsulation: ViewEncapsulation.None, }) export class Menu { - private _preventTime: number = 0; private _cntEle: HTMLElement; private _cntGesture: MenuContentGesture; private _type: MenuType; private _resizeUnreg: Function; private _isEnabled: boolean = true; private _isSwipeEnabled: boolean = true; + private _isAnimating: boolean = false; private _isPers: boolean = false; private _init: boolean = false; + /** * @private */ @@ -378,21 +379,20 @@ export class Menu { * @private */ private _setListeners() { - let self = this; - - if (self._init) { - // only listen/unlisten if the menu has initialized + if (!this._init) { + return; + } - if (self._isEnabled && self._isSwipeEnabled && !self._cntGesture.isListening) { - // should listen, but is not currently listening - console.debug('menu, gesture listen', self.side); - self._cntGesture.listen(); + // only listen/unlisten if the menu has initialized + if (this._isEnabled && this._isSwipeEnabled && !this._cntGesture.isListening) { + // should listen, but is not currently listening + console.debug('menu, gesture listen', this.side); + this._cntGesture.listen(); - } else if (self._cntGesture.isListening && (!self._isEnabled || !self._isSwipeEnabled)) { - // should not listen, but is currently listening - console.debug('menu, gesture unlisten', self.side); - self._cntGesture.unlisten(); - } + } else if (this._cntGesture.isListening && (!this._isEnabled || !this._isSwipeEnabled)) { + // should not listen, but is currently listening + console.debug('menu, gesture unlisten', this.side); + this._cntGesture.unlisten(); } } @@ -416,7 +416,7 @@ export class Menu { setOpen(shouldOpen: boolean, animated: boolean = true): Promise { // _isPrevented is used to prevent unwanted opening/closing after swiping open/close // or swiping open the menu while pressing down on the MenuToggle button - if ((shouldOpen && this.isOpen) || this._isPrevented()) { + if ((shouldOpen && this.isOpen) || !this._isEnabled || this._isAnimating) { return Promise.resolve(this.isOpen); } @@ -430,12 +430,20 @@ export class Menu { }); } + + /** + * @private + */ + canSwipe(): boolean { + return this._isEnabled && this._isSwipeEnabled && !this._isAnimating; + } + /** * @private */ swipeStart() { // user started swiping the menu open/close - if (this._isEnabled && this._isSwipeEnabled && !this._isPrevented()) { + if (this.canSwipe()) { this._before(); this._getType().setProgressStart(this.isOpen); } @@ -446,46 +454,42 @@ export class Menu { */ swipeProgress(stepValue: number) { // user actively dragging the menu - if (this._isEnabled && this._isSwipeEnabled) { - this._prevent(); - this._getType().setProgessStep(stepValue); - this.ionDrag.emit(stepValue); + if (!this._isAnimating) { + return; } + this._getType().setProgessStep(stepValue); + this.ionDrag.emit(stepValue); } /** * @private */ swipeEnd(shouldCompleteLeft: boolean, shouldCompleteRight: boolean, stepValue: number) { + if (!this._isAnimating) { + return; + } // user has finished dragging the menu - if (this._isEnabled && this._isSwipeEnabled) { - this._prevent(); - - let opening = !this.isOpen; - let shouldComplete = false; - if (opening) { - shouldComplete = (this.side === 'right') ? shouldCompleteLeft : shouldCompleteRight; - } else { - shouldComplete = (this.side === 'right') ? shouldCompleteRight : shouldCompleteLeft; - } - - this._getType().setProgressEnd(shouldComplete, stepValue, (isOpen: boolean) => { - console.debug('menu, swipeEnd', this.side); - this._after(isOpen); - }); + let opening = !this.isOpen; + let shouldComplete = false; + if (opening) { + shouldComplete = (this.side === 'right') ? shouldCompleteLeft : shouldCompleteRight; + } else { + shouldComplete = (this.side === 'right') ? shouldCompleteRight : shouldCompleteLeft; } + + this._getType().setProgressEnd(shouldComplete, stepValue, (isOpen: boolean) => { + console.debug('menu, swipeEnd', this.side); + this._after(isOpen); + }); } private _before() { // this places the menu into the correct location before it animates in // this css class doesn't actually kick off any animations - if (this._isEnabled) { - this.getNativeElement().classList.add('show-menu'); - this.getBackdropElement().classList.add('show-backdrop'); - - this._prevent(); - this._keyboard.close(); - } + this.getNativeElement().classList.add('show-menu'); + this.getBackdropElement().classList.add('show-backdrop'); + this._keyboard.close(); + this._isAnimating = true; } private _after(isOpen: boolean) { @@ -493,37 +497,24 @@ export class Menu { // only add listeners/css if it's enabled and isOpen // and only remove listeners/css if it's not open // emit opened/closed events - if ((this._isEnabled && isOpen) || !isOpen) { - this._prevent(); - - this.isOpen = isOpen; + this.isOpen = isOpen; + this._isAnimating = false; - (this._cntEle.classList)[isOpen ? 'add' : 'remove']('menu-content-open'); + (this._cntEle.classList)[isOpen ? 'add' : 'remove']('menu-content-open'); - this._cntEle.removeEventListener('click', this.onContentClick); + this._cntEle.removeEventListener('click', this.onContentClick); - if (isOpen) { - this._cntEle.addEventListener('click', this.onContentClick); - this.ionOpen.emit(true); + if (isOpen) { + this._cntEle.addEventListener('click', this.onContentClick); + this.ionOpen.emit(true); - } else { - this.getNativeElement().classList.remove('show-menu'); - this.getBackdropElement().classList.remove('show-backdrop'); - this.ionClose.emit(true); - } + } else { + this.getNativeElement().classList.remove('show-menu'); + this.getBackdropElement().classList.remove('show-backdrop'); + this.ionClose.emit(true); } } - private _prevent() { - // used to prevent unwanted opening/closing after swiping open/close - // or swiping open the menu while pressing down on the MenuToggle - this._preventTime = Date.now() + 20; - } - - private _isPrevented() { - return this._preventTime > Date.now(); - } - /** * @private */ @@ -564,6 +555,9 @@ export class Menu { .map(m => m.enabled = false); } + // TODO + // what happens if menu is disabled while swipping? + return this; } @@ -572,6 +566,8 @@ export class Menu { */ swipeEnable(shouldEnable: boolean): Menu { this.swipeEnabled = shouldEnable; + // TODO + // what happens if menu swipe is disabled while swipping? return this; } @@ -619,7 +615,11 @@ export class Menu { this._cntGesture && this._cntGesture.destroy(); this._type && this._type.destroy(); this._resizeUnreg && this._resizeUnreg(); + + this._cntGesture = null; + this._type = null; this._cntEle = null; + this._resizeUnreg = null; } } diff --git a/src/gestures/slide-gesture.ts b/src/gestures/slide-gesture.ts index 29bd411a907..7a88961ee16 100644 --- a/src/gestures/slide-gesture.ts +++ b/src/gestures/slide-gesture.ts @@ -33,23 +33,32 @@ export class SlideGesture extends PanGesture { } onDragStart(ev: any) { - this.slide = {}; - this.onSlideBeforeStart(this.slide, ev); + this.onSlideBeforeStart(ev); + let coord = pointerCoord(ev); + let pos = coord[this.direction]; + this.slide = { + min: 0, + max: 0, + pointerStartPos: pos, + pos: pos, + timestamp: Date.now(), + elementStartPos: 0, + started: true, + delta: 0, + distance: 0, + velocity: 0, + }; let {min, max} = this.getSlideBoundaries(this.slide, ev); - let coord = pointerCoord(ev); this.slide.min = min; this.slide.max = max; this.slide.elementStartPos = this.getElementStartPos(this.slide, ev); - this.slide.pos = this.slide.pointerStartPos = coord[this.direction]; - this.slide.timestamp = Date.now(); - this.slide.started = true; - this.slide.velocity = 0; + this.onSlideStart(this.slide, ev); } onDragMove(ev: any) { - let slide = this.slide; + let slide: SlideData = this.slide; let coord = pointerCoord(ev); let newPos = coord[this.direction]; let newTimestamp = Date.now(); @@ -74,7 +83,7 @@ export class SlideGesture extends PanGesture { this.slide = null; } - onSlideBeforeStart(slide?: SlideData, ev?: any): void {} + onSlideBeforeStart(ev?: any): void {} onSlideStart(slide?: SlideData, ev?: any): void {} onSlide(slide?: SlideData, ev?: any): void {} onSlideEnd(slide?: SlideData, ev?: any): void {} @@ -84,14 +93,14 @@ export class SlideGesture extends PanGesture { * @private */ export interface SlideData { - min?: number; - max?: number; - distance?: number; - delta?: number; - started?: boolean; - pos?: any; - timestamp?: number; - pointerStartPos?: number; - elementStartPos?: number; - velocity?: number; + min: number; + max: number; + distance: number; + delta: number; + started: boolean; + pos: any; + timestamp: number; + pointerStartPos: number; + elementStartPos: number; + velocity: number; } diff --git a/src/navigation/swipe-back.ts b/src/navigation/swipe-back.ts index ffd4f0a27dc..9fb989dee6a 100644 --- a/src/navigation/swipe-back.ts +++ b/src/navigation/swipe-back.ts @@ -33,7 +33,7 @@ export class SwipeBackGesture extends SlideEdgeGesture { } - onSlideBeforeStart(slideData: SlideData, ev: any) { + onSlideBeforeStart(ev: any) { console.debug('swipeBack, onSlideBeforeStart', ev.type); this._nav.swipeBackStart(); }