Looks like the checkbox is only shown for the multi-select picker.

Fixed!

One of my concerns over table is whether the aria semantics are right for a picker.

For grid and list I think it's easy to use the listbox role and it works well, but for table it's a bit harder to find the right fit because table already has its own role. Maybe there's a way to make it work. 🤔

Perhaps grid is a better fit than the default table roles, as I believe it's supposed to support aria-selected for rows - https://www.w3.org/WAI/ARIA/apg/patterns/grid/.

I think infinite scroll should always be a feed role regardless. whether that's compatible with the picker semantics I don't know 😅

Is it worth updating the README as well?

https://github.com/WordPress/gutenberg/blob/try/data-picker-table-view/packages/dataviews/README.md#dataviewspicker

The items in the view are rendered using the listbox and option aria roles.

Asking coz I don't know 😄

Thinking about this, is there any good reason for it not to be a listbox? Really the only difference between picker-grid and picker-table is the visual layout, not anything functional. Wouldn't it make sense for them to use the same role?

Perhaps it would work.

So I guess the idea would be to make the table the listbox and then the table rows become options, but then for table all the cells also have implicit aria roles, and that's the part that I'm not sure what to do:

I don't think cell roles work within option roles.

Similar situation with the header:

I think you end up losing the header relationship with the cells if it's switched to listbox.

Do you mean it's unlikely to have multiple columns?

Yes sorry, columns, not rows!

OK let's try a listbox and removing table semantics. We can always revisit later if we find it doesn't work.

Oh, I was actually wondering if folks would use multiple columns to be able to sort by different columns easily. Would that still be possible in a listbox?

I think it would work, as the buttons that open the dropdowns within the table header cells would still be accessible/tabbable and have the button role, but some semantic info from the table might be lost. I haven't tested how well it works in trunk or #72997 yet though, so maybe it wouldn't be too different:

Yeah the dropdowns work. I'm not sure how clear it would be for a screen reader user that e.g. the "title" menu pop-up button hides sorting options for that column, but that's probably also the case in the table layout.

I've updated to use the listbox semantics, and added presentation roles to table elements that aren't interactive.

There's no need for row actions for a picker.

Should probably be able to remove stuff like this, it's non-picker related.

I wasn't sure about this because pickers can have actions, can't they? So they might have bulk actions too?

They work a bit differently though. Regular dataviews doesn't allow row selection when there are no bulk actions (which is what this LOC prevents). It shows single item actions on the row itself so doesn't require an item to be checked.

The picker always allows row selection even when there are no bulk actions, single item actions appear in the footer so require an item to be checked.

Oooh I see, thanks for explaining!

For the regular dataview, clicking the row only selects one item at a time (same as what this code does). I think it could be revised when the picker support multi-selection so that clicking the row also multi-selects.

Actually I don't think clicking the row to select is working at all here, need to fix that too 😅

Does the checkbox column need a <th /> here? Without it the headers aren't matched with new columns:

oh I accidentally scrapped that in my earlier bulk select deletion spree 🤦

Is it worth updating the README as well?

https://github.com/WordPress/gutenberg/blob/try/data-picker-table-view/packages/dataviews/README.md#dataviewspicker

The items in the view are rendered using the listbox and option aria roles.

Asking coz I don't know 😄

Is an aria-selected prop appropriate here?

That depends on what role we end up assigning to the parent container following the discussion here 😄

I think this will be true most of the time.

What if it checks if the click came from the checkbox (or other interactive elements)?

Oh well spotted I don't think we need this check at all!

Do you reckon we should check for touch devices like the regular table does?

https://github.com/WordPress/gutenberg/blob/try/data-picker-table-view/packages/dataviews/src/dataviews-layouts/table/index.tsx#L171

That logic in the table layout handles multi-selecting with modifier keys (it only checks whether we're not on a touch device) whereas in the picker we can multi-select just by clicking on several items, no modifiers needed (as long as multi-select is enabled).

Not sure, but just wondering if the colSpan should include the checkbox column? E.g.,

colSpan={
  columns.length +
  ( hasPrimaryColumn ? 1 : 0 ) +
  1 // checkbox column
}

Oh yeah, maybe that's why it's looking weird 😂

In my testing, nothing breaks, but I noticed that the regular table stops event propagation to stop click events from bubbling up to the table row.

https://github.com/WordPress/gutenberg/blob/try/data-picker-table-view/packages/dataviews/src/dataviews-layouts/table/index.tsx#L263

Worth doing here as well?

The regular table does that because it can have inline buttons which do different actions, whereas here we want any click on any part of the row to select that row, so it should propagate.

Ah gotcha. Thanks for the explainer.

This PR's testing really nicely for me so far! This is such a minor point, but I was wondering if it's worth defaulting just the table view in this story to show more of the fields by default? Just to get a slightly more real world sense of how this view would look, because otherwise these stories only show the title and media fields. I.e. defaulting it to look more like this:

Feel free to skip this idea, though, we can always keep tweaking the storybook stories for these in follow-ups.

The grid uses the group role for grouping (the listbox role supports it), so maybe it can work the same here.

Testing with Safari + Voiceover, and it's interesting that when a checkbox receives focus all the content on the row is announced. Including some of the aria-labels:

Hopefully that's something that can be improved if we add keyboard navigation (so the rows are selectable instead of the checkboxes). I expect stuff on the row to still be announced, but maybe the "Click Item" will go away and the title won't be repeated.

I think we should also consider changing the styles compared to regular dataviews so that the checkboxes are always visible.

if we add keyboard navigation (so the rows are selectable instead of the checkboxes)

I'm trying this now using Composite similarly to picker-grid, but for some reason it doesn't work - the whole list focuses instead of the first element, when tabbing into it.

Feel free to push the changes and I'd be happy to help debugging.

Ok I've pushed the changes, and I added table layout to the experimental media modal too, so it's easier to debug (the react dev tools don't seem to work very well in storybook)

Oh, I thought we'd want to preserve tab navigation for the table header elements, which is why I put it in the body. But if it only works on table then I guess that's what we have to do 😅

If your changes look good would you like to push them?

Oh good point, I didn't think of that.

Actually I retested with your changes, and it does work, the issue is that the styles need to be updated. The virtualFocus option uses aria-activedescendent, and ariakit adds data-active-item attributes to the elements, so that need to be styled. Here's the equivalent styles for the picker grid:

I think it is still a bit broken for the grouped view, so that might need some fixes, or we decide not to support it.