Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(FocusTrapZone): Do not force focus inside focus trap zone on outside focus #1930

Merged
merged 3 commits into from
Sep 13, 2019

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Sep 13, 2019

Popup in Menu scenario was broken due to a recent focus trap zone update to the Fabric latest version.

For now, switching it back to Stardust default behavior, we will investigate later how to align.

Repro: https://codesandbox.io/s/stardust-ui-example-yjnqv

@sophieH29 sophieH29 added 🚀 ready for review accessibility All the Accessibility tasks and bugs should be tagged with this. labels Sep 13, 2019
@DustyTheBot
Copy link
Collaborator

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@98b8229). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1930   +/-   ##
========================================
  Coverage          ?   70.5%           
========================================
  Files             ?     884           
  Lines             ?    7794           
  Branches          ?    2254           
========================================
  Hits              ?    5495           
  Misses            ?    2289           
  Partials          ?      10
Impacted Files Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx 71.71% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98b8229...b3ac9e1. Read the comment docs.

@jurokapsiar
Copy link
Contributor

Keeping some context for the future here:

This behavior changed between different versions of (Fabric) FocusTrapZone. So reverting to the previous behavior as it breaks some of the scenarios in Stardust.

To use the default Fabric behavior, we can still use

trapFocus={ {
  forceFocusInsideTrapOnOutsideFocus: true
} }

We should probably use false for popup by default and true for dialog (?) but it requires some testing.

@jurokapsiar jurokapsiar merged commit f70cabf into master Sep 13, 2019
@jurokapsiar jurokapsiar deleted the fix/focus-trap-zone-on-outside-focus branch September 13, 2019 13:01
layershifter pushed a commit that referenced this pull request Sep 13, 2019
…ide focus (#1930)

* fix(FocusTrapZone): Do not force focus inside focus trap zone on outside focus

* changelog

(cherry picked from commit f70cabf)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility All the Accessibility tasks and bugs should be tagged with this. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants