[SwipeableDrawer] Add callback to customise touchstart ignore for swipeable drawer #30759

tech-meppem · 2022-01-24T14:11:13Z

Currently with SwipeableDrawer, if you have a button inside a swipeable edge (following the demo), then you cannot click on it due to pointer-events: none being applied.
You can easily set pointer-events: all to reallow clicking the button.
However, this means you can no longer click and drag the button to open the drawer, making a large area of the drawer's edge unswipeable.
Looking through the source, I discovered that this line was the problem:
if (disableSwipeToOpen || nativeEvent.target !== swipeAreaRef.current) {

This PR adds a callback that allows overriding of this behaviour, allowing for custom filtering of which elements / target elements can also be used to drag the drawer.

Edit:
Created issue thread: #30805

I have followed (at least) the PR section of the contributing guide.

mui-bot · 2022-01-24T14:13:54Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-30759--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against 20877e1

mnajdova · 2022-01-27T09:59:38Z

Could you please create an issue/codeasndbox illustrating what is the problem and how it can be solved. It's hard to follow based on the PR description what is the issue you are trying to solve.

tech-meppem · 2022-01-27T10:26:28Z

@mnajdova I have created an issue thread: #30805

siriwatknp · 2022-02-04T04:35:54Z

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

pull latest master from upstream to your branch
if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
2.1 revert the change on those markdown files you did
2.2 pull latest master from upstream (you should not see conflict)
2.3 make the changes again at docs/data/material/*
run yarn docs:api
- you might see the changes in docs/pages/material/api/* if your PR touches some of the components
- it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

michaldudak · 2022-02-10T20:33:20Z

The swipeAreaIgnoreTouchStartEvent prop you introduced feels very specific to this use case. I propose to consider something easier to use, such as introducing the detectSwipeWithinChildren prop (a boolean), then used as such:

if (!open) {
  if (
    disableSwipeToOpen ||
    !(
      nativeEvent.target === swipeAreaRef.current ||
      (detectSwipeWithinChildren && paperRef.current?.contains(nativeEvent.target))
    )
  ) {
    return;
  }
  // ...
}

@mnajdova, @hbjORbj - could you also take a look at this, please?

tech-meppem · 2022-02-11T10:27:38Z

The swipeAreaIgnoreTouchStartEvent prop you introduced feels very specific to this use case. I propose to consider something easier to use, such as introducing the detectSwipeWithinChildren prop (a boolean), then used as such:
if (!open) {
  if (
    disableSwipeToOpen ||
    !(
      nativeEvent.target === swipeAreaRef.current ||
      (detectSwipeWithinChildren && paperRef.current?.contains(nativeEvent.target))
    )
  ) {
    return;
  }
  // ...
}
@mnajdova, @hbjORbj - could you also take a look at this, please?

The reason I made it a callback is so you could customise which children you want to be able to swipe with, and not assume all of them.
For my use case, we want to swipe with buttons, checkboxes, tabs etc...
But not allow opening the drawer using a slider (but still be able to interact with the slider), as that feels very bad to use.

So this is the code we're using for that:

swipeAreaIgnoreTouchStartEvent={(e, swipeArea) => {
    const target = e.target as HTMLElement;
    return target.closest(".MuiSlider-root") != null;
}}

tech-meppem · 2022-02-14T10:29:41Z

Maybe making it a boolean | function might be better, so you can use true to enable detect swiping in children, but then also have the choice to pass a function for custom filtering instead?

So the default would be something like:

const swipeAreaIgnoreTouchStartEvent = typeof detectSwipeWithinChildren === "function" ? detectSwipeWithinChildren : (
    detectSwipeWithinChildren ? (e, swipeArea, paper) => !paper?.contains(nativeEvent.target) : null
);

Edit:
Also, if a prop like that is enabled, maybe it would be a good idea to make it so pointer-events: none is not applied to those children? As currently it is, and you have to override the css manually.

tech-meppem · 2022-03-21T17:23:09Z

There is a problem with this that I have discovered (although, it's a new problem, not one that breaks previous functionality).
It's a bug on Safari Mobile with click handling and events.

If you have a button in a swipeable edge of a drawer, then you can click on it with this PR's fix.
However, on Safari Mobile, it requires a double click.
This is due to the difference in the way Safari Mobile handles the click event:

Mouse events are delivered in the same order you'd expect in other web browsers illustrated in Figure 6-4. If the user taps a nonclickable element, no events are generated. If the user taps a clickable element, events arrive in this order: mouseover, mousemove, mousedown, mouseup, and click. The mouseout event occurs only if the user taps on another clickable item. Also, if the contents of the page changes on the mousemove event, no subsequent events in the sequence are sent. This behavior allows the user to tap in the new content.
From apple developer docs (link, or wayback link because their site was down for me)

The key part being the last part:

The mouseout event occurs only if the user taps on another clickable item. Also, if the contents of the page changes on the mousemove event, no subsequent events in the sequence are sent.

Because the touchstart & subsequently mousemove events causes a load of changes (like classes and stuff), it causes further mouse events to be cancelled, and not happen at all.

The way to fix this would be to prevent any changes on iOS until actually moving, however, that directly contradicts with drawer discovery.
Luckily, the times you would use this PR's feature of 'AllowSwipingInChildren', would be with a button in a swipeable edge drawer, and you would not want discovery enabled for that drawer.
Also, discovery is disabled on iOS by default.

Edit: fixed in 74d2c81

michaldudak

Could you please also add a section in the documentation explaining how to use the new prop?

@mnajdova, @hbjORbj please take a look at the implementation as you're the owners of the component.

michaldudak · 2022-05-02T09:33:02Z

packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.d.ts

+   *
+   * @default false
+   */
+  AllowSwipeInChildren?:


Use camel case in prop names

Suggested change

AllowSwipeInChildren?:

allowSwipeInChildren?:

tech-meppem · 2022-05-27T10:43:32Z

Firstly, kudos for fixing this bug. I've been struggling with it as well.

One problem I'm seeing by testing your PR is for devices that have enabled swipe gestures (disableSwipeToOpen={false}). The open and close swipe gestures bug out when both disableDiscovery and AllowSwipeInChildren are enabled.

In the specific case of enabling both these flags for your Mobile Safari fix, the swipe gesture bugs out on open and refuse to work on close. I've attached 2 videos, one of what it looks like without disableDiscovery and another of what it looks like with disableDiscovery. If this isn't immediately re-creatable, I can try to create a CodeSandbox later. Removing the disableDiscovery flag resolves this issue, but then you have to do the "double-click" workaround you mentioned above.

I would also suggest that, if there is no plan to support AllowSwipeInChildren when discovery is enabled on Mobile Safari, to update the documentation accordingly before an unsuspecting dev doesn't realize this restriction.

RPReplay_Final1653108284.MP4
RPReplay_Final1653108652.MP4

Hi there. You may want to look into the other PR I have done (#30763).
This may help with your problem, as I use both of these pr's with each other for my work.

mnajdova · 2022-05-31T10:43:16Z

Let's add tests and docs for this new behavior and do one final review :)

randuck-dev · 2022-06-25T13:41:02Z

Looking forward to seeing this feature coming to master. Anything that can be helped with to progress with that?

tech-meppem · 2022-06-27T09:43:43Z

Looking forward to seeing this feature coming to master. Anything that can be helped with to progress with that?

Sorry, I do apologise. I have very little free time to work on this. I'll see if I can do it this week.
It's also hard to make the tests, as I am not too familiar with the library used, and there seems to be some rendering issues with them, of which I have detailed previously.

tech-meppem · 2022-08-30T11:41:15Z

I apologise for how long it has taken, but I have finally had time and finished the tests for this prop.

bryan-hunter · 2022-09-06T22:16:17Z

@tech-meppem I just ran into this and needed this as well - what are the odds - 7 days ago :p

@mnajdova figured I would tag in case it fell off the radar

bryan-hunter · 2022-09-26T22:34:01Z

@hbjORbj @michaldudak checking back in on this PR to avoid maintaining a patch package for a while :p

mnajdova · 2022-09-29T11:38:53Z

Coming back to this PR after some time, as I was working on some issues related to the SwipeableDrawer. I tend to agree with Michal's feedback about adding just a boolean prop detectSwipeWithinChildren. @tech-meppem ignoring swiping the drawer when conflicting with a slider component is handled earlier in https://github.com/mui/material-ui/blob/master/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js#L440 so the code should never get to that specific line at all. Can you verify this, or maybe share a codesandbox with a scenario that doesn't work? Overall I think it would be confusing to allow people to swipe on some elements but not others, and we shouldn't encourage that kind of API in the component itself, unless there is a real use-case.

tech-meppem · 2022-09-29T12:28:08Z

Coming back to this PR after some time, as I was working on some issues related to the SwipeableDrawer. I tend to agree with Michal's feedback about adding just a boolean prop detectSwipeWithinChildren. @tech-meppem ignoring swiping the drawer when conflicting with a slider component is handled earlier in https://github.com/mui/material-ui/blob/master/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js#L440 so the code should never get to that specific line at all. Can you verify this, or maybe share a codesandbox with a scenario that doesn't work? Overall I think it would be confusing to allow people to swipe on some elements but not others, and we shouldn't encourage that kind of API in the component itself, unless there is a real use-case.

I did make it so that you can use either a boolean, or a function. The reason for this is because there is some weird interactivity with sliders & buttons, but also there's the use-case if you want a specific drag-handle, rather than everything.

There was a problem when using this patch when I originally made it as a boolean, when it came to sliders in drawers (our main use-case for this). It would move both the slider & drawer at the same time, depending on what you did first. If you started swiping horizontally, then the slider would consume the event, and the drawer wouldn't open on swiping up or down.
However, if you started on the slider thumb, then dragged upwards to open a bottom-anchored drawer, then now both the slider & drawer would be moving, and you could move in a circle pattern affecting both at the same time. So I made this patch a function instead of a boolean to specifically ignore touch events from inside a slider specifically.

I've managed to recreate the problem I encountered just editing the swipeable-drawer demo slightly (with mobile touch emulation):
https://codesandbox.io/s/heuristic-hertz-qcyfkn?file=/demo.tsx

I think it just makes more sense to allow both, especially as that's how it's currently working.

mnajdova · 2022-09-30T10:06:38Z

packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js

+    startMaybeSwiping();
+


Suggested change

startMaybeSwiping();

This looks unrelated to the changes, it can create some changes in the behavior.

That was to fix the problem listed here:
#30759 (comment)

This looks like the only blocker at this moment.

Just realised I didn't set force to true.
The call here is meant to have the param true to force it.
I'll update that, and can change the comment as suggested below if wanted to.

I've pushed the change.

We need a test for this. This component is already loaded, so if we decide to refactor in the future, we may remove this without a test.

Or even better, let's create an issue + a separate fix for it, as it doesn't seem much related to these changes.

It's here because it was only affecting interactivity with components (such as buttons) inside a swipeable edge drawer.
It's an issue that only affects the contents of the drawer, which previously, were not interactable.
I agree that it could be split out, however to test it, you would need the changes from this PR to be able to interact with the contents of the drawer & trigger the drawer simultaneously, which is where the issue is triggered.

Ok, we can then either create a follow up PR, or add the test now. Whatever works better for you :)

mnajdova · 2022-09-30T10:34:57Z

packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.d.ts

+   * Callback to determine what children of the drawer the user can use to drag open the drawer.
+   * Can be a custom function to control the behavior.
+   * Set or return true / false to allow / disallow the swipe event.


Suggested change

* Callback to determine what children of the drawer the user can use to drag open the drawer.

* Can be a custom function to control the behavior.

* Set or return true / false to allow / disallow the swipe event.

* If set to true the swipe event will open the drawer even if it starts over some of its children.

* It can be useful in scenarios where the drawer is partially visible.

* Can be further customized by a callback to determine which children the user can drag over to open the drawer (for e.g. to ignore other elements that handle the touch move events, like sliders).

I've restructured a bit to start with the "most common" usage first. Then I explained a possible scenario, and then finished with the least common use-case. cc @samuelsycamore for a final checkup on the wording.

cc @samuelsycamore pinging you again

Sorry I missed this the first time! I'm not sure exactly what is meant here:

even if it starts over some of its children

Is there another way to phrase this?

It is related to the drag event. You started dragging over some children element. Considering your comment, this would be confusing for other users of the library too :\

@michaldudak maybe you can help here to unblock the PR, the only thing left as far as I remember is the prop descriotion. Sam is on PTO, so I don't expect him to respond, and the PR has been sitting for quite some time.

I'm back! 😁 How does this sound?

Suggested change

* Callback to determine what children of the drawer the user can use to drag open the drawer.

* Can be a custom function to control the behavior.

* Set or return true / false to allow / disallow the swipe event.

* If set to true, the swipe event will open the drawer even if the user begins the swipe on one of the drawer's children.

* This can be useful in scenarios where the drawer is partially visible.

* You can customize it further with a callback that determines which children the user can drag over to open the drawer (for example, to ignore other elements that handle touch move events, like sliders).

Sam's suggestion looks good. I merged in the latest master and fixed all the conflicts. It should be good to go now.

mnajdova · 2023-01-17T13:30:07Z

Thanks a lot for the contribution & patience @tech-meppem It took us a while to get this one in. Good work 👌

tech-meppem added 2 commits January 24, 2022 12:25

Add callback to customise touchstart ignore for swipeable drawer

fae7925

Fixes for tests

26cee90

Merge branch 'mui-org:master' into swipe-ignore

251fd02

danilo-leal added the component: SwipeableDrawer The React component. label Jan 24, 2022

tech-meppem mentioned this pull request Jan 27, 2022

[SwipeableDrawer] Buttons and other interactable components prevent opening a Swipeable edge drawer. #30805

Open

2 tasks

tech-meppem added 2 commits February 8, 2022 15:33

Merge remote-tracking branch 'upstream/master' into swipe-ignore

380d88d

Update for migration PR

cdc91e7

tech-meppem added 2 commits February 25, 2022 15:12

Merge remote-tracking branch 'upstream/master' into swipe-ignore

06e42d9

Changed to be 'AllowSwipeInChildren'

36e3e10

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2022

Merge remote-tracking branch 'upstream/master' into swipe-ignore

9958424

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2022

tech-meppem added 2 commits March 8, 2022 15:41

Merge remote-tracking branch 'upstream/master' into swipe-ignore

dce8162

Merge remote-tracking branch 'upstream/master' into swipe-ignore

05cefd6

tech-meppem added 2 commits March 22, 2022 10:16

Fix iOS click bug

74d2c81

Merge remote-tracking branch 'upstream/master' into swipe-ignore

51ecfff

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 8, 2022

Merge remote-tracking branch 'upstream/master' into swipe-ignore

a1c39e5

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 19, 2022

michaldudak reviewed May 2, 2022

View reviewed changes

mnajdova requested a review from hbjORbj May 18, 2022 08:59

mnajdova assigned hbjORbj May 18, 2022

tech-meppem added 2 commits May 27, 2022 12:12

Merge remote-tracking branch 'upstream/master' into swipe-ignore

3ea297a

Change 'AllowSwipeInChildren' to 'allowSwipeInChildren'

4411ecf

mnajdova added new feature New feature or request PR: needs test The pull request can't be merged labels May 31, 2022

tech-meppem added 3 commits June 27, 2022 11:01

Merge remote-tracking branch 'upstream/master' into swipe-ignore

97e848b

Merge remote-tracking branch 'upstream/master' into swipe-ignore

214221b

Added tests for allowSwipeInChildren prop

987a7a0

tech-meppem requested a review from mnajdova as a code owner August 30, 2022 11:40

prettier fixes

1dfaf72

Merge branch 'master' into swipe-ignore

93704ac

mnajdova reviewed Sep 30, 2022

View reviewed changes

SwipeableDrawer: fix missing bool for iOS fix

fa7b76e

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2022

Merge remote-tracking branch 'upstream/master' into swipe-ignore

d48c9ee

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 28, 2022

updated comment for allowSwipeInChildren

6445cd4

mnajdova approved these changes Jan 17, 2023

View reviewed changes

Merge branch 'master' into swipe-ignore

20877e1

mnajdova merged commit 0ff044d into mui:master Jan 23, 2023

[SwipeableDrawer] Add callback to customise touchstart ignore for swipeable drawer #30759

[SwipeableDrawer] Add callback to customise touchstart ignore for swipeable drawer #30759

Conversation

tech-meppem commented Jan 24, 2022 • edited Loading

mui-bot commented Jan 24, 2022 • edited Loading

mnajdova commented Jan 27, 2022

tech-meppem commented Jan 27, 2022

siriwatknp commented Feb 4, 2022

michaldudak commented Feb 10, 2022

tech-meppem commented Feb 11, 2022 • edited Loading

tech-meppem commented Feb 14, 2022 • edited Loading

tech-meppem commented Mar 21, 2022 • edited Loading

michaldudak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech-meppem commented May 27, 2022

mnajdova commented May 31, 2022 • edited Loading

randuck-dev commented Jun 25, 2022

tech-meppem commented Jun 27, 2022 • edited Loading

tech-meppem commented Aug 30, 2022

bryan-hunter commented Sep 6, 2022

bryan-hunter commented Sep 26, 2022

mnajdova commented Sep 29, 2022

tech-meppem commented Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech-meppem Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Jan 17, 2023

tech-meppem commented Jan 24, 2022 •

edited

Loading

mui-bot commented Jan 24, 2022 •

edited

Loading

tech-meppem commented Feb 11, 2022 •

edited

Loading

tech-meppem commented Feb 14, 2022 •

edited

Loading

tech-meppem commented Mar 21, 2022 •

edited

Loading

mnajdova commented May 31, 2022 •

edited

Loading

tech-meppem commented Jun 27, 2022 •

edited

Loading

tech-meppem commented Sep 29, 2022 •

edited

Loading

tech-meppem Sep 30, 2022 •

edited

Loading