Skip to content

Commit

Permalink
fix(modal): improve sheet modal scrolling and gesture behavior (#29312)
Browse files Browse the repository at this point in the history
Issue number: resolves #24583

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

See
#24583 (comment)

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- See #29260 and
#29259. This PR is a
combination of previously reviewed fixes.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->


Dev build: `7.8.3-dev.11712695191.1d7ec370`
  • Loading branch information
liamdebeasi committed Apr 10, 2024
1 parent 1388014 commit 9738228
Showing 1 changed file with 47 additions and 21 deletions.
68 changes: 47 additions & 21 deletions core/src/components/modal/gestures/sheet.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand All @@ -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
Expand Down Expand Up @@ -314,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<void>((resolve) => {
animation
.onFinish(
Expand All @@ -334,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
Expand Down

0 comments on commit 9738228

Please sign in to comment.