Skip to content

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

Merged
cee-chen merged 5 commits intoelastic:mainfrom
cee-chen:cross-frame-focus-traps
Jul 10, 2023
Merged

[EuiFocusTrap] Default to not trapping focus across iframes#6908
cee-chen merged 5 commits intoelastic:mainfrom
cee-chen:cross-frame-focus-traps

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Jul 5, 2023 •
edited
Loading

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

@cee-chen 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 cee-chen added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Jul 5, 2023
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 5, 2023

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

1 similar comment
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 6, 2023

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

@cee-chen cee-chen force-pushed the cross-frame-focus-traps branch from 0f38c16 to b75bf8f Compare July 6, 2023 17:25
@cee-chen cee-chen marked this pull request as ready for review July 6, 2023 17:41
@cee-chen cee-chen requested a review from tkajtoch July 6, 2023 17:41
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 6, 2023

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

@cee-chen

This comment was marked as resolved.

Sign in to view
@cee-chen cee-chen force-pushed the cross-frame-focus-traps branch from b75bf8f to bd7655d Compare July 10, 2023 18:44
cee-chen
cee-chen commented Jul 10, 2023
View reviewed changes
package.json
"**/prismjs": "1.27.0",
"**/react": "^17.0.0"
"**/react": "^17.0.0",
"react-focus-lock": "^2.9.5"
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

package.json
"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",
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown

kibanamachine commented Jul 10, 2023

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

tkajtoch
tkajtoch approved these changes Jul 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

cee-chen commented Jul 10, 2023

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.

@cee-chen cee-chen enabled auto-merge (squash) July 10, 2023 19:22
@cee-chen cee-chen merged commit efb2360 into elastic:main Jul 10, 2023
@cee-chen cee-chen deleted the cross-frame-focus-traps branch July 10, 2023 19:22
@cee-chen cee-chen mentioned this pull request Jul 11, 2023
Merged
cee-chen added a commit to elastic/kibana that referenced this pull request Jul 14, 2023
@cee-chen @kibanamachine
Upgrade EUI to v84.0.0 (#161716)
fc9ac7a 
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tkajtoch tkajtoch tkajtoch approved these changes

Assignees

No one assigned

Labels

breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cee-chen @kibanamachine @tkajtoch