Skip to content

[react-interactions] Prevent duplicate onPress firing for keyboard Enter #17266

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

Merged
merged 2 commits into from
Nov 5, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 4, 2019

Internally, we encountered an issue where the LegacyPress responder would double fire onPress when the Enter keyboard key was pressed. Looking into this deeply, the reason for this was because the browser triggers a click event which passes the logic in isScreenReaderVirtualClick.

I tried to find different discrepancies between screen reader clicks and the click that is fired from the browser when you press Enter but I couldn't see anything that stood out – so I took another approach and realized, by accident, the reason this doesn't happen when you press Space or key – we trigger nativeEvent.preventDefault. This means that the corresponding native click never gets triggered by the browser, but the responder still correctly fires the onPress callback – this time only once though!

I also noticed that is behavior didn't occur in the newer Press responder. I added support for the Spacebar key type there, but I also noticed a difference in input and textarea logic between the two responders that might be worth while tracking in a separate task – but maybe this is something @necolas has already considered.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 4, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9034656:

Sandbox Source
modern-cache-oi3sz Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 4, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 529a5dd:

Sandbox Source
hungry-saha-vez43 Configuration

@sizebot
Copy link

sizebot commented Nov 4, 2019

Size changes (experimental)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 529a5dd

@sizebot
Copy link

sizebot commented Nov 4, 2019

Size changes (stable)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 529a5dd

@trueadm trueadm force-pushed the duplicate-on-press branch 2 times, most recently from 221bdba to 53bc9d4 Compare November 4, 2019 18:19
@trueadm trueadm force-pushed the duplicate-on-press branch from 53bc9d4 to 8ac6c1e Compare November 4, 2019 18:21
@@ -914,8 +914,7 @@ describe.each(environmentTable)('Press responder', hasPointerEvents => {
ReactDOM.render(<Component />, container);

const target = createEventTarget(ref.current);
target.keydown({key: 'Enter'});
target.click({preventDefault});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the click event removed from this test? When Enter is pressed is certain host elements it will cause a click to be dispatched

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not if you preventDefault, meaning click will never fire.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we preventDefault on Spacebar it because otherwise it scrolls the window. Should we preventDefault on keydown for Enter though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, unless you know of any reason why we should not?

@trueadm
Copy link
Contributor Author

trueadm commented Nov 4, 2019

@necolas You seem to know quite a bit about this logic. If you are happy and have time, please could you do some digging into it to help me? I'd really appreciate it :)

@trueadm trueadm merged commit cd1bdcd into facebook:master Nov 5, 2019
@trueadm trueadm deleted the duplicate-on-press branch November 5, 2019 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants