allow default TableRow keypress event behavior (#211) #276
Conversation
| if (this.props.isSelectable) { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| this.props.onSelect() | ||
| e.preventDefault() |
There was a problem hiding this comment.
Nice! Should we find another way to allow consumers to opt-in to preventing scroll of the page upon these key presses?
There was a problem hiding this comment.
@mshwery Thanks for the feedback and additional context on the original implementation here. Appreciate it!
As an alternative to additional configuration for opt-in behavior, it occurred to me that we could instead continue to invoke preventDefault but first wrap the function call within a conditional that would check if the event target is in fact the TableRow itself.
This way, if consumers of 🌲 decide to nest additional UI that can take focus or accept user input within an isSelectable TableRow, the browser can and will scroll and direct focus to these child UI elements when the event originates there. This is good, and helps with web accessibility standards. This is the scenario captured from the original issue report here.
Alternatively, for an isSelectable TableRow that contains text or other children that do not provide a target to fire a keyPress event, preventDefault will be called and browser scrolling will not occur when selecting a row. This scenario is captured in the existing storybook example here
src/table/src/TableRow.js
Outdated
| > | ||
| {children} | ||
| </Pane> | ||
| <div ref={this.handleRef}> |
There was a problem hiding this comment.
I'm using a wrapping div here so that we can attach a ref to the underlying node and avoid a call to findDOMNode which is discouraged for React 16 onward.
There was a problem hiding this comment.
Not sure why we would need to do to this? If you need the ref of the Pane you can use innerRef={ref => this.paneRef = ref}. Eslint will probably complain, so you might have to create a func to do it innerRef={this.onRef}
There was a problem hiding this comment.
Thanks @jeroenransijn. I took this approach because using a wrapping div was seemingly the easiest way preserve encapsulation. However, as you pointed out, Pane (via ui-box) has already been wired up to expose a ref prop that we can use. I wasn't aware of this, and that approach would work just fine as well.
For reference:
https://github.com/segmentio/ui-box/blob/master/src/box.js#L50-L54
I'll amend my commit to use this approach, and include a new storybook example that will make it easier to verify the bug has been fixed.
src/table/src/TableRow.js
Outdated
| // NOTE: In scenarios where the target for key press is our | ||
| // tabIndex <Pane> below, prevent the default event action | ||
| // so that browser scroll will not occur. | ||
| if (e.target.parentNode === this.node) { |
There was a problem hiding this comment.
Because of the wrapping div below, we compare the event target's parent node to our ref node here. If they match, the event was fired while keyboard focus was on the tabIndex <Pane> below and we should stop browser scroll by preventing the default event action.
If the event target was some other node, then the user has interacted with other types of nested UI, and we should not prevent the default action so that input and change events can fire normally
jeroenransijn
left a comment
jeroenransijn
left a comment
There was a problem hiding this comment.
I will have to take a bit more of a look at this, and potentially add this to a v4 branch to see if the expected behavior is good. Hopefully next week!
src/table/src/TableRow.js
Outdated
| > | ||
| {children} | ||
| </Pane> | ||
| <div ref={this.handleRef}> |
There was a problem hiding this comment.
Not sure why we would need to do to this? If you need the ref of the Pane you can use innerRef={ref => this.paneRef = ref}. Eslint will probably complain, so you might have to create a func to do it innerRef={this.onRef}
mshwery
left a comment
mshwery
left a comment
There was a problem hiding this comment.
Awesome! Nice update. LGTM 🚢
|
Thanks for the work @JustAboutJeff ! Highly appreciated! Will take a look soon to move this in v4 as well. 🌲 💯 🎉 |
* master: (25 commits) Remove storybook-deployer v3.2.7 Fix typo in Tooltip proptypes (#321) fix githubLink pathname on ComponentReadme (#310) Use `rm -rf` in prepublishOnly Run the babel builds concurrently circleci: fix the gh-pages ignore config circleci: fix the gh-pages ignore Migrate to circleci 2.0 Adding a link to Synapse Fix wording in Toaster docs (#294) v3.2.6 allow default TableRow keypress events (#221) (#276) Fix misspelling of ListItem component name (#268) 🌲 CI status image (#263) Add link to v4 docs and PR Add more spacing Add Hero image Add GitHub hero Add note to v4 ... # Conflicts: # README.md # docs/src/components/ComponentReadme.js # package.json # src/segmented-control/src/SegmentedControl.js # src/segmented-control/src/SegmentedControlRadio.js # src/segmented-control/src/styles/SegmentedControlAppearances.js # src/select-menu/src/OptionsList.js # src/select-menu/src/SelectMenuContent.js # src/table/src/TableRow.js # src/table/stories/index.stories.js # src/toaster/docs/index.js # yarn.lock
* v4: (30 commits) Tracking fix (#338) add segment tracking (#337) Upgrade dependencies v4 (#336) V4 Docs (#335) Remove storybook-deployer Remove storybook-deployer v3.2.7 Fix typo in Tooltip proptypes (#321) fix githubLink pathname on ComponentReadme (#310) Use `rm -rf` in prepublishOnly Run the babel builds concurrently circleci: fix the gh-pages ignore config circleci: fix the gh-pages ignore Migrate to circleci 2.0 Adding a link to Synapse Fix wording in Toaster docs (#294) v3.2.6 allow default TableRow keypress events (#221) (#276) Fix misspelling of ListItem component name (#268) 🌲 CI status image (#263) ... # Conflicts: # package.json
3 participants
allow default TableRow keypress event behavior (#211)
Thanks for creating evergreen! 🌲
tl;dr remove the
e.preventDefault()function call so thatinputandchangeevents can be fired normally after thekeypressevent .Web standards specify that a
keypressevent should be fired prior to the correspondinginputevent for the same keyWeb standards also specify that cancelling a
keypressevent will also prevent subsequentinputevents from being firedWeb standards further specify that a
inputevent should be fired prior to anychangeevents for a given inputFrom all this, we can conclude that
keypressevent handlers will be invoked prior to anychangehandlers attached in our DOM tree. These two examples confirm this for both native DOM and React SyntheticEvent. In addition, cancelling the default behavior for akeypressevent will preventinputandchangeevents from firing for the same user interaction, the cause of our bug here!