[EuiDataGrid] Change header actions trigger element and enable interactive children

[mgadewoll]

Summary

closes #7660

This PR updates how EuiDataGrid header cells behave to support interactive cell content.

There are 2 separate scenarios for standalone header cell navigations with this update:
a) the header cell only has no or only 1 actions button and no other interactive children
b) the header cell has at least 1 interactive child other than the actions button

For scenario a) the navigation works the same as before: Focusing the header cell announces that Enter keypress will trigger the actions button and open the actions popover.

For scenario b) the change enables support for interactive content navigation the same way as it's already used for body cells: focusing the header cell announces that Enter keypress will enter the cell to navigate cell content, navigating the cell content follows default DOM navigation and pressing the Escape key exits the cell and focuses the header cell.
This PR additionally adds aria-live output on leaving cells to provide feedback.

To ensure WCAG compliancy for interactive targets, the actions button size was increased from 16px to 24px and a visual hover state was added.

Screen.Recording.2024-08-05.at.13.39.24.mov

QA

• review EuiDataGrid storybook and verify that the header cells look as expected and keyboard navigation works

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
  • Checked in VoiceOver, NVDA, JAWS, for screen reader behavior
• Docs site QA
  • Added documentation
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Checked Code Sandbox works for any docs examples
• Code quality checklist
  • Added or updated jest and cypress tests
  • Updated visual regression tests
• Release checklist
  • A changelog entry exists and is marked appropriately.
  • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
• Designer checklist
  • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

[1Copenut]

@mgadewoll I tested this today with NVDA (Chrome, Firefox) and JAWS (Chrome). The results were very good. Your live regions announced properly and the aria-describedby attributes did too. No axe-core issues detected. This is a big improvement for our users who navigate with screen readers. Thank you!

[cee-chen]

@1Copenut I don't know if this is just me but it feels like a net keyboard UX loss to have to press Enter twice now to toggle header cell actions as opposed to just once (production).

@mgadewoll Do you think there's any way if we can detect if there's interactive children other than the cell header actions and only create a focus trap then, but otherwise auto trigger the cell actions on enter keypress if it's the only interactive child?

[cee-chen]

HandleInteractiveChildren already iterates through tabbable children here:

eui/packages/eui/src/components/datagrid/body/cell/focus_utils.tsx

Lines 44 to 50 in 5c40315

	if (cellEl) {
	const interactiveChildren = disableInteractives(cellEl);
	if (renderFocusTrap) {
	setHasInteractiveChildren(interactiveChildren!.length > 0);
	}
	}

Instead of adding another O(n) iteration through the DOM, can we create/pass an optional onInteractiveChildrenFound callback that passes the array of interactive children? e.g.,

      const interactiveChildren = disableInteractives(cellEl);
      onInteractiveChildrenFound?.(interactiveChildren);

      if (renderFocusTrap) {
        setHasInteractiveChildren(interactiveChildren!.length > 0);
      }

And then this component can then simply re-use it/inspect it for the actions button.

[mgadewoll]

The main issue here is that disableInteractives uses tabbable which excludes tab-index="-1". What happens (already considering your proposal):

• on mount renderFocusTrap is false because initially we don't know if hasInteractiveChildren is true/false
• disableInteractives returns the correct array of interactive children and onInteractiveChildrenFound can be used to update the hasInteractives state in the header wrapper component which updates renderFocusTrap prop on HandleInteractives
• this then triggers disableInteractives again through the useEffect, which now returns an empty array because the run on mount disabled the children, that's why I used focusable here which does not exclude tab-index="-1" -> this empty array triggers the hasInteractives in focus_utils to be false and return the children instead of a focus trap here

We do need some way to distinguish this, but only for header cells as it's specific to them. That's why I moved the functionality to the wrapper initially to separate it and shouldDisableInteractives would be the condition to switch between that behavior.
You're right that we can just use focus_utils anyway, but I think we still need shouldDisableInteractives to prevent re-running disableInteractives for header cells once renderFocusTrap updates on hasInteractives state update.

[mgadewoll]

I updated to use the suggested approach of reusing the functionality in HandleInteractiveChildren here

