fix(modal): consider scrollable content while dragging when expandToScroll is false

[kumibrr]

Issue number: internal

---

What is the current behavior?

Changes introduced by #30235 caused two major issues:

• Animations were not being played when increasing breakpoints.
• When the scrollable content was at the top, and a big scroll to the bottom of the list was made, the modal would jump to a higher breakpoint.

What is the new behavior?

• When expandToScroll is false, the swipe gesture is allowed unless it's a pull down within scrollable content.
• When expandToScroll is false, we cancel the moveSheetToBreakpoint when a scroll to top is being done within the scrollable content

Does this introduce a breaking change?

• Yes
• No

Other information

Before	After
Screen.Recording.2025-03-15.at.04.53.20.mov	Screen.Recording.2025-03-15.at.04.50.58.mov

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ionic-framework	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 18, 2025 9:06am

[brandyscarney]

Thank you for the PR! We have this on our list to review soon. Please ignore the failing screenshot tests. We are looking into some issues with a recent Playwright update.

[ShaneK]

Solid, elegant solution! Great work, thanks for the PR!

[ShaneK]

@kumibrr if you could pull main into your branch and push again, the workflow should work this time!

[ShaneK]

This has been merged and will deploy with the next patch version, thank you for your PR!

[kumibrr]

@ShaneK Thank you guys for moving this so quickly and thank you for the feedback!

❤️

[aeharding]

Hi! I am concerned about the logic in onMove causing performance regressions for existing Ionic apps. onMove is caused a lot and onMove was designed to be optimized and not call the DOM. This is a high touchpoint part of the code for every Ionic app that uses swipe (edit: with expandToScroll).

Is it possible to move DOM query logic outside of onMove? Or could we please be able to disable this logic for performance optimized apps? Thanks.

[kumibrr]

Hi! I am concerned about the logic in onMove causing performance regressions for existing Ionic apps. onMove is caused a lot and onMove was designed to be optimized and not call the DOM. This is a high touchpoint part of the code for every Ionic app that uses swipe (edit: with expandToScroll).

Is it possible to move DOM query logic outside of onMove? Or could we please be able to disable this logic for performance optimized apps? Thanks.

Hi! Thank you for sharing your thoughts, I had the same concerns while making this PR, I'll add the section of the code that worries you and will comment on it on why it should not be problematic.

const onMove = (detail: GestureDetail) => {
     if (!expandToScroll && detail.deltaY <= 0) {
       // If expandToScroll is true, it won't check anything else, I'll just skip all the code bellow and no DOM queries will be executed.
       // If expandToScroll is false, it will check if the deltaY is less or equal to zero. If it's not, same as before, it'll skip all the code bellow and no DOM queries will be executed.

       // the DOM query will only fire if the followings conditions are true: expandToScroll is false, deltaY is zero or negative
       const contentEl = findClosestIonContent(detail.event.target! as HTMLElement);
       // DOM query count is now 1.
       const scrollEl =
         contentEl && 
         // If contentEl is null, it'll skip the code bellow. and one DOM Query count would've been executed.
         isIonContent(contentEl) 
         // If contentEl is not an ionContent, it'll skip the next line.
         ? getElementRoot(contentEl).querySelector('.inner-scroll')
         // If contentEl is an ionContent, it'll query it's .inner-scroll
         // DOM query count is now 2.
         : contentEl;
       if (scrollEl) {
         return;
       }
     }

I've tested this component in a 100$ 6 year old android and performance is not highly impacted, though I understand your concerns (I have the same performance concerns in the app I'm developing).
Do you @aeharding have any suggestions?

[kumibrr]

Alright, I've refactored the check to only do a contentEl.contains and seems to work fine and does not make any DOM queries. Does anyone have any thoughts on the possible downsides of this implementation from the current?

  const onMove = (detail: GestureDetail) => {
    /**
     * If `expandToScroll` is disabled, and an upwards swipe gesture is done within
     * the scrollable content, we should not allow the swipe gesture to continue.
     */
    if (!expandToScroll && detail.deltaY <= 0 && contentEl?.contains(detail.event.target as HTMLElement)) {
      return;
    }

  const onEnd = (detail: GestureDetail) => {
    /**
     * If expandToScroll is disabled, we should not allow the moveSheetToBreakpoint
     * function to be called if the user is trying to swipe content upwards and the content
     * is not scrolled to the top.
     */
    if (!expandToScroll && detail.deltaY <= 0 && contentEl?.contains(detail.event.target as HTMLElement)) {
      return;
    }

[ShaneK]

@kumibrr I believe that solution would be a regression of this PR because, from what I can tell, it would prevent the user from dragging the sheet up from within it when expandToScroll is disabled.

[ShaneK]

I'm considering a potential alternative solution where we cache the targetted element and if it's a scrollable in onStart and reference the cached value in onMove and onEnd. I think this might be a solid compromise with only a minor potential issue where the drag target changes without triggering onStart, but I need testing to see if that will be a problem.

[ShaneK]

@kumibrr, @aeharding: after testing, I believe the caching solution should work great. I have a PR open here and I hope it'll be merged before the end of the day #30267

[ShaneK]

BTW, these changes and the related regression fix got deployed with 8.5.1 yesterday 🚀