[BottomNavigationAction] Fix onClick doublefire in Firefox #28242
Conversation
|
Details of bundle changes (experimental) @material-ui/core: parsed: +Infinity% , gzip: +Infinity% |
|
It seems like the codesandbox check is stuck? It's been running for 40 minutes now, other builds only go for about 4-5 minutes? 🤔 |
| target: container, | ||
| clientX: 42, | ||
| clientY: 42, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| fireEvent.touchEnd(container.firstChild, { | ||
| changedTouches: [ | ||
| new Touch({ | ||
| identifier: 1, | ||
| target: container, | ||
| clientX: 42, | ||
| clientY: 42, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| getByRole('button').click(); | ||
|
|
||
| clock.tick(100); | ||
|
|
||
| expect(handleClick.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it('should not fire onClick if swiping', () => { | ||
| const handleClick = spy(); | ||
|
|
||
| // Need disableTouchRipple to avoid ripple missing act (async setState after touchEnd) | ||
| const { container } = render( | ||
| <BottomNavigationAction onClick={handleClick} disableTouchRipple />, | ||
| ); | ||
|
|
||
| fireEvent.touchStart(container.firstChild, { | ||
| touches: [ | ||
| new Touch({ | ||
| identifier: 1, | ||
| target: container, | ||
| clientX: 42, | ||
| clientY: 42, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| fireEvent.touchEnd(container.firstChild, { | ||
| changedTouches: [ | ||
| new Touch({ | ||
| identifier: 1, | ||
| target: container, | ||
| clientX: 84, | ||
| clientY: 84, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| clock.tick(10); | ||
|
|
||
| expect(handleClick.callCount).to.equal(0); | ||
| }); | ||
|
|
||
| it('should forward onTouchStart and onTouchEnd events', () => { | ||
| const handleTouchStart = spy(); | ||
| const handleTouchEnd = spy(); | ||
|
|
||
| // Need disableTouchRipple to avoid ripple missing act (async setState after touchEnd). | ||
| const { container } = render( | ||
| <BottomNavigationAction | ||
| onTouchStart={handleTouchStart} | ||
| onTouchEnd={handleTouchEnd} | ||
| disableTouchRipple | ||
| />, | ||
| ); | ||
|
|
||
| fireEvent.touchStart(container.firstChild, { | ||
| touches: [ | ||
| new Touch({ | ||
| identifier: 1, | ||
| target: container, | ||
| clientX: 42, | ||
| clientY: 42, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| expect(handleTouchStart.callCount).to.be.equals(1); | ||
|
|
||
| fireEvent.touchEnd(container.firstChild, { | ||
| changedTouches: [ | ||
| new Touch({ | ||
| identifier: 1, | ||
| target: container, | ||
| clientX: 84, | ||
| clientY: 84, | ||
| }), | ||
| ], | ||
| }); | ||
|
|
||
| expect(handleTouchEnd.callCount).to.be.equals(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
How did you come up with this event order? A codesandbox to verify that this is actually happening, is important. For example, it's a bit odd that a child is the target of the touch event but the touch itself is the parent.
There was a problem hiding this comment.
I do not remember, the PR for this was accepted by @oliviertassinari a year ago, in #22524.
The only new code in this PR, is the code in commit 6082ceb.
packages/mui-material/src/BottomNavigationAction/BottomNavigationAction.test.js
Outdated
Show resolved
Hide resolved
@eps1lon Honestly it's pretty frustrating that you disagree internally, when interacting outwards with contributers who want to help out. @oliviertassinari clearly asked me to work on this here, so that you now continue to question it's entire existence (despite a core MUI member asking me to fix that regression) is a pretty confusing choice of communication. |
github-actions
bot
added
the
PR: out-of-date
This PR reverts #27690, fixes #27664 and closes #22368, by increasing the timeout used on touch events, to determine whether the onClick event should be fired or not.
BottomNavigationAction now uses the DELAY_RIPPLE timeout, imported from TouchRipple.js.
The investigation for the regression can be found here #22368 (comment).