🗒️ This still uses shouldDisableInteractives to ensure we prevent duplicate runs of disableInteractives when unwanted as explained above.

[cee-chen]

Ahh gotcha. I think we can avoid all this simply by removing the if (renderFocusTrap) conditional which triggers the useEffect to re-run. To be totally honest I'm not really sure why I added it in the first place. Can you try this and see if it works?

  // On mount, disable all interactive children
  useEffect(() => {
    if (cellEl) {
      const interactiveChildren = disableInteractives(cellEl);
      setHasInteractiveChildren(interactiveChildren!.length > 0);
      onInteractiveChildrenFound?.(interactiveChildren);
    }
  }, [cellEl, onInteractiveChildrenFound]);

Note that this will require wrapping onInteractiveChildrenFound in a useCallback to not trigger the effect repeatedly, but we should be memoizing everything we can in EuiDataGrid in any case.

[mgadewoll]

Hmm, looking at the logic I think renderFocusTrap is needed.

• on mount disableInteractives returns elements for all cells as they initially are not disabled yet at the same time we have renderFocusTrap=false and hasInteractiveChildren=false
• if we remove the renderFocusTrap check, then on mount all cells would be set to interactive which is not the expected behavior

[mgadewoll]

Since we have two distinct variants (interactive/non-interactive) I'd say the the labelling would need to be conditional now. What I'm thinking is something like this:

// EuiDataGridHeaderCell.tsx

// actionsButton
<button
  ...
  onFocus={() => setIsActionsButtonFocused(true)}
  onBlur={() => setIsActionsButtonFocused(false)}
  aria-labelledby={
    isInteractive && isActionsButtonFocused ? contentAriaId : undefined
  } // adds cell title/content as context to the button e.g. `Name`
  aria-describedby={
    !isInteractive || isActionsButtonFocused ? actionsAriaId : undefined
  } // adds instructions `Click to view header actions`
  aria-hidden={
    !isInteractive || isActionsButtonFocused ? 'false' : 'true'
  } // hacky way to prevent button from being read on header cell focus
>
...
</button>

// EuiDataGridHeaderCellWrapper 
<EuiDataGridHeaderCellWrapper
  ...
  aria-label={isInteractive ? title : undefined} // use the aria-label only for interactive cells to ensure reading the title first for context
  aria-describedby={sortingAriaId}
  onInteractivesFound={(interactive) => setInteractive(interactive)}
>
...
</EuiDataGridHeaderCellWrapper>

[cee-chen]

Screen reader testing is really throwing me for a loop. I'm not sure I'm headed in the right direction, so just thought I'd throw this spike your way: https://github.com/mgadewoll/eui/compare/datagrid/7660-change-header-action-trigger-element...cee-chen:eui:datagrid/7660-change-header-action-trigger-element?w=1

I added an optional render prop to the cell wrapper that passes back whether a focus trap has been rendered or not - not sure if that feels better or worse than a onInteractivesFound callback to you, no strong feelings here either way.

The one big change I want to push for is that we cannot use only title for the aria-label. displayAsText is an optional prop and IMO we cannot rely on it being the thing we present to users - it's far more likely to fall back to a very human unreadable id string (which can be incredibly long for some production datagrids, e.g. a random hash etc).

The thing I'm not sure I'm testing right is I'm getting a bunch of really weird VoiceOver behavior when setting aria-labelledby and aria-describedby - it repeats child content at the end unless I explicitly set aria-hidden on it, which is frustrating. It also repeats the sorting text twice for no discernible reason that I can tell. I might need Trevor to see how JAWS and NVDA behaves and if VO is the incorrect edge case in this scenario.

[mgadewoll]

🗒️ We agreed to continue with the suggestion from @cee-chen for now and run tests to see the behavior in JAWS and NVDA first before trying to fix for VoiceOver specifically in case the others are fine (since the majority usage is not in VoiceOver)

I pinged @1Copenut for another review with this current state.

[mgadewoll]

Based on the test results done by @1Copenut we needed to update the code to ensure a better screen reader output. I tried to achieve what we initially had as output with the latest code status in this commit.

