Data views: Add click-to-select behavior on table rows #59803
Conversation
jameskoster
added
[Type] Enhancement
jameskoster
requested review from
oandregal,
jorgefilipecosta,
ntsekouras and
a team
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +1.06 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
A question, can we make it so clicking anywhere on the row selects it? We could keep the pointer cursor for just the checkbox, and have the default cursor remain for just selecting. This would:
|
|
Good question. We can do it, but it would be good to fully explore what the on-row-click action should be. Selection is a good option because it can be applied consistently across all data views. But there may be an argument to use the action associated with the primary field. In the case of pages, templates, and patterns that would mean taking you to the editor. If that were the case we could remove 'Edit' as a quick action in #59551. |
|
Pushed a change to try row click = selection. I think it works well, though it is quite easy to mis-click a title and end up in the editor. |
Pushed a couple of other tweaks to address this. The select.mp4 |
jameskoster
changed the title
|
In testing this, this feels like a solid step forward in terms of the ergonomics of selecting items, as well as unifying the behavior with the List view config. Finally it can open the opportunity for a future shift-click behaviors (click the top, hold shift, click the bottom, to select the range). To that end, I think this works in terms of behavior. I'd think a good next step would be to get a review on the behavior from a wider range. |
|
Cool. Tagging @andrewhayward as you worked on the original implementation of the click capture. |
| } ) } | ||
| onMouseEnter={ handleMouseEnter } | ||
| onMouseLeave={ handleMouseLeave } | ||
| onClickCapture={ ( event ) => { |
There was a problem hiding this comment.
The use of onClickCapture was introduced #59563 following-up similar changes for the grid layout at #59565.
As far as I understand:
- we wanted to keep the
onClickevent for the titles (go to editor) while providing a way to select the whole row - using CTRL or META keys as modifiers provided a way to tell apart the user's intention
By reducing the title box surface, we have a larger TR area and the different surfaces become clearer targets. So, if we go this route (selecting the row without the modifiers), we could use onClick instead of onClickCapture. No difference that I can see other than a simpler mental model for onClick.
cc @jorgefilipecosta @mcsf in case there's some other context I'm missing. With what I know, I like selecting the whole row on click without the modifiers.
There was a problem hiding this comment.
Yeah, the greedy onClickCapture approach was justifiable when it was predicated on event.ctrlKey || event.metaKey, otherwise it immediately becomes a blunt tool. Case in point, from the current branch:
Screen.Recording.2024-03-14.at.13.54.55.mov
There are tradeoffs to make here. Some approaches:
- Simplify, get rid of this trick, expand the title box area, and listen to regular (non captured) click events on the title box. Get rid of the fancy ctrl/meta click behaviour.
- Keep both side by side: ctrl/meta catches any click on the row, ignoring any underlying elements, while regular clicks with no modifiers will be caught on the title box (but no wider). After very short consideration, this is what I'd try first.
- Go for the more complex solution without title boxes. For instance:
- Ctrl/meta-clicks anywhere on the row should be caught at capture time, trigger a selection toggle, and stop event propagation.
- Regular clicks anywhere on the row will need to consider whether more granular listeners were triggered (i.e. whether the user has clicked on any interactive element inside the row, like a button). There some ways to do this (e.g. inspecting all DOM nodes between
event.targetandevent.currentTargetto find attached listeners), but I don't know how robust they are.
There was a problem hiding this comment.
Keep both side by side: ctrl/meta catches any click on the row, ignoring any underlying elements, while regular clicks with no modifiers will be caught on the title box (but no wider). After very short consideration, this is what I'd try first.
To clarify what I meant here: have two adjacent handlers:
<tr
onClickCapture={ e => {
if (e.ctrlKey || e.metaKey) {
// Same as before
e.stopPropagation(); e.preventDefault(); maybeToggle();
}
}
>
...
<TitleArea
onClick={ () => /* regular click handler to toggle */ }
/>
</tr>
| @@ -243,10 +250,10 @@ | |||
| white-space: nowrap; | |||
| overflow: hidden; | |||
| display: block; | |||
| width: 100%; | |||
| flex-grow: 0; | |||
There was a problem hiding this comment.
Not sure if this happens as a result of this change, but here's what I see for the focus ring:
There was a problem hiding this comment.
@jameskoster it sounds like this PR introduced this regression:
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Oh shoot, apologies I forgot to address this. I'll make a PR.
|
As per @oandregal's comment, we'd be better off moving this click-to-select behaviour behind a Cmd/Ctrl keypress, in much the same way we do with grid layouts. While it's not destructive to accidentally select the row, it is confusing and fairly unintuitive; it's not a typical table interaction. |
| } ) } | ||
| onMouseEnter={ handleMouseEnter } | ||
| onMouseLeave={ handleMouseLeave } | ||
| onClickCapture={ ( event ) => { |
There was a problem hiding this comment.
Yeah, the greedy onClickCapture approach was justifiable when it was predicated on event.ctrlKey || event.metaKey, otherwise it immediately becomes a blunt tool. Case in point, from the current branch:
Screen.Recording.2024-03-14.at.13.54.55.mov
There are tradeoffs to make here. Some approaches:
- Simplify, get rid of this trick, expand the title box area, and listen to regular (non captured) click events on the title box. Get rid of the fancy ctrl/meta click behaviour.
- Keep both side by side: ctrl/meta catches any click on the row, ignoring any underlying elements, while regular clicks with no modifiers will be caught on the title box (but no wider). After very short consideration, this is what I'd try first.
- Go for the more complex solution without title boxes. For instance:
- Ctrl/meta-clicks anywhere on the row should be caught at capture time, trigger a selection toggle, and stop event propagation.
- Regular clicks anywhere on the row will need to consider whether more granular listeners were triggered (i.e. whether the user has clicked on any interactive element inside the row, like a button). There some ways to do this (e.g. inspecting all DOM nodes between
event.targetandevent.currentTargetto find attached listeners), but I don't know how robust they are.
I'm more on board with this too 👌 |
Hah, this is how it already works on trunk. I feel that clicking a row to select is reasonably intuitive (though acknowledge it's subjective), but more importantly it will be an important interaction when the Inspector is accessible from data views (#59689). It would be frustrating to have to click the tiny checkbox every time you wanted to inspect a different entry. It also paves the way for shift+click and cmd+click for multi-selection. Another motivator here was to have the same selection mechanic as list view where click = selection. Perhaps it would be better to define all the behaviors in an issue first? |
|
This is not the most important thing in the world, it's not something we have to solve now, etc, but I wanted to echo Jay and comment on this one:
Permit me to disagree, I think it's incredibly common behavior, and approaching table-stakes for this type of item-management. Here's a little prior art. Google drive:
Dropbox:
Onedrive: Figma: MacOS finder, for good measure:
Outside of being, as I would argue with the above, a fairly common interaction, it's very ergonomic, providing a huge tap target for when you're managing multiple files or items. Finally it opens the doors for shift-range selection tools and ⌘A, in addition to ⌘-click to manually bulk select. I can see this being especially useful in the media library. Both select to insert in a gallery, or select to delete or tag multiple. |
|
I pushed the last three commits solely as proofs of concept:
Play with these commits as you wish, revert them, etc. I just wanted to illustrate. :) I tried to comment clearly. |
Point taken! Thank you for the multitude of examples 😆 With that being said, a couple of the examples exhibit slightly different behaviour; Google Drive and MacOS Finder, for example, use single click to select, and double click to open. The others nest interactive content, by putting links inside clickable containers, which is not a great interaction design decision, albeit evidently common; even if the row is not semantically interactive, it can be confusing for users who intend to tap the inner target but miss, and equally confusing for users who don't realise the inner target is interactive, tapping it with the intention of actioning the outer target. Ideally, if we proceed with clickable rows, we would avoid nesting interactive content such that there is only one tap target. I appreciate this isn't 100% possible, with the actions buttons and menu at the end of the row, but something to consider. Regardless, I will flag that we should ensure text can be selected without triggering the row selection. We can do this by checking the type of the current selection range during the click handler, and bailing if it is if ( document.getSelection().type === 'Range' ) ... |
|
Thanks @mcsf! 242966d looks pretty close to me, and hopefully addresses a good portion of @andrewhayward's feedback given the Actions cell is ignored for the purpose of selection. The title cell is a bit tricky as we need the nested interactivity there. For touch devices might we consider a long-press-to-select behavior? For desktop hopefully the interactivity (hover styles) help there. I reverted the bonus commit and pushed the text-selection fix. |
IMO we don't need the table-clicking behavior on mobile if that's in any way tricky, the checkboxes feel like something we could potentially emphasize there if need be. |
|
Yup, disabling this interaction on touch devices seems a reasonable place to start. |
|
@andrewhayward any objection to handling the touch-device behaviors in a follow-up? I'm not sure how best to go about that... |
|
Let me know if there's anything else I can do to help move this one forward. |
|
@mcsf thanks. Do you know if we can reliably detect a touch device and disable this behavior accordingly? If it's trivial I think that's the final detail. Otherwise I think we could merge and explore that in a follow-up. |
You can do a CSS-only fix, potentially: Was fairly solid last I used this. |
|
The click behavior is js, so I think the logic to disable it would need to also be js? |
|
Yes, unless you wanted to look at something featuring pointer-events. |
@jameskoster: I don't know if this is the prescribed way, but this is what felt right to me: by listening to |
|
That seems to be working well for me, thanks so much :) |
|
For completion (or perhaps for my own reference in the future): After a tip from @jorgefilipecosta, I looked at the That means that we can't reliably consolidate everything in a single // Won't work
<tr onClick={ e => {
if (e.pointerType !== 'touch') { ... }So we would need to resort to extra handlers, which I think defeats the purpose of // Might work, but I feel more confident about `touchstart`
<tr
onPointerDown={ e => { if (e.pointerType === 'touch') isTouch = true } }
onClick={ e => if (! isTouch) { ... } } |
There was a problem hiding this comment.
@jameskoster: looks ready as far as I'm concerned. Feel free to push that button!
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: andrewhayward <andrewhayward@git.wordpress.org>
Prior to WordPress/gutenberg#59803, the `a` inside the `.dataviews-view-table__primary-field` div occupied the full width of the div, so when Playwright clicked in the center of the div it would hit the link. That PR changed things up a bit, now the `a` is not expanded to fill the entire div and a click in the middle of the div misses the link, meaning the test never navigates to the next page as expected. This updates the logic to click the actual `a` instead of the div.
Prior to WordPress/gutenberg#59803, the `a` inside the `.dataviews-view-table__primary-field` div occupied the full width of the div, so when Playwright clicked in the center of the div it would hit the link. That PR changed things up a bit, now the `a` is not expanded to fill the entire div and a click in the middle of the div misses the link, meaning the test never navigates to the next page as expected. This updates the logic to click the actual `a` instead of the div.
What?
Capture clicks on on each table row and select the associated record accordingly.
Why?
A larger 'hitbox' makes it a bit easier for mouse / trackpad users to select items.
How?
Adjusts the keyboard listener we had to include clicks.
Testing Instructions
select.mp4
Notes: