Skip to content

Commit 9738228

Browse files
authored
fix(modal): improve sheet modal scrolling and gesture behavior (#29312)
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`
1 parent 1388014 commit 9738228

File tree

1 file changed

+47
-21
lines changed
  • core/src/components/modal/gestures

1 file changed

+47
-21
lines changed

core/src/components/modal/gestures/sheet.ts

Lines changed: 47 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
import { isIonContent, findClosestIonContent } from '@utils/content';
12
import { createGesture } from '@utils/gesture';
2-
import { clamp, raf } from '@utils/helpers';
3+
import { clamp, raf, getElementRoot } from '@utils/helpers';
34

45
import type { Animation } from '../../../interface';
56
import type { GestureDetail } from '../../../utils/gesture';
@@ -142,22 +143,35 @@ export const createSheetGesture = (
142143

143144
const canStart = (detail: GestureDetail) => {
144145
/**
145-
* If the sheet is fully expanded and
146-
* the user is swiping on the content,
147-
* the gesture should not start to
148-
* allow for scrolling on the content.
146+
* If we are swiping on the content, swiping should only be possible if the content
147+
* is scrolled all the way to the top so that we do not interfere with scrolling.
148+
*
149+
* We cannot assume that the `ion-content` target will remain consistent between swipes.
150+
* For example, when using ion-nav within a modal it is possible to swipe, push a view,
151+
* and then swipe again. The target content will not be the same between swipes.
149152
*/
150-
const content = (detail.event.target! as HTMLElement).closest('ion-content');
153+
const contentEl = findClosestIonContent(detail.event.target! as HTMLElement);
151154
currentBreakpoint = getCurrentBreakpoint();
152155

153-
if (currentBreakpoint === 1 && content) {
154-
return false;
156+
if (currentBreakpoint === 1 && contentEl) {
157+
/**
158+
* The modal should never swipe to close on the content with a refresher.
159+
* Note 1: We cannot solve this by making this gesture have a higher priority than
160+
* the refresher gesture as the iOS native refresh gesture uses a scroll listener in
161+
* addition to a gesture.
162+
*
163+
* Note 2: Do not use getScrollElement here because we need this to be a synchronous
164+
* operation, and getScrollElement is asynchronous.
165+
*/
166+
const scrollEl = isIonContent(contentEl) ? getElementRoot(contentEl).querySelector('.inner-scroll') : contentEl;
167+
const hasRefresherInContent = !!contentEl.querySelector('ion-refresher');
168+
return !hasRefresherInContent && scrollEl!.scrollTop === 0;
155169
}
156170

157171
return true;
158172
};
159173

160-
const onStart = () => {
174+
const onStart = (detail: GestureDetail) => {
161175
/**
162176
* If canDismiss is anything other than `true`
163177
* then users should be able to swipe down
@@ -173,11 +187,10 @@ export const createSheetGesture = (
173187
canDismissBlocksGesture = baseEl.canDismiss !== undefined && baseEl.canDismiss !== true && minBreakpoint === 0;
174188

175189
/**
176-
* If swiping on the content
177-
* we should disable scrolling otherwise
178-
* the sheet will expand and the content will scroll.
190+
* If we are pulling down, then it is possible we are pulling on the content.
191+
* We do not want scrolling to happen at the same time as the gesture.
179192
*/
180-
if (contentEl) {
193+
if (detail.deltaY > 0 && contentEl) {
181194
contentEl.scrollY = false;
182195
}
183196

@@ -193,6 +206,16 @@ export const createSheetGesture = (
193206
};
194207

195208
const onMove = (detail: GestureDetail) => {
209+
/**
210+
* If we are pulling down, then it is possible we are pulling on the content.
211+
* We do not want scrolling to happen at the same time as the gesture.
212+
* This accounts for when the user scrolls down, scrolls all the way up, and then
213+
* pulls down again such that the modal should start to move.
214+
*/
215+
if (detail.deltaY > 0 && contentEl) {
216+
contentEl.scrollY = false;
217+
}
218+
196219
/**
197220
* Given the change in gesture position on the Y axis,
198221
* compute where the offset of the animation should be
@@ -314,6 +337,17 @@ export const createSheetGesture = (
314337
onDismiss();
315338
}
316339

340+
/**
341+
* If the sheet is going to be fully expanded then we should enable
342+
* scrolling immediately. The sheet modal animation takes ~500ms to finish
343+
* so if we wait until then there is a visible delay for when scrolling is
344+
* re-enabled. Native iOS allows for scrolling on the sheet modal as soon
345+
* as the gesture is released, so we align with that.
346+
*/
347+
if (contentEl && snapToBreakpoint === breakpoints[breakpoints.length - 1]) {
348+
contentEl.scrollY = true;
349+
}
350+
317351
return new Promise<void>((resolve) => {
318352
animation
319353
.onFinish(
@@ -334,14 +368,6 @@ export const createSheetGesture = (
334368
currentBreakpoint = snapToBreakpoint;
335369
onBreakpointChange(currentBreakpoint);
336370

337-
/**
338-
* If the sheet is fully expanded, we can safely
339-
* enable scrolling again.
340-
*/
341-
if (contentEl && currentBreakpoint === breakpoints[breakpoints.length - 1]) {
342-
contentEl.scrollY = true;
343-
}
344-
345371
/**
346372
* Backdrop should become enabled
347373
* after the backdropBreakpoint value

0 commit comments

Comments
 (0)