[EuiFocusTrap] Default to not trapping focus across iframes #6908

cee-chen · 2023-07-05T20:50:22Z

Summary

Upgrades to latest react-focus-on which allows setting crossFrame=false (theKashey/react-focus-on#72).

Example of previous cross frame behavior:

Go to https://codesandbox.io/s/qzf8zp?file=/demo.js
Click the "Show flyout" button
Click into the code panel/frame and try to type. Notice that it won't let you (at first at least, it may intermittently work if you click enough but the first click+type never does)

QA

I wasn't able to test this working behavior locally for whatever bizarre reason, so I did a pre-release RC and created a demo CodeSandbox with the RC:

Go to https://codesandbox.io/s/red-grass-zqrgvj?file=/demo.js
Click the blank EuiFieldText and tab back and forth, confirm tabbing is trapped and does not go down to the console
Click into the code editor panel and try to type. Confirm it allows you to immediately
Clicking the "Toggle crossFrame" button and trying to type in the code editor should have the old non-working behavior

General checklist

Checked in Chrome, Safari, Edge, and Firefox
- ⚠️ This doesn't work as well in Safari as it does Chrome/FF, but I'm not sure there's anything we can do about that 😖 I opened an issue for it to see if others can repro: crossFrame={false} has cross-browser Safari issues theKashey/react-focus-lock#249
Added documentation
[ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
- Our props table appears to exclude ReactFocusOn's props, so I documented the behavior manually instead
Checked Code Sandbox works for any docs examples
[ ] Added or updated jest and cypress tests
- Honestly, I couldn't figure out an easy and non-headache-y way to test iframes via Cypress (and Jest would be right out for multiple reasons). Since EUI iframe usage is a bit of an edge case and the core behavior should theoretically be managed by react-focus-on, not us, I'm opting to skip writing a test for this 🤷
- If we absolutely had to, I think I'd personally suggest writing an actual E2E (not Cypress) smoke test for a CodeSandbox demo.
A changelog entry exists and is marked appropriately

kibanamachine · 2023-07-05T21:17:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6908/

kibanamachine · 2023-07-06T02:02:26Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6908/

kibanamachine · 2023-07-06T17:49:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6908/

- force focus-lock 2.9.5 for now until Anton has time to upgrade

cee-chen · 2023-07-10T18:45:46Z

package.json

@@ -56,7 +56,8 @@
  },
  "resolutions": {
    "**/prismjs": "1.27.0",
-    "**/react": "^17.0.0"
+    "**/react": "^17.0.0",
+    "react-focus-lock": "^2.9.5"


This resolution is necessary as latest react-focus-on still points to 2.9.4 which doesn't yet have the react-focus-lock fix we need (theKashey/react-focus-lock#249). Once react-focus-on updates, we can remove this resolution

cee-chen · 2023-07-10T18:47:50Z

package.json

@@ -81,7 +82,7 @@
    "react-beautiful-dnd": "^13.1.0",
    "react-dropzone": "^11.5.3",
    "react-element-to-jsx-string": "^14.3.4",
-    "react-focus-on": "^3.7.0",
+    "react-focus-on": "^3.9.1",


Upgrading to 3.9.1 (from 3.9.0) should contain the fix necessary to resolve #6848 - @tkajtoch, do you think we need to write any specific tests or storybooks to ensure this is the case, or would you be OK with just merging and testing in CodeSandbox after?

kibanamachine · 2023-07-10T19:08:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6908/

tkajtoch

The changes look and work great! The cross-frame focus trap is certainly fixed now and I'm happy to merge this PR as is, but I noticed another strange behavior in react-focus-lock (I think?) when focus trap is enabled and crossFrame=true. See the button getting focus styles when I'm clicking on the elements outside the app iframe.

Screen.Recording.2023-07-10.at.21.02.17.mov

cee-chen · 2023-07-10T19:20:03Z

Thanks Tomasz! That might be something to flag/file with the core react-focus-lock library - for the purposes of @LucaWintergerst's usage, they only care that focus on outside-iframe items isn't prevented.

## Summary `eui@83.1.0` ⏩ `eui@84.0.0` --- ## [`84.0.0`](https://github.com/elastic/eui/tree/v84.0.0) - Updated `EuiDualRange`'s `minInputProps` and `maxInputProps` to support passing more props to underlying inputs ([#6902](elastic/eui#6902)) - `EuiFocusTrap` now supports configuring cross-iframe focus trapping via the `crossFrame` prop ([#6908](elastic/eui#6908)) **Bug fixes** - Fixed `EuiFilterButton` icon display ([#6900](elastic/eui#6900)) - Fixed `EuiCombobox` compressed plain text display ([#6910](elastic/eui#6910)) - Fixed visual appearance of collapse buttons on collapsible `EuiResizablePanel`s ([#6926](elastic/eui#6926)) **Breaking changes** - `EuiFocusTrap` now defaults to *not* trapping focus across iframes ([#6908](elastic/eui#6908)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen force-pushed the cross-frame-focus-traps branch 2 times, most recently from 1284bba to 09d836d Compare July 5, 2023 20:51

cee-chen added the breaking change label Jul 5, 2023

cee-chen force-pushed the cross-frame-focus-traps branch from 0f38c16 to b75bf8f Compare July 6, 2023 17:25

cee-chen marked this pull request as ready for review July 6, 2023 17:41

cee-chen requested a review from tkajtoch July 6, 2023 17:41

This comment was marked as resolved.

Sign in to view

cee-chen added 5 commits July 10, 2023 11:41

Upgrade react-focus-on to latest with crossFrame support

752c134

Add documentation

6f689b1

changelog

6e37fc9

Add storybook for testing

c7dc325

Upgrade to latest react-focus packages w/ various fixes

bd7655d

- force focus-lock 2.9.5 for now until Anton has time to upgrade

cee-chen force-pushed the cross-frame-focus-traps branch from b75bf8f to bd7655d Compare July 10, 2023 18:44

cee-chen commented Jul 10, 2023

View reviewed changes

tkajtoch approved these changes Jul 10, 2023

View reviewed changes

cee-chen enabled auto-merge (squash) July 10, 2023 19:22

cee-chen merged commit efb2360 into elastic:main Jul 10, 2023

cee-chen deleted the cross-frame-focus-traps branch July 10, 2023 19:22

cee-chen mentioned this pull request Jul 11, 2023

Upgrade EUI to v84.0.0 elastic/kibana#161716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiFocusTrap] Default to not trapping focus across iframes #6908

[EuiFocusTrap] Default to not trapping focus across iframes #6908

cee-chen commented Jul 5, 2023 •

edited

Loading

kibanamachine commented Jul 5, 2023

kibanamachine commented Jul 6, 2023

kibanamachine commented Jul 6, 2023

This comment was marked as resolved.

cee-chen Jul 10, 2023

cee-chen Jul 10, 2023 •

edited

Loading

kibanamachine commented Jul 10, 2023

tkajtoch left a comment

cee-chen commented Jul 10, 2023

[EuiFocusTrap] Default to not trapping focus across iframes #6908

[EuiFocusTrap] Default to not trapping focus across iframes #6908

Conversation

cee-chen commented Jul 5, 2023 • edited Loading

Summary

QA

General checklist

kibanamachine commented Jul 5, 2023

kibanamachine commented Jul 6, 2023

kibanamachine commented Jul 6, 2023

This comment was marked as resolved.

cee-chen Jul 10, 2023

Choose a reason for hiding this comment

cee-chen Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Jul 10, 2023

tkajtoch left a comment

Choose a reason for hiding this comment

cee-chen commented Jul 10, 2023

cee-chen commented Jul 5, 2023 •

edited

Loading

cee-chen Jul 10, 2023 •

edited

Loading