Skip to content

bug: scrolling issues with bottom sheet modal #24583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
3 tasks done
davideas opened this issue Jan 16, 2022 · 17 comments · Fixed by #29312
Closed
3 tasks done

bug: scrolling issues with bottom sheet modal #24583

davideas opened this issue Jan 16, 2022 · 17 comments · Fixed by #29312
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@davideas
Copy link

davideas commented Jan 16, 2022

Prerequisites

Describe the Feature Request

Possibility to scroll content if this exceeds screen size only when bottom sheet is fully expanded and touched the max breakpoint.
Then, when finishing to scroll the content down and there's nothng to scroll anymore, the bottom sheet should be dragged again at lower breakpoint.

Describe the Use Case

Having longer content in bottom sheet, content that exceeds the screen height is not visible because container not scrollable.
It's even a real problem for smaller screens.

Describe Preferred Solution

No response

Describe Alternatives

No response

Related Code

No response

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jan 16, 2022
@davideas davideas changed the title feat: Bottom Sheet with overeflow when reaching max breakpoint feat: Bottom Sheet with overflow when reaching max breakpoint Jan 16, 2022
@averyjohnston
Copy link
Contributor

Thanks for the issue. Could you replicate your use case in a starter app and post the link here? This sounds like it may just be a bug, so I want to make sure I'm following.

@averyjohnston averyjohnston added the ionitron: needs reproduction a code reproduction is needed from the issue author label Jan 19, 2022
@ionitron-bot
Copy link

ionitron-bot bot commented Jan 19, 2022

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Jan 19, 2022
@davideas
Copy link
Author

davideas commented Jan 20, 2022

I created a quick sample project here: https://ionic-angular-action-sheet-g1wwzx.stackblitz.io
But it should be used in mobile view with any browser not desktop view! In desktop view, scrolling works, it is usable nicely with mouse wheel too, but the problem of course is on mobile view.
There, the touch creates issues when content is longer than the screen.

I saw a css property on <bottom-sheet-modal> element in ion-page class: overflow: hidden;
Well even modifying it or adding overflow to an internal div of the bottom sheet when touching the component it drag and scroll at same moment. That's why I suggest to activate overflow-y: auto only when max breakpont is reached.
When at lower breakpoints the touch should only drag the bottom sheet up and down without interferring with the scrolling of the content.

Could you please verify how we can scroll this content?

@averyjohnston
Copy link
Contributor

Thanks! I see what you mean now. After investigation, it looks like the behavior you want exists on ion-content -- once the sheet modal is at its max breakpoint, any ion-content components inside get the scroll-y class, allowing scrolling. Could you try replacing the div.modal-content with ion-content and let me know if that looks how you would expect?

Either way, our documentation around this should be improved, since there isn't any example HTML for ModalPage in the usage examples (https://ionicframework.com/docs/api/modal#usage).

@averyjohnston averyjohnston removed their assignment Jan 21, 2022
@averyjohnston averyjohnston added the needs: reply the issue needs a response from the user label Jan 21, 2022
@ionitron-bot ionitron-bot bot removed the triage label Jan 21, 2022
@matthewgoodman13
Copy link

matthewgoodman13 commented Jan 21, 2022

I'd like to chime in here! Your fix definitely does help, that is wrapping the Modal's content inside a IonContent. This allows for cards to stick to the top of the modal until its fully opened and allows for proper scrolling.

Did notice a couple things though.
1) The time delay from once the modal is fully opened and when the content is able to be scrolled is noticeable (can be seen when the scrollbars become visible). Its about half a second. This does not allow for a smooth scrolling experience.
2) Once this scrolling enables - it becomes much harder to actually swipe to close the modal. If the modal's content is already at the top, the next "down" gesture should close the modal.

@davideas let me know if you can confirm the same experience.

Note that my experience have been with using Ionic with ReactJS. No CSS properties applied.

Code Snippet:

const MyBottomSheetModal = ({ showModal, setShowModal }) => {
  return (
    <IonModal
      isOpen={showModal}
      initialBreakpoint={0.5}
      breakpoints={[0, 0.5, 1]}
      onDidDismiss={() => setShowModal(false)}
      >
      <IonContent>
      {[...Array(10)].map((x, i) => (
        <IonCard key={i}>
          <IonCardHeader><IonCardTitle>Card Title</IonCardTitle></IonCardHeader>
          <IonCardContent><p>This is a card.</p></IonCardContent>
        </IonCard>
      )  )}
      </IonContent>
  </IonModal>
  );
}

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jan 21, 2022
@davideas
Copy link
Author

