Skip to content

Commit c4b9212

Browse files
authored
perf(sheet): fixing performance regression on modal sheets when expandToScroll is false (#30267)
Issue number: internal --------- <!-- 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. --> Currently, when a sheet is moved while `expandToScroll` is disabled, the DOM is queried excessively causing performance degradation. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> We now cache the targeted element in `onStart` and refer to it in `onMove` and `onEnd`, preventing over-querying the DOM ## 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. --> This regression was introduced in #30257 and quickly highlighted by a member of the community
1 parent 2018a04 commit c4b9212

File tree

1 file changed

+18
-15
lines changed
  • core/src/components/modal/gestures

1 file changed

+18
-15
lines changed

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
import { isIonContent, findClosestIonContent } from '@utils/content';
1+
import { findClosestIonContent, isIonContent } from '@utils/content';
22
import { createGesture } from '@utils/gesture';
3-
import { clamp, raf, getElementRoot } from '@utils/helpers';
3+
import { clamp, getElementRoot, raf } from '@utils/helpers';
44
import { FOCUS_TRAP_DISABLE_CLASS } from '@utils/overlays';
55

66
import type { Animation } from '../../../interface';
@@ -83,6 +83,7 @@ export const createSheetGesture = (
8383
let currentBreakpoint = initialBreakpoint;
8484
let offset = 0;
8585
let canDismissBlocksGesture = false;
86+
let cachedScrollEl: HTMLElement | null = null;
8687
const canDismissMaxStep = 0.95;
8788
const maxBreakpoint = breakpoints[breakpoints.length - 1];
8889
const minBreakpoint = breakpoints[0];
@@ -233,6 +234,17 @@ export const createSheetGesture = (
233234
*/
234235
canDismissBlocksGesture = baseEl.canDismiss !== undefined && baseEl.canDismiss !== true && minBreakpoint === 0;
235236

237+
/**
238+
* Cache the scroll element reference when the gesture starts,
239+
* this allows us to avoid querying the DOM for the target in onMove,
240+
* which would impact performance significantly.
241+
*/
242+
if (!expandToScroll) {
243+
const targetEl = findClosestIonContent(detail.event.target! as HTMLElement);
244+
cachedScrollEl =
245+
targetEl && isIonContent(targetEl) ? getElementRoot(targetEl).querySelector('.inner-scroll') : targetEl;
246+
}
247+
236248
/**
237249
* If expandToScroll is disabled, we need to swap
238250
* the footer visibility to the original, so if the modal
@@ -267,13 +279,8 @@ export const createSheetGesture = (
267279
* If `expandToScroll` is disabled, and an upwards swipe gesture is done within
268280
* the scrollable content, we should not allow the swipe gesture to continue.
269281
*/
270-
if (!expandToScroll && detail.deltaY <= 0) {
271-
const contentEl = findClosestIonContent(detail.event.target! as HTMLElement);
272-
const scrollEl =
273-
contentEl && isIonContent(contentEl) ? getElementRoot(contentEl).querySelector('.inner-scroll') : contentEl;
274-
if (scrollEl) {
275-
return;
276-
}
282+
if (!expandToScroll && detail.deltaY <= 0 && cachedScrollEl) {
283+
return;
277284
}
278285

279286
/**
@@ -334,12 +341,8 @@ export const createSheetGesture = (
334341
* function to be called if the user is trying to swipe content upwards and the content
335342
* is not scrolled to the top.
336343
*/
337-
if (!expandToScroll && detail.deltaY <= 0 && findClosestIonContent(detail.event.target! as HTMLElement)) {
338-
const contentEl = findClosestIonContent(detail.event.target! as HTMLElement)!;
339-
const scrollEl = isIonContent(contentEl) ? getElementRoot(contentEl).querySelector('.inner-scroll') : contentEl;
340-
if (scrollEl!.scrollTop > 0) {
341-
return;
342-
}
344+
if (!expandToScroll && detail.deltaY <= 0 && cachedScrollEl && cachedScrollEl.scrollTop > 0) {
345+
return;
343346
}
344347

345348
/**

0 commit comments

Comments
 (0)