ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.02k stars 13.51k forks source link

bug: sheet should allow for short but quick swipes to move to between breakpoints #24296

Closed RaphaelFelten closed 2 years ago

RaphaelFelten commented 2 years ago

Prerequisites

Describe the Feature Request

I would like to suggest a property on the modal that lets you control the swipe "sensitivity" (or speed, I don't really know how to describe this).

Describe the Use Case

As far as I can see, the modal snaps to the next breakpoint if you've swiped more than half of the distance to the next breakpoint (right?). That makes it kind of hard to close the modal by swiping down. Escpecially when swiping quickly, I rarely reach the distance necessary to close the modal (or go to the next breakpoint).

Describe Preferred Solution

I have two proposals:

1. A new property on the modal that lets you control at what speed the modal should automatically go to the next breakpoint and ignore the "half the distance" logic.

  1. A new boolean property that lets you control whether the "half the distance" logic is applied or if the modal should automatically go to the next breakpoint based on the swipe direction (speed is ignored, just the direction matters).

Describe Alternatives

No response

Related Code

No response

Additional Information

No response

liamdebeasi commented 2 years ago

Thanks! Could you provide a video of this issue happening? I think it might be confusing to expose a property which controls snapping sensitivity, but I am open to making adjustments internally to make the snapping behavior easier to interact with.

RaphaelFelten commented 2 years ago

https://user-images.githubusercontent.com/22978583/144285529-56918191-021b-4279-b221-71bdeac35780.mp4

Here's a video. As you can see, when swiping quickly I sort of expect the modal to close but I usually don't reach half the distance.

Maybe this makes it clearer what I mean 🙂

liamdebeasi commented 2 years ago

Thanks, this is helpful. Do you have a minimal code reproduction we can try too?

RaphaelFelten commented 2 years ago

You can create a blank ionic app and add a modal with the initalBreakpoint at 0.8 and breakpoints at [0, 0.8, 1]. This should replicate what I have in my application.

fluid-design-io commented 2 years ago

I had experience with useGesture drag function from "@use-gesture/react" where I can add custom logic to detect the "swiper power" using swipe distance (y) and swipe velocity (vy), here's the sample function:

const swipeConfidenceThreshold = 2000;
const swipePower = (offset: number, velocity: number) => {
    return offset * Math.sqrt(offset) * velocity;
  };
// To implement the function
useGesture(
    {
        onDragEnd: ({ velocity: [vx, vy], offset: [x, y], cancel, ...rest }) => {
          if(swipePower(y,vy) > swipeConfidenceThreshold){
          dismiss() // Dismiss the modal if user swipe quick enough
          }
        }
});

Hope this makes sense?

RaphaelFelten commented 2 years ago

I had experience with useGesture drag function from "@use-gesture/react" where I can add custom logic to detect the "swiper power" using swipe distance (y) and swipe velocity (vy), here's the sample function:

const swipeConfidenceThreshold = 2000;
const swipePower = (offset: number, velocity: number) => {
    return offset * Math.sqrt(offset) * velocity;
  };
// To implement the function
useGesture(
    {
        onDragEnd: ({ velocity: [vx, vy], offset: [x, y], cancel, ...rest }) => {
          if(swipePower(y,vy) > swipeConfidenceThreshold){
          dismiss() // Dismiss the modal if user swipe quick enough
          }
        }
});

Hope this makes sense?

I've tried something similar with the GestureController, but the onMove method does not fire when dragging the modal since there's already a gesture defined on the element. Unfortunately this doesn't work for me.

const swipeEl = this.header.el.parentElement;
const gesture = this.gestureController.create({
  el: swipeEl,
  direction: 'y',
  gestureName: 'swipe-to-close',
  onMove: (detail: GestureDetail) => {
    console.log(detail); // This never fires
  },
});
gesture.enable();
liamdebeasi commented 2 years ago

I think I am a bit confused as to what the root issue is here. In the video in https://github.com/ionic-team/ionic-framework/issues/24296#issue-1068619792 it looks like you have a sheet modal with breakpoints [0, 1] and are trying to swipe from open to close.

The code snippet provided in https://github.com/ionic-team/ionic-framework/issues/24296#issuecomment-983939143 adds a new 0.8 breakpoint which changes the scenario.

Given breakpoints of [0, 1] I can see the argument for tweaking the velocity threshold in our sheet modal to allow for small but quick swipes to dismiss the modal. Swiping less than 50% of the way down but doing it quickly would let you move to the next breakpoint, in this case 0. This behavior is more aligned with how our card modal behaves.

However, when we use the [0, 0.8, 1] breakpoints the situation changes and the question becomes "Should we allow users to skip over breakpoint 0.8 and go directly to breakpoint 0?". To me this does not seem correct as skipping over breakpoints defeats the purpose of having breakpoints in the first place. Additionally, doing this does not align with how native iOS sheets behave.

Could you provide some clarification as to which scenario you are running into?

RaphaelFelten commented 2 years ago

I have the breakpoints at [0, 0.8, 1] to have the modal appear just under the header (the actual value is calculated). It looks like this:

const windowHeight = window.innerHeight;
const headerRect = this.header.el.getBoundingClientRect();
const percFromTop = 1 - (headerRect.top + headerRect.height - 5) / windowHeight;
const modal = await this.modalController.create({
  component: ActionsComponent,
  componentProps: { article: articleObj },
  showBackdrop: false,
  cssClass: 'mp-actions-modal',
  breakpoints: [0, percFromTop, 1],
  initialBreakpoint: percFromTop,
});
await modal.present();

The breakpoint at 1 is just there to let the user swipe the modal completely up if they want to (or if some content is hidden on really small devices).

I get your point about skipping breakpoints. That wouldn't make much sense. However, as you pointed out, it would make sense to tweak the velocity when there are just 2 breakpoints.

liamdebeasi commented 2 years ago

Oh I see, so the 0.8 breakpoint is really to make the modal not cover the full height of the page? In that case I think it would make more sense to use the --height CSS Variable to customize how tall the sheet can extend. The breakpoints are calculated relative to the height of the modal, so you could change the --height and stick with breakpoints of [0, 1].

Here is an example: https://codepen.io/liamdebeasi/pen/LYzpYpP

You would still get the swipe velocity issue, but you wouldn't need to add the 0.8 breakpoint anymore. Can you give that a try and let me know if it helps? I think it makes sense to customize the swipe velocity to allow for short but quick swipes.

RaphaelFelten commented 2 years ago

Thanks for the tip, that worked great !

snimavat commented 2 years ago

Quick but short swipe should be able to close it (when i have 0, 1 two breakpoints - - thts what I find user friendly, thts how popular apps eg FB does it -- It takes me considerable efforts and two, three tries before it would close.

liamdebeasi commented 2 years ago

Can everyone try the following dev build and let me know if it resolves the issue?

6.1.8-dev.11653682454.137427c0

Note: This build applies to any Ionic package (@ionic/core, @ionic/angular, etc).

lincolnthree commented 2 years ago

@liamdebeasi This appears to resolve the issue for our use case where the breakpoints are [0,1]!

lincolnthree commented 2 years ago

I do notice however that it's not possible to "intercept" the "snap-back" when you let go after a partial swipe. Resulting in a bit of a waiting period before the element can be grabbed to try again if you don't get it the first time.

This can lead to some frustrating attempts:

https://user-images.githubusercontent.com/362329/170785889-d9cc078e-baba-4dae-a7aa-9c3e6fd6cad3.mov

liamdebeasi commented 2 years ago

Does that happen only on the dev build? If it happens in the latest stable version of Ionic too then it's probably this: https://github.com/ionic-team/ionic-framework/issues/24583#issuecomment-1020197730

lincolnthree commented 2 years ago

Hmm. No I guess it does that in the current release build as well. This probably isn't a new thing.

Is this the same momentum calculation as swipeToClose?

It felt a little bit different, which is why I mentioned the above, but it could all be in my head.

liamdebeasi commented 2 years ago

Yes, it is the same swipe sensitivity as the card modal.

Card modal: https://github.com/ionic-team/ionic-framework/blob/FW-389/core/src/components/modal/gestures/swipe-to-close.ts#L222 Proposed sheet modal change: https://github.com/ionic-team/ionic-framework/blob/FW-389/core/src/components/modal/gestures/sheet.ts#L257

lincolnthree commented 2 years ago

Fantastic. Then I'd say it functioned as expected and definitely fills the last gap in our issue with swipeToClose being removed. I didn't test extensively as I had to step out. (Actually stepped back in for a moment.)

Out of curiosity, is that snap back grab behavior I mentioned something you'd want to consider as an issue? Not high priority for me at all. Definitely icing.

liamdebeasi commented 2 years ago

Yes, we consider that an issue and would like to fix it. #24583 should really be split out into separate issues, but for now we are tracking the separate tickets internally.


I found an issue with the dev build. If you have breakpoints [0, 0.5, 1]. You cannot explicitly swipe from 1 to 0 to dismiss the modal as it always forces you to 0.5. I have a fix in progress and will provide another dev build when I can.

lincolnthree commented 2 years ago

Just to clarify, "yes I should open an issue?"

I don't want to create more spam than I already have, lol. (I'll have to wait until I'm back in the office).

RaphaelFelten commented 2 years ago

Great, this works as expected. Thanks!

liamdebeasi commented 2 years ago

Just to clarify, "yes I should open an issue?"

You don't need to create a new issue. We are already aware of the issue and are tracking tickets for this internally.

lincolnthree commented 2 years ago

@liamdebeasi Hope you had a good weekend. If you get a spare moment and it's not too much to ask, could you put out a quick (if that's possible) 6.1.9 branch dev build that includes the recently merged https://github.com/ionic-team/ionic-framework/issues/25353 ? I'd love to test these changes with Angular 14 ~ Thanks so much!

liamdebeasi commented 2 years ago

I won't have time to loop back to this until next week at the earliest. Once I fix the issue in https://github.com/ionic-team/ionic-framework/issues/24296#issuecomment-1140037862, I'll make a new dev build with the latest main changes.

lincolnthree commented 2 years ago

No worries, Liam. Thanks for the update.

Possibly related to https://github.com/ionic-team/ionic-framework/issues/24296#issuecomment-1140037862 - I have been doing more testing as I can, and I have seen a (very) sporadic issue where the modal sheet does not seem to actually dismiss when swiped to 0 when breakpoints are [0,1]. I have only been able to reproduce in production builds so far, and only a few times. Working on a repro - will open a new issue if I can reproduce.

Only mentioning it since you said you found an issue with breakpoints earlier. Thought it might be of use.

lincolnthree commented 2 years ago

Ok just an update on my last comment. There does not appear to be any issue here. It was a timing/async issue, and something we caused in our modal navigation framework. Apparently the timing between ionWillDismiss and ionDidDismiss is slightly different with the new sheet modal. I can't say how exactly, but we had to adjust our event stream/flow.

KMenant commented 2 years ago

Hi @liamdebeasi Any news about this issue ? Is it planned to be merged?

I just tested and indeed increasing the threshold seems to correct the problem! const threshold = (detail.deltaY + velocity * 1000) / height; https://github.com/ionic-team/ionic-framework/blob/629f1ed33f25b5bb3677fc65d46e81d84ff19f71/core/src/components/modal/gestures/sheet.ts#L237

liamdebeasi commented 2 years ago

Yes, here is a new dev build with the latest main: 6.2.6-dev.11662502742.1ac13490

liamdebeasi commented 2 years ago

Thanks for the issue. This has been resolved via https://github.com/ionic-team/ionic-framework/pull/25883, and a fix will be available in an upcoming release of Ionic Framework.

ionitron-bot[bot] commented 2 years ago

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.