davideas commented Jan 22, 2022

Could you try replacing the div.modal-content with ion-content and let me know if that looks how you would expect?

Yes thanks @amandaesmith3 ! now at least is scrollable, well this wasn't documented :-)

Did notice a couple things though. 1) The time delay from once the modal is fully opened and when the content is able to be scrolled is noticeable (can be seen when the scrollbars become visible). Its about half a second. This does not allow for a smooth scrolling experience. 2) Once this scrolling enables - it becomes much harder to actually swipe to close the modal. If the modal's content is already at the top, the next "down" gesture should close the modal.

  • Unfortunately, I confirm both points from @matthewgoodman13.
    @amandaesmith3 Can we improve there? When maxed, it can be dragged down again only by the handle. Dragging from the center of the sheet is doesn't work anymore.
    EDIT: Dragging is not working anymore even if content fits the screen.

  • I also add another point I mentioned in my initial message, the "max breakpoint"! If the max isn't 1 than the dragging should stop at the max breakpoint we set, for example if max is 0.75 then the sheet should not move up anymore so that the content can scroll too, currently it allows the move and then snap down again, it happened the content scrolls too having the ui a bit broken (to reproduce slowly drag up).
    I modified the project to show the issue with max at 0.75.

@matthewgoodman13
Copy link

  • I also add another point I mentioned in my initial message, the "max breakpoint"! If the max isn't 1 than the dragging should stop at the max breakpoint we set, for example if max is 0.75 then the sheet should not move up anymore so that the content can scroll too, currently it allows the move and then snap down again, it happened the content scrolls too having the ui a bit broken (to reproduce slowly drag up).

Getting this as well. I also do find it is important, especially if you want absolute positioned buttons at the bottom of a 75% modal. If the max breakpoint is at 75%, scrolling up creates extra "whitespace" and shouldn't be allowed.

@matthewgoodman13
Copy link

matthewgoodman13 commented Jan 23, 2022

  • I also add another point I mentioned in my initial message, the "max breakpoint"! If the max isn't 1 than the dragging should stop at the max breakpoint we set, for example if max is 0.75 then the sheet should not move up anymore so that the content can scroll too, currently it allows the move and then snap down again, it happened the content scrolls too having the ui a bit broken (to reproduce slowly drag up).

Getting this as well. I also do find it is important, especially if you want absolute positioned buttons at the bottom of a 75% modal. If the max breakpoint is at 75%, scrolling up creates extra "whitespace" and shouldn't be allowed.

I found a fix for the height thing. You can set modal height to 75vh, then use a max breakpoint of 1. Modal will not expand above this threshold. Gesture to close as discussed still needs to be fixed along with the "delay"

@averyjohnston
Copy link
Contributor

Thanks for the extra details, folks. I'm able to replicate all issues mentioned, and in core, meaning they likely happen in all frameworks. To summarize:

  1. Once the modal is fully opened, there is a noticeable delay before content becomes scrollable. (screencast)
  2. Once you can scroll, dragging down on anywhere but the handle when the content is scrolled to the top doesn't do anything, when it should move the modal. (screencast)
  3. The sheet modal usage examples need updating with example HTML for the modal, and a note about using ion-content to ensure things are scrollable.
  4. When the max breakpoint is not 1, and the modal is fully opened, it can still be dragged up higher before snapping back down. (Workaround mentioned above: set --height: 75vh on the ion-modal, or whatever height you need, and use a max breakpoint of 1.) (screencast)
  5. For the above scenario, scrolling on mobile is very difficult, since most of your gestures drag the modal around instead. This is a slightly separate issue from 4 as it happens in both directions -- if you're scrolled down, and drag down, the modal will usually move when this should scroll the content up instead. (screencast)
  6. I found this one while investigating: for modals with a max breakpoint of less than 1, the bottom of the content is cut off and can't be scrolled to. (screencast) The --height: 75vh workaround works here too.

@averyjohnston averyjohnston changed the title feat: Bottom Sheet with overflow when reaching max breakpoint bug: scrolling issues with bottom sheet modal Jan 24, 2022
@averyjohnston averyjohnston added package: core @ionic/core package type: bug a confirmed bug report labels Jan 24, 2022
@matthewgoodman13
Copy link

@amandaesmith3 Great article! Has this upgrade resolved all the found modal bugs?

@averyjohnston
Copy link
Contributor

@matthewgoodman13 We're still working on these issues; you can see the full 6.1.0 changelog here: https://github.com/ionic-team/ionic-framework/releases/tag/v6.1.0

