[test] Convert SwipeableDrawer tests to testing-library #26916
Conversation
| > | ||
| <div>Drawer2</div> | ||
| </SwipeableDrawer> | ||
| </div>, | ||
| ); | ||
|
|
||
| // use the same event object for both touch start events, one would propagate to the other swipe area in the browser |
There was a problem hiding this comment.
Need to do some manual testing to understand what was replayed here.
- events are never re-used
- if this is about propagating then the DOM takes care of it (not dispatching multiple times). The original idea was probably replaying about hit detection
There was a problem hiding this comment.
Verified in https://codesandbox.io/s/single-swipearea-lock-ksyss (monitorEvents(document.body, ['touchstart', 'touchmove', 'touchend'])) that the way the test is setup a touchstart would only be fired at the last rendered SwipeableDrawer. The SwipeAreas are stacked on top of each other and hit detection never "pierces" elements i.e. if a touch hits an element then the event is targetted at that element not at all the ones the touch pierces. Then the event bubbles up but since the SwipeAreas are siblings, only one registers the touchstart.
I also remembered that subsequent touchmove and touchend target the same element as the touchstart.
But I don't know how I could get this test to fail. I thought https://github.com/mui-org/material-ui/blob/5f882fefadbdd72e950a4940a550c1d3287dd150/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L300-L303 was responsible for this behavior but removing the early return does not fail any test.
@oliviertassinari Do you know what implementation this test was ensuring or how one would test https://github.com/mui-org/material-ui/blob/5f882fefadbdd72e950a4940a550c1d3287dd150/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L300-L303
Also please review the full test diff (https://github.com/mui-org/material-ui/pull/26916/files#diff-2e95d46f49f995c596826a9b8b12a84dc9de890ac9ffb65d208dd2fc23b2ccbeR508-R551) to make sure I didn't miss something.
There was a problem hiding this comment.
The new test case looks correct. Regarding getting it to fail. We added more defensive logic, later on, to avoid two swipeable drawers responding to the same event. It seems that this logic is winning over the previous solutions. For instance, event.defaultMuiPrevented now only seems to make sense if there is a custom component.
claimedSwipeInstance now seems to be solved by testing the target of the event. We should be able to remove this logic.
There was a problem hiding this comment.
I'm somewhat leaning towards keeping the implementation to be safe. Touch related interactions are hard to test automatically so it may take a while before people realize we broken something.
Part of #22911
Part of #13394