HandleInteractiveChildren already iterates through tabbable children here:

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.

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.

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.

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.

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

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>

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.

🗒️ 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.

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"

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.