Skip to content
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

[react-ui] Add preventDefault+ to Keyboard + update a11y components #16833

Merged
merged 1 commit into from
Sep 20, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Sep 19, 2019

This PR is a sort of spiritual follow up to #16822. Here are the changes outlined in this PR:

  • Keyboard responder now provides preventDefault and continuePropagation to the user event. Keyboard still does not propagate to other keyboard responders by default, but provides a method to allow it to continue and extra level. I had to update tests respectfully, as we previous used the return value from the event (which was a bit confusing).
  • Keyboard responder now does not preventDefault any events by default. Instead, the user can optionally control this by calling preventDefault like already done on the web. The Press responder has been updated to ensure that the same standard behavior exists as before.
  • The accessibility components have been updated to properly make use of this new functionality.

@sizebot
Copy link

sizebot commented Sep 19, 2019

Details of bundled changes.

Comparing: 924a305...4bf5585

react-ui

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-ui-events/focus.development.js 0.0% -0.1% 10.92 KB 10.92 KB 2.36 KB 2.36 KB UMD_DEV
react-ui-events/press.development.js +3.4% +1.0% 4.37 KB 4.52 KB 1.3 KB 1.32 KB UMD_DEV
react-ui-events/tap.development.js 0.0% -0.0% 16.39 KB 16.39 KB 3.66 KB 3.66 KB UMD_DEV
react-ui-events/focus.production.min.js 0.0% -0.1% 4.09 KB 4.09 KB 1.39 KB 1.39 KB UMD_PROD
react-ui-events/press.production.min.js 🔺+2.0% 🔺+0.9% 2.02 KB 2.06 KB 935 B 943 B UMD_PROD
react-ui-events/tap.production.min.js 0.0% -0.1% 5.8 KB 5.8 KB 2.25 KB 2.24 KB UMD_PROD
ReactFocusTable-dev.js -0.1% +0.4% 6.69 KB 6.68 KB 1.58 KB 1.59 KB FB_WWW_DEV
react-ui-events/context-menu.development.js 0.0% -0.1% 2.67 KB 2.67 KB 1004 B 1003 B UMD_DEV
react-ui-events/input.development.js 0.0% -0.1% 4.51 KB 4.51 KB 1.44 KB 1.44 KB UMD_DEV
react-ui-events/context-menu.production.min.js 0.0% -0.7% 1.38 KB 1.38 KB 732 B 727 B UMD_PROD
react-ui-events/input.production.min.js 0.0% -0.2% 1.83 KB 1.83 KB 982 B 980 B UMD_PROD
ReactEventsKeyboard-dev.js -20.3% -12.5% 7.01 KB 5.58 KB 2.39 KB 2.09 KB FB_WWW_DEV
react-ui-events/scroll.production.min.js 0.0% -0.2% 2.62 KB 2.62 KB 1.15 KB 1.14 KB UMD_PROD
ReactEventsKeyboard-prod.js -24.8% -19.1% 5.38 KB 4.04 KB 1.73 KB 1.4 KB FB_WWW_PROD
react-ui-events/context-menu.development.js 0.0% -0.1% 2.48 KB 2.48 KB 958 B 957 B NODE_DEV
react-ui-events/input.development.js 0.0% -0.1% 4.33 KB 4.33 KB 1.4 KB 1.39 KB NODE_DEV
react-ui-events/scroll.development.js 0.0% -0.1% 6.11 KB 6.11 KB 1.61 KB 1.61 KB NODE_DEV
react-ui-accessibility/tab-focus.development.js +8.9% +3.1% 4.38 KB 4.77 KB 1.21 KB 1.25 KB NODE_DEV
react-ui-events/context-menu.production.min.js 0.0% -0.3% 1.19 KB 1.19 KB 668 B 666 B NODE_PROD
react-ui-events/input.production.min.js 0.0% -0.1% 1.65 KB 1.65 KB 914 B 913 B NODE_PROD
react-ui-events/scroll.production.min.js 0.0% -0.2% 2.43 KB 2.43 KB 1.09 KB 1.09 KB NODE_PROD
react-ui-accessibility/tab-focus.production.min.js 🔺+6.6% 🔺+1.9% 1.66 KB 1.77 KB 836 B 852 B NODE_PROD
react-ui-events/hover.development.js 0.0% -0.1% 7 KB 7 KB 1.55 KB 1.55 KB UMD_DEV
react-ui-events/press-legacy.development.js 0.0% -0.0% 20.73 KB 20.73 KB 4.83 KB 4.83 KB UMD_DEV
react-ui-events/hover.production.min.js 0.0% -0.3% 3.11 KB 3.11 KB 1.13 KB 1.13 KB UMD_PROD
react-ui-events/press-legacy.production.min.js 0.0% -0.1% 6.97 KB 6.97 KB 2.63 KB 2.63 KB UMD_PROD
ReactTabFocus-dev.js +7.6% +2.2% 4.43 KB 4.77 KB 1.19 KB 1.21 KB FB_WWW_DEV
ReactTabFocus-prod.js 🔺+9.2% 🔺+3.2% 3.53 KB 3.85 KB 1.07 KB 1.11 KB FB_WWW_PROD
react-ui-events/hover.development.js 0.0% -0.1% 6.81 KB 6.81 KB 1.51 KB 1.51 KB NODE_DEV
react-ui-events/press-legacy.development.js 0.0% -0.0% 20.54 KB 20.54 KB 4.79 KB 4.78 KB NODE_DEV
react-ui-accessibility/focus-table.development.js -0.1% +0.3% 6.83 KB 6.83 KB 1.63 KB 1.63 KB NODE_DEV
react-ui-events/hover.production.min.js 0.0% -0.3% 2.93 KB 2.93 KB 1.07 KB 1.07 KB NODE_PROD
react-ui-events/press-legacy.production.min.js 0.0% -0.1% 6.79 KB 6.79 KB 2.57 KB 2.57 KB NODE_PROD
react-ui-accessibility/focus-table.production.min.js 🔺+0.2% 🔺+0.3% 2.27 KB 2.27 KB 995 B 998 B NODE_PROD
ReactFocusTable-prod.js -3.1% -0.6% 6.08 KB 5.89 KB 1.39 KB 1.38 KB FB_WWW_PROD
react-ui-events/focus.development.js 0.0% -0.1% 10.73 KB 10.73 KB 2.32 KB 2.31 KB NODE_DEV
react-ui-events/press.development.js +3.6% +1.2% 4.1 KB 4.25 KB 1.23 KB 1.24 KB NODE_DEV
react-ui-events/tap.development.js 0.0% -0.1% 16.21 KB 16.21 KB 3.61 KB 3.6 KB NODE_DEV
react-ui-events/focus.production.min.js 0.0% -0.1% 3.92 KB 3.92 KB 1.32 KB 1.32 KB NODE_PROD
react-ui-events/press.production.min.js 🔺+2.3% 🔺+0.9% 1.75 KB 1.79 KB 846 B 854 B NODE_PROD
react-ui-events/tap.production.min.js 0.0% -0.1% 5.7 KB 5.7 KB 2.23 KB 2.23 KB NODE_PROD
react-ui-events/drag.development.js 0.0% -0.1% 5.22 KB 5.22 KB 1.54 KB 1.54 KB UMD_DEV
react-ui-events/keyboard.development.js -16.9% -10.6% 6.91 KB 5.75 KB 2.39 KB 2.14 KB UMD_DEV
react-ui-events/swipe.development.js 0.0% -0.1% 5.99 KB 5.99 KB 1.62 KB 1.62 KB UMD_DEV
react-ui-events/drag.production.min.js 0.0% -0.1% 2.24 KB 2.24 KB 1.07 KB 1.07 KB UMD_PROD
react-ui-events/keyboard.production.min.js -14.1% -13.3% 2.79 KB 2.4 KB 1.39 KB 1.21 KB UMD_PROD
ReactEventsPress-dev.js +3.6% +1.5% 4.08 KB 4.23 KB 1.2 KB 1.21 KB FB_WWW_DEV
react-ui-events/swipe.production.min.js 0.0% -0.2% 2.44 KB 2.44 KB 1.11 KB 1.1 KB UMD_PROD
ReactEventsPress-prod.js 🔺+3.3% 🔺+0.2% 3.54 KB 3.66 KB 1.02 KB 1.02 KB FB_WWW_PROD
react-ui-events/keyboard.development.js -17.3% -10.7% 6.73 KB 5.56 KB 2.34 KB 2.09 KB NODE_DEV
react-ui-events/swipe.development.js 0.0% -0.1% 5.81 KB 5.81 KB 1.58 KB 1.58 KB NODE_DEV
react-ui-accessibility/tabbable-scope.development.js 0.0% -0.3% 1.17 KB 1.17 KB 581 B 579 B NODE_DEV
react-ui-events/drag.production.min.js 0.0% -0.1% 2.87 KB 2.87 KB 1.38 KB 1.38 KB NODE_PROD
react-ui-events/keyboard.production.min.js -15.1% -14.1% 2.62 KB 2.22 KB 1.34 KB 1.15 KB NODE_PROD
react-ui-events/swipe.production.min.js 0.0% -0.1% 2.26 KB 2.26 KB 1.05 KB 1.05 KB NODE_PROD
react-ui-accessibility/tabbable-scope.production.min.js 0.0% -0.4% 670 B 670 B 445 B 443 B NODE_PROD