One slightly spicy point of change here: We use displayAsText as aria-label if available to ensure we have a text-only label. If it's not passed, then the content is read after the cell description. We can't use the arbitrary content as label since we don't know what it will be.

That means the following output for an example column for "Name" (on VoiceOver, macOS):

// with `display` for interactive content and with `displayAsText` additionally
"Name,  Press the Enter key to interact with this cell's contents. table 6 columns, 11 rows"

// with `display` for interactive content but without `displayAsText`
"Press the Enter key to interact with this cell's contents. name table 5 columns, 11 rows"

[mgadewoll]

Based on Windows screen reader testing, the latest update (commit) links the hint messages via aria-describedby to the cell element as that's the only way to ensure it's read out without duplication on Windows screen readers.
For VoiceOver it does not read out the cell on re-focus after exiting an interactive cell. We removed the aria-live workaround as we optimize for Windows (majority of usage) and accept that there is no feedback given on leaving a cell.

[1Copenut]

@mgadewoll I took this for another drive with NVDA and JAWS. I'm hearing a lot of repeated information both when the heading has focus and when I move to cells in the column. Here's a brief recap:

1. When focus in on a heading cell, I'm hearing the cell name and "Additional information" which is the aria-label for the icon. I was really looking for hearing the cell name only.
2. There's an empty aria-describedby P tag that is referenced by the heading cell.
3. There's a live region that announces the prompt to press Enter to interact with the cell's contents.
4. Column cells that reference the heading cell announce the entire name + "Additional Information" and the Enter prompt. I would expect these to only announce the name.

I'm attaching a screenshot with some notes and highlights of how I think this could be improved. Please ping me with any questions.

[mgadewoll]

@mgadewoll I took this for another drive with NVDA and JAWS. I'm hearing a lot of repeated information both when the heading has focus and when I move to cells in the column. Here's a brief recap:

1. When focus in on a heading cell, I'm hearing the cell name and "Additional information" which is the aria-label for the icon. I was really looking for hearing the cell name only.
2. There's an empty aria-describedby P tag that is referenced by the heading cell.
3. There's a live region that announces the prompt to press Enter to interact with the cell's contents.
4. Column cells that reference the heading cell announce the entire name + "Additional Information" and the Enter prompt. I would expect these to only announce the name.

I'm attaching a screenshot with some notes and highlights of how I think this could be improved. Please ping me with any questions.

@1Copenut Thanks for the check! I agree that we should not have duplicate or unexpected information being read. I'll check how we can bring it back to the initial experience we had for your first check.

Some additional information for context:

• we have to label the header cell with all its content (instead of just the title span) as the content can be passed and hence is unknown to us
• the empty p tag is the optional sorting description text (not new)
• the live region was added to support the additional "exited cell" functionality

[cee-chen]

SR code changes LGTM! If @1Copenut re-reviews this and finds it works in screen readers without duplicated copy, let's ship it!! 🚢

[1Copenut]

I took another pass and my notes weren't as clear as I'd hoped, causing a slight regression. I put a 30 minute invite together to live review markup and that should get us to the finish line quickly.

[1Copenut]

@mgadewoll && @cee-chen I just retested with NVDA and JAWS, and wow. This is the experience I was hoping for. The native traversal method in Forms/Focus Mode gives clear direction for cell entry and exit, and handles the nested content perfectly. The tabular traversal method (my guess is this is a secondary means of traversal) is pretty solid and gives enough clues that users can easily drop into content and are automatically moved to Forms/Focus mode when they exit a cell using ESC. I'm very happy with this latest iteration! 🚢 it.

[cee-chen]

🎉 Final code diff readthrough looks amazing!! Really great work on this Lene, and thanks for the quick responses to all my feedback!

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 98fed3a

History

• 💚 Build #2671 succeeded c04b1a7
• 💚 Build #2670 succeeded 793822b
• 💚 Build #2642 succeeded ce11b57
• 💚 Build #2598 succeeded f33f981
• 💚 Build #2591 succeeded 837b58a
• 💔 Build #2585 failed 770ed5a