fix(EuiInputPopover): close popover on Tab, not Shift Tab

[weronikaolejniczak]

Summary

If disableFocusTrap is false, regardless of ownFocus, the active item is the last tabbable item in the EuiInputPopover overlay and if we press Shift + Tab, the popover will close. It's expected to close only on pressing Tab because it will mean circling back to the first tabbable element. But in the other direction, it should not close.

Why are we making this change?

Resolves #8949

This PR comes out of a triage, so I kept the changes to the minimum, only to address the issue that the consumer is struggling with.

That being said, the keyboard navigation in this component feels weird to me. It seems that only some combination of disableTrapFocus and ownFocus produces the expected result. From my perspective, it's best if the keyboard navigation in this component is reworked.

⚠️ I also noticed on Esc nothing happens.

If you have an idea how to best handle the popover, I'm open to suggestions! Otherwise, we can address it separately whenever the time allows 😄

Screenshots

	Prod	Local
disableTrapFocus={true}, ownFocus={true}
disableTrapFocus={true}, ownFocus={false}
disableTrapFocus={false}, ownFocus={true}
disableTrapFocus={false}, ownFocus={false}

Impact to users

🟠 It's not a breaking change BUT it's a difference in component behavior which may affect some use cases detrimentally, and especially automated tests.

This change should prompt a cleanup of: elastic/kibana#230852 (comment)

QA

Specific checklist

• Verify in Storybook that the EuiInputPopover doesn't close on Shift + Tab when the last interactive element is active

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in both MacOS and Windows high contrast modes
    • (emulate forced colors if you do not have access to a Windows machine.)
  • 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)

[mgadewoll]

@weronikaolejniczak Thanks for raising the mentioned issues. I had a look at what's going on and you're right, the keyboard behavior is currently a bit weird. Additionally I'm a bit confused why EuiInputPopover has it's own EuiFocusTrap while the internal EuiPopover already has one, but that's a different topic 🤷‍♀️

🗒️ Anyway, from what I understand these are the focus behaviors base on the two props disableFocusTrap and ownFocus:

• disableFocusTrap=true & ownFocus=true

  • panel has a focus stop (ownFocus)
  • focus is not trapped (disableFocusTrap disables the focus trap in EuiInputPopover)

• disableFocusTrap=false & ownFocus=false

  • panel has no focus stop (due to ownFocus=false)
  • focus is not trapped (ownFocus=false disables the focus trap in EuiPopover)

• disableFocusTrap=false & ownFocus=true

  • panel has a focus stop (due to ownFocus=true)
  • focus is trapped (due to disableFocusTrap=false)

• disableFocusTrap=true & ownFocus=false

  • panel has no focus stop (due to ownFocus=false)
  • focus is not trapped (ownFocus=false disables the focus trap in EuiPopover, disableFocusTrap=true disables the trap in EuiInputPopover)

---

🐞 And concerning the keyboard navigation I saw 3 main issues:

1. Shift + Tab on the last item closes the popover

   • This is fixed by your update for disableFocusTrap=false but it also happens for disableFocusTrap=true. Looks like we need to update the function to apply to all cases of !ownFocus

2. Tab from the first focusable item closes the popover unexpectedly when disableFocusTrap=true & ownFocus=false

   • We should also add tabbingFromFirstItemInPopover && e.shiftKey to the closing condition

3. As mentioned by you: Escape doesn't work as expected

   • This looks to be use case specific. The stories have a custom onFocus handler which opens the popover on focus. So when the popover closes and returns the focus, the focus on the input triggers the popover to open again as it's simply reacting to the isOpen prop. 💀

[mgadewoll]

ℹ️ @weronikaolejniczak I pushed up changes to address the issues mentioned here.

• ensure Tab and Tab + Shift don't close the popover from last or first focusable item
• update the onFocus handler in our stories to prevent reopening popovers after closing the popover

[weronikaolejniczak]

@mgadewoll LGTM 🟢 2 small things:

1. A test is failing:

/app/packages/eui/cypress/screenshots/components/popover/input_popover.spec.tsx/     (1280x633)
--
  | EuiPopover -- focustab management -- with focus trap disabled -- automatically c
  | loses the popover when users tab from anywhere in the popover

2. disableTrapFocus={false} is not working as I'd expect it to, i.e. doesn't trap focus:
   

I think we can investigate the trap focus separately.

[mgadewoll]

2. disableTrapFocus={false} is not working as I'd expect it to, i.e. doesn't trap focus:

@weronikaolejniczak When disableTrapFocus=false and ownFocus=false it shouldn't trap focus, as the focus trap is disabled in EuiPopover due to the ownFocus=false 😅 (code)

[weronikaolejniczak]

As you wrote above:

disableFocusTrap=false & ownFocus=false

• panel has no focus stop (due to ownFocus=false)
• focus is not trapped (ownFocus=false disables the focus trap in EuiPopover)

I can imagine if it's confusing to me then it could be more confusing to the consumer. That being said, it seems like the expected behavior.

[mgadewoll]

As you wrote above:

disableFocusTrap=false & ownFocus=false

• panel has no focus stop (due to ownFocus=false)
• focus is not trapped (ownFocus=false disables the focus trap in EuiPopover)

I can imagine if it's confusing to me then it could be more confusing to the consumer. That being said, it seems like the expected behavior.

Yeah, I'm also confused by it 😅 I still don't understand why it has duplicate FocusTraps. Probably something we could look into in the future on how to make this better.

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 3827ff7
• Documentation website
• Storybook

History

• 💚 Build #1071 succeeded e983fa5
• 💚 Build #1046 succeeded f0c2a0f
• 💚 Build #1040 succeeded 4a40697

cc @weronikaolejniczak @mgadewoll

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 3827ff7

History

• 💔 Build #4593 failed e983fa5
• 💚 Build #4574 succeeded f0c2a0f
• 💚 Build #4569 succeeded 4a40697

cc @weronikaolejniczak @mgadewoll

[weronikaolejniczak]

@mgadewoll I reviewed the recent changes: 3827ff7 and 1ecc1c2, I believe both make sense.

refactor: remove support for ownFocus on EuiDualRange inputPopoverProps

• this is a bit of a spicy decision, but it's meant to prevent issues with unexpected focus traps, as the popover doesn't have focusable content

test: remove cypress test

• the test asserts an unexpected behavior. Checking EuiDualRange, there are no tab stops inside the popover and even if, it would be wrong to close the popover on tab

[mgadewoll]

@weronikaolejniczak Sorry, I hadn't posted my comment about the changes yet. You're too fast 😄
Glad you agree with the changes 👍

Anyway here still the comments:

1. removed the failing cypress test
   • the test was already commented as potentially wrong. Imho, the test asserts a behavior that's unexpected. I checked on the EuiDualRange behavior and usages and didn't see an issue in removing the test.
2. when checking EuiDualRange I noticed that there would be an issue with focus traps if ownFocus=true as the popover doesn't have focusable content. To prevent confusion I chose to remove support for ownFocus on EuiDualRange (there are no usages of it across Kibana or Cloud)
   • the changes are currently running against Kibana CI as sanity check
3. rebased with main (because of wanting to run it in latest Kibana)

ℹ️ I'm currently still waiting for the Kibana CI to finish.

[mgadewoll]

🟢 The check in Kibana CI succeeded and the overall changes in this PR ensure a more predictable keyboard navigation for EuiInputPopover.