From fbfd57afc043570b63ba1ee864c8361a67ec09ea Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 9 Apr 2024 16:37:48 -0400 Subject: [PATCH 1/2] fix(modal): sheet modal can be swiped down on content (#29260) Issue number: Part of https://github.com/ionic-team/ionic-framework/issues/24583 --------- ## What is the current behavior? When a sheet modal is fully opened swiping down on the IonContent when the content is scrolled to top does not move the sheet modal down. This does not align with the card modal where doing the same actions causes the card modal to move down. ## What is the new behavior? - Swiping down on the content now moves the sheet modal down as long as the content is scrolled to the top. ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Dev build: `7.8.3-dev.11712114476.152fc43d` Reviewers: Please test this on physical iOS and Android devices. Please check the following: 1. When a modal is fully opened, ensure that users can still scroll down (i.e. swipe up on the content to scroll down). 2. When a modal is fully opened, ensure that users can swipe down on the modal to dismiss when the content is scrolled to the top. 3. Repeat step 2 but ensure that swiping down scrolls content to the top when content is NOT already scrolled to the top. **Card Behavior** | Demo | | - | | | **Sheet Behavior** | `main` | branch | | - | - | | | | --- core/src/components/modal/gestures/sheet.ts | 49 +++++++++++++++------ 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/core/src/components/modal/gestures/sheet.ts b/core/src/components/modal/gestures/sheet.ts index 0e3fca6b8bb..2307d5e19d6 100644 --- a/core/src/components/modal/gestures/sheet.ts +++ b/core/src/components/modal/gestures/sheet.ts @@ -1,5 +1,6 @@ +import { isIonContent, findClosestIonContent } from '@utils/content'; import { createGesture } from '@utils/gesture'; -import { clamp, raf } from '@utils/helpers'; +import { clamp, raf, getElementRoot } from '@utils/helpers'; import type { Animation } from '../../../interface'; import type { GestureDetail } from '../../../utils/gesture'; @@ -142,22 +143,35 @@ export const createSheetGesture = ( const canStart = (detail: GestureDetail) => { /** - * If the sheet is fully expanded and - * the user is swiping on the content, - * the gesture should not start to - * allow for scrolling on the content. + * If we are swiping on the content, swiping should only be possible if the content + * is scrolled all the way to the top so that we do not interfere with scrolling. + * + * We cannot assume that the `ion-content` target will remain consistent between swipes. + * For example, when using ion-nav within a modal it is possible to swipe, push a view, + * and then swipe again. The target content will not be the same between swipes. */ - const content = (detail.event.target! as HTMLElement).closest('ion-content'); + const contentEl = findClosestIonContent(detail.event.target! as HTMLElement); currentBreakpoint = getCurrentBreakpoint(); - if (currentBreakpoint === 1 && content) { - return false; + if (currentBreakpoint === 1 && contentEl) { + /** + * The modal should never swipe to close on the content with a refresher. + * Note 1: We cannot solve this by making this gesture have a higher priority than + * the refresher gesture as the iOS native refresh gesture uses a scroll listener in + * addition to a gesture. + * + * Note 2: Do not use getScrollElement here because we need this to be a synchronous + * operation, and getScrollElement is asynchronous. + */ + const scrollEl = isIonContent(contentEl) ? getElementRoot(contentEl).querySelector('.inner-scroll') : contentEl; + const hasRefresherInContent = !!contentEl.querySelector('ion-refresher'); + return !hasRefresherInContent && scrollEl!.scrollTop === 0; } return true; }; - const onStart = () => { + const onStart = (detail: GestureDetail) => { /** * If canDismiss is anything other than `true` * then users should be able to swipe down @@ -173,11 +187,10 @@ export const createSheetGesture = ( canDismissBlocksGesture = baseEl.canDismiss !== undefined && baseEl.canDismiss !== true && minBreakpoint === 0; /** - * If swiping on the content - * we should disable scrolling otherwise - * the sheet will expand and the content will scroll. + * If we are pulling down, then it is possible we are pulling on the content. + * We do not want scrolling to happen at the same time as the gesture. */ - if (contentEl) { + if (detail.deltaY > 0 && contentEl) { contentEl.scrollY = false; } @@ -193,6 +206,16 @@ export const createSheetGesture = ( }; const onMove = (detail: GestureDetail) => { + /** + * If we are pulling down, then it is possible we are pulling on the content. + * We do not want scrolling to happen at the same time as the gesture. + * This accounts for when the user scrolls down, scrolls all the way up, and then + * pulls down again such that the modal should start to move. + */ + if (detail.deltaY > 0 && contentEl) { + contentEl.scrollY = false; + } + /** * Given the change in gesture position on the Y axis, * compute where the offset of the animation should be From d7ec370ed60a1a029a1bbb3a9113fbc7b5cfa8ef Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 9 Apr 2024 16:37:56 -0400 Subject: [PATCH 2/2] fix(modal): remove enable scrolling delay with sheet modal (#29259) Issue number: Part of https://github.com/ionic-team/ionic-framework/issues/24583 --------- ## What is the current behavior? There is a noticeable delay between when users finish swiping and when scrolling is re-enabled. This is because the animation takes ~500ms to complete when finishing the swipe. This does not align with how we [re-enable scrolling in the card modal](https://github.com/ionic-team/ionic-framework/blob/9b3cf9fbc2c5189871b2255d2d765e78509f5027/core/src/components/modal/gestures/swipe-to-close.ts#L262-L264). In the card modal, scrolling is re-enabled as soon as the gesture is released. ## What is the new behavior? - To fix this, I aligned the behavior of the sheet modal with that of the card modal. As a result, whether or not the content is scrollable is now tied to the state of the gesture rather than the state of the animation. ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Dev build: `7.8.3-dev.11712114476.152fc43d` Reviewers: Please test this on a physical iOS and Android devices. Ensure that swiping the modal fully open does a) re-enable scrolling and b) re-enable scrolling without a 500ms delay. **Card Behavior** | Demo | | - | | | **Sheet Behavior** | `main` | branch | | - | - | | | | --- core/src/components/modal/gestures/sheet.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/core/src/components/modal/gestures/sheet.ts b/core/src/components/modal/gestures/sheet.ts index 2307d5e19d6..f61a3baaa8a 100644 --- a/core/src/components/modal/gestures/sheet.ts +++ b/core/src/components/modal/gestures/sheet.ts @@ -337,6 +337,17 @@ export const createSheetGesture = ( onDismiss(); } + /** + * If the sheet is going to be fully expanded then we should enable + * scrolling immediately. The sheet modal animation takes ~500ms to finish + * so if we wait until then there is a visible delay for when scrolling is + * re-enabled. Native iOS allows for scrolling on the sheet modal as soon + * as the gesture is released, so we align with that. + */ + if (contentEl && snapToBreakpoint === breakpoints[breakpoints.length - 1]) { + contentEl.scrollY = true; + } + return new Promise((resolve) => { animation .onFinish( @@ -357,14 +368,6 @@ export const createSheetGesture = ( currentBreakpoint = snapToBreakpoint; onBreakpointChange(currentBreakpoint); - /** - * If the sheet is fully expanded, we can safely - * enable scrolling again. - */ - if (contentEl && currentBreakpoint === breakpoints[breakpoints.length - 1]) { - contentEl.scrollY = true; - } - /** * Backdrop should become enabled * after the backdropBreakpoint value