@jakubkoje
Copy link

No news I guess? Maybe the steps above should be at least mentioned in the documentation because without them, the sheet modal is unusable.

@Captainfl4me
Copy link

Has anyone found a fix for the second report issue ? This is definitly causing accessibility issue on ios as when fully extand the bar is near the phone status bar so you often open the notification center. I was thinking handling the problem myself by using custom gesture to move the modal when user reach top of the scroll but I don't really know how I can move the modal directly from Angular ?

@durgamalleswariyeruva
Copy link

durgamalleswariyeruva commented Dec 19, 2023

@amandaejohnston , @liamdebeasi , As per the comment on jan 24,
When the max breakpoint is not 1, and the modal is fully opened, it can still be dragged up higher before snapping back down. (Workaround mentioned above: set --height: 75vh on the ion-modal, or whatever height you need, and use a max breakpoint of 1. ---- I am not able to smoothly drag down without touching the handler.(If i can drag by touching the handle(top notch) initially then only sheetmodal is moving to previous breakpoint, otherwise i am not able to drag down especially in ios devices.
currently using below properties inside sheetmodal
[backdropDismiss]="false" [backdropBreakpoint]="1" [isOpen]="true"
[initialBreakpoint]="0.25" [breakpoints]="[0.25,1]" [swipeToClose]="false"

Any help is appreciated. currently using ionic version 6.18.0 and angular 12 version.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Apr 3, 2024

Hi everyone,

I have an update on the status of this thread. There are several issues noted here, so I'll give updates per-issue:

  1. There is a delay when re-enabling scrolling.
    This is being addressed in a PR on this repo.

  2. Dragging down on a fully open modal does not do anything unless users swipe on the header.
    This is being a addressed in a PR on this repo.

  3. Sheet modal documentation should note that IonContent is necessary.
    This is being addressed in a PR on the Ionic Docs repo.

  4. Sheet modal can be dragged above max breakpoint before snapping back.
    This is being tracked in a separate issue. The main here is that once you hit the max breakpoint, you should not be able to drag up anymore. There should be a rubber band effect instead.

  5. Scrolling up on a sheet modal with a max breakpoint of < 1 is difficult.
    The root cause here is the same as 4 due to our lack of support for the "rubber band" effect when swiping up past the max breakpoint. We need additional logic to detect when a user swipes beyond the max breakpoint. This will be tracked in the same thread as point 4 above.

  6. Max breakpoint of <1 causes part of the modal to be inaccessible offscreen.
    Ionic is working as intended here. The max breakpoint essentially says "This is the max percentage of the modal that can be shown in the viewport". For example, if you have a max breakpoint of 0.9 that is the same as saying "At most 90% of the modal can be shown in the viewport". As a result, 10% will always be inaccessible outside of the viewport. Developers probably should not make the max breakpoint anything less than 1. It seems like the most common use case for this is to have a sheet modal that does not take up the full screen (but still has all of its content visible). In that case, developers should change the height of the modal rather than make the max breakpoint less than 1. I can look into some better documentation for this. edit: This is being addressed in docs(modal): clarify changing height of sheet modal ionic-docs#3589.


Here is a dev build that should fix issues 1 and 2 if anyone is interested in testing: 7.8.3-dev.11712114476.152fc43d

liamdebeasi added a commit that referenced this issue Apr 9, 2024
Issue number: Part of
#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. -->

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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- 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

<!--
  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/.github/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.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 |
| - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/910755c8-808e-4e0b-85d2-9a56a3b127c4"></video>
|

**Sheet Behavior**

| `main` | branch |
| - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/eb0b740f-b644-4602-93f9-8c634458239b"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/3844983e-ebbc-43f3-b5c2-20aad738b201"></video>
|
liamdebeasi added a commit that referenced this issue Apr 9, 2024
Issue number: Part of
#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. -->

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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- 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

<!--
  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/.github/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.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 |
| - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/175c5721-b694-45bf-9acf-044214d979c9"></video>
|

**Sheet Behavior**

| `main` | branch |
| - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/a6d5fffd-57a5-4d43-9856-dc28dced5417"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/797395e9-f455-4a0b-87a3-b2f72c50afab"></video>
|
github-merge-queue bot pushed a commit that referenced this issue Apr 10, 2024
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`
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #29312, and a fix will be available in an upcoming release of Ionic Framework. Please see #24583 (comment) for updates on related work.

Copy link

ionitron-bot bot commented May 10, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
7 participants