Generated by 🚫 dangerJS against 4bf5585

@necolas
Copy link
Contributor

necolas commented Sep 19, 2019

Not really convinced about event.continuePropagation() rather than event.stopPropagation(). Why not have keyboard events propagate unless otherwise prevented from doing so by the callback handling a key?

@trueadm
Copy link
Contributor Author

trueadm commented Sep 19, 2019

@necolas Simply because it means reworking the core event system to have a way of stopping and starting propagation, which then confuses the action with the web equivalent. We won't actually be stopping native propagation here, but overwhelmingly that's what people thought it was doing when we had the declarative prop internally.

TBH I'd rather stopPropagation was named differenty in general here, so people knowit was different from how they already understand these methods working on React and the web today.

@necolas
Copy link
Contributor

necolas commented Sep 19, 2019

it means reworking the core event system to have a way of stopping and starting propagation

Before we had return false mean "stop propagation". I'd rather we keep that than invert everything and have to call continuePropagation

We won't actually be stopping native propagation here, but overwhelmingly that's what people thought it was doing when we had the declarative prop internally.

Bike-shedding of names aside, people shouldn't expect that calling stopPropagation on an object (or returning false) is going to operate on a native DOM event when the callback isn't directly related to a DOM event in the first place.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 19, 2019

Let's go with stopPropagation then. I think this would be a good one for you to involved in (I'll reach out directly). Let's land the PR and then you follow up with the changes. Makes a lot of sense to me and I agree with your reasoning. The current PR still makes semantic sense, as the actual behavior won't change after your changes anyway, you're just inversing the API for propgataion, which makes more sense now given your above comment.

@trueadm trueadm merged commit 57a5805 into facebook:master Sep 20, 2019
@trueadm trueadm deleted the keyboard-responder-changes branch September 20, 2019 09:21
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.

None yet

4 participants