eui

eui copied to clipboard

To establish context, the scenario this issue refers to is the 3rd column in https://elastic.github.io/eui/#/tabular-content/data-grid-focus:

The logic that controls this behavior (focusing immediately into the only interactive child if isExpandable is false) is set in these 3 locations in data_grid_cell:

Expand to show code snippets

https://github.com/elastic/eui/blob/6ed06df31d8b3fb20d270f98ccf9e66cf26623d7/src/components/datagrid/body/data_grid_cell.tsx#L163-L167

https://github.com/elastic/eui/blob/6ed06df31d8b3fb20d270f98ccf9e66cf26623d7/src/components/datagrid/body/data_grid_cell.tsx#L396-L399

https://github.com/elastic/eui/blob/6ed06df31d8b3fb20d270f98ccf9e66cf26623d7/src/components/datagrid/body/data_grid_cell.tsx#L575-L579

UI concerns

@cchaos originally raised this issue as a UI one. Due to the fact that querying through the cell's children can take JS a non-zero amount of time to do, there is an intermittent flash of focus ring on Chrome before focus is manually shifted to the child:

https://user-images.githubusercontent.com/549407/156667945-3a2dc746-e783-40e2-bef3-1bf01d906e88.mp4

Accessibility concerns

@cchaos also highlighted that this is a potential accessibility issue as well. This behavior unintentionally affects cells that have single interactive children (e.g.) links but also other non-interactive content.

In the above auto heights example/screencap (https://elastic.github.io/eui/#/tabular-content/data-grid-row-heights-options#auto-heights-for-rows), the second column items have a title link that is immediately focused on arrow key navigation, with the rest of the cell contents not being read out.

After some testing, I determined that screen reader ctrl+opt+arrow key navigation still works to get to the rest of the cell contents, so it's not a hugely blocking issue, but still a friction point that could likely be smoothed out.

WAI-ARIA spec

My best guess as to the reason why we implemented the above conditional logic for non-expandable cells with single interactive children is that we were following W3's datagrid spec, particularly the "Description" cells: https://www.w3.org/TR/wai-aria-practices-1.2/examples/grid/dataGrids.html

Possible solutions

1. Change our conditional logic to check that there's only 1 child I think where our current logic falls apart is determining whether there is one interactive element in the cell vs the interactive element being the only child of the cell. For example, our logic works well for control columns with 1 button, but not for the auto height example with a link and paragraphs of text. Caveat: it is not trivial to tell (DOM-wise) whether a cell's content is "just one interactive element" or not; for example if a user wraps their single <button> in a <span> that defeats a quick childNode.length === 1 && isTabbable type check.

2. Treat cells with 1 interactive child the same way we currently treat cells with 2+ interactive children You can examine the way we treat isExpandable cells with 2+ interactive children in Column of https://elastic.github.io/eui/#/tabular-content/data-grid-focus. Currently with 2+ interactive children, keyboard users are required to press the Enter key to enter the cell's focus trap and be able to tab between contents: Caveat: We should likely still keep an exception for control columns here, since control columns are typically only buttons/checkboxes etc., which should auto-focus if possible

3. Force cells with any interactive children to be isExpandable I played around with a spike for approach 2 and still ran into some screen reader difficulties. I couldn't get screen reader navigation to work quite as expected when focus is trapped within a cell. SR navigation within cell popovers always worked, though, so it's tempting to take the easy way out and force the expansion popover whenever interactive children are present. Caveat: This isn't as technically easy as I make it sound, as we'll need to add some sort of mutation observer to update isExpandable state whenever cell contents change (either cells being reused by virtualization or data changing dynamically upstream).

@zuhairmahd & @1Copenut, we'd definitely love any recommendations y'all have here on accessibility and screen reader behavior/concerns.

Reading through this issue a few times, I feel like option 3 Force cells with any interactive children to be isExpandable is the best path forward. It does require a second key press, but normalizes the experience with interactive content and doesn't have the overhead of conditional checks.

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context.