[EuiModal] Adjust position of close button

[acstll]

Summary

Should fix #7222

The close button is placed right after the EuiHeader, if used. Otherwise is placed last.

When this is ready, I'll apply the same logic for #7223

I need to figure out how to properly test the screen reader part.

QA

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
• 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)

[acstll]

Reading the changelog docs now, will fix 🤓

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_8202/
• Storybook: https://eui.elastic.co/pr_8202/storybook
• New docs: https://eui.elastic.co/pr_8202/new-docs

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: 7715ebe

Failed CI Steps

• 🔨 EUI pull request test

cc @acstll

[cee-chen]

I'm sorry to be a blocker on your first PR 🙈 But I'm going to ask us to pause on implementing this PR. I don't think I fully thought through Trevor's proposal when he opened #7222, and I absolutely shouldn't have added the low hanging fruit tag to it. I apologize, that's totally on me.

My concern with this the fallibility of iterating through React.Children:

• It doesn't handle recursively checking children (e.g. if a consumer uses a <Context.Provider> wrapper around <EuiModalHeader>)
• It doesn't scale well / has potential performance issues with components with many rerenders and many children (admittedly not likely in production with EuiModal compared to EuiFlyout, but nevertheless not ideal or a best practice)

While those aren't necessarily dealbreaking tradeoffs by themselves, what tips me towards blocking this PR is that I don't feel that the accessibility gain from this, if any, is worthwhile. The inciting feature that Trevor was using an example when he wrote up the original issue was for EuiCallOut, which isn't the same use case at all in terms of screen reader experience:

• EuiModals already have an aria-labelledby pattern that points to EuiModalHeader/EuiModalTitle, so modals should already have their titles read out automatically on open, unlike callouts
• EuiModals have a focus trap and separate context / dialog role, so it's important that users be able to quickly exit from them if necessary if they did not mean to open this interrupting dialog

With those considerations in mind, and if our accessibility specialists @alexwizp and @L1nBra agree that EuiModal does not require this change, I personally vote we should instead close #7222 as not planned and leave the close button in its current DOM order.

[acstll]

@cee-chen no apology necessary! Thanks for taking a look so quickly and for the extensive explanation 🙂

I wasn't sure about using Children but I saw it's being used in a couple of places so I thought it would be OK. But I didn't think about the possibility of EuiHeader being wrapped, my bad!

While those aren't necessarily dealbreaking tradeoffs by themselves, what tips me towards blocking this PR is that I don't feel that the accessibility gain from this, if any, is worthwhile.

I completely agree with this. I'm also not keen on this implementation and would rather just close this PR (or go with a simpler solution).

From my experience with role=dialog and screen readers, I got the best results recently with the heading element (e.g. h2) being the very first child of the dialog element, unwrapped. And the close button always last. Maybe that's something for the accessibility experts to consider, to leave everything as it is but only place the button last? It would then be a very simple change.

[cee-chen]

From my experience with role=dialog and screen readers, I got the best results recently with the heading element (e.g. h2) being the very first child of the dialog element, unwrapped. And the close button always last. Maybe that's something for the accessibility experts to consider, to leave everything as it is but only place the button last? It would then be a very simple change.

Interesting! As long as we convey to screen readers on dialog open that the Escape key works to immediately close out modals/flyouts/popovers etc. immediately without having to navigate all the way to the close button, I'd be good with that - but I'll also defer to any a11y experts or user research on that decision.

[acstll]

Closing in favor of #8230