Fixes #7303 - Panel: When IsHiddenOnDismiss is true, Focus does not automatically enter Panel and is not returned when Panel is dismissed by LevnLime · Pull Request #7362 · microsoft/fluentui

LevnLime · 2018-12-11T23:41:56Z

Description of changes

Please see Issue#7303 for full background explanation of the issue.

FocusTrapZone already has a "forceFocusInsideTrap" property, which the Panel sets and unsets to implement isHiddenOnDismiss. However, the feature is incomplete.
This change adds logic so that when the forceFocusInsideTrap property transitions from false to true, then bring focus into the FocusTrapZone just like if we were mounting the FocusTrapZone.
Conversely, when the forceFocusInsideTrap property transitions from true to false, return focus back outside the FocusTrapZone just like we are unmounting the FocusTrapZone.

There is also a minor change to Panel to default forceFocusInsideTrap to true like the description claims it should be.

Focus areas to test

Panel when isHiddenOnDismiss is set
FocusTrapZone

Known issues observed, but NOT being addressed by this change

Having forceFocusInsideTrap set to false will NOT prevent focus from being brought into the FocusTrapZone when the zone is mounted. The "disableFirstFocus" property can do that
If "disableFirstFocus" is set to true, then transitioning forceFocusInsideTrap from false to true will not bring focus into the trap zone.

Microsoft Reviewers: Open in CodeFlow

…ideTrap changes

packages/office-ui-fabric-react/src/components/Panel/Panel.base.tsx

size-auditor · 2019-01-02T20:10:21Z

Component	Baseline Size	PR Size	Size Change	Bundle Size	Tolerance Level
ActivityItem	162559	162919	360	🔺	✔️
MessageBar	227968	228328	360	🔺	✔️
Pickers	313578	313938	360	🔺	✔️
Dialog	234771	235131	360	🔺	✔️
PersonaCoin	156309	156669	360	🔺	✔️
Persona	156242	156602	360	🔺	✔️
Panel	232131	232491	360	🔺	✔️
DocumentCard	267969	268329	360	🔺	✔️
Nav	228309	228669	360	🔺	✔️
Modal	110235	110595	360	🔺	✔️
Dropdown	261386	261746	360	🔺	✔️
DatePicker	241091	241451	360	🔺	✔️
ExtendedPicker	207009	207369	360	🔺	✔️
Tooltip	127074	127434	360	🔺	✔️
Facepile	255211	255571	360	🔺	✔️
KeytipLayer	204784	205144	360	🔺	✔️
FloatingPicker	338750	339110	360	🔺	✔️
Keytip	189139	189499	360	🔺	✔️
FocusTrapZone	66681	67041	360	🔺	✔️
Grid	222028	222388	360	🔺	✔️
Pivot	227504	227864	360	🔺	✔️
HoverCard	131908	132268	360	🔺	✔️
ContextualMenu	186233	186593	360	🔺	✔️
Callout	120814	121174	360	🔺	✔️
CommandBar	239831	240191	360	🔺	✔️
ComboBox	277830	278190	360	🔺	✔️
SearchBox	224943	225303	360	🔺	✔️
SelectedItemsList	277663	278023	360	🔺	✔️
Breadcrumb	239537	239897	360	🔺	✔️
Coachmark	129793	130153	360	🔺	✔️
Button	218635	218995	360	🔺	✔️
TeachingBubble	229818	230178	360	🔺	✔️
SwatchColorPicker	236974	237334	360	🔺	✔️
SpinButton	232238	232598	360	🔺	✔️

atneik

Looks good to me. But I am not sure about updating the default for forceFocusInsideTrap to true, as it might be a breaking change. Even though it might be a right thing to do, we might just have to wait until it is depreciated.

jspurlin · 2019-01-02T23:32:34Z

packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx

+
+    if (!prevForceFocusInsideTrap && newForceFocusInsideTrap) {
+      // Transition from forceFocusInsideTrap disabled to enabled. Emulate what happens when a FocusTrapZone gets mounted
+      FocusTrapZone._focusStack.push(this);


FocusTrapZone._focusStack.push(this); [](start = 6, length = 37)

Seems a little odd that we are not pushing every time we are going to bringFocusIntoZone. Is that expected?

I looked closer into this. We do push every time before going into bringFocusIntoZone; however, the first _focusStack.push happens in componentWillMount(), so it's not immediately noticeable.
I looked through the code and I don't think there's a reason it NEEDS to be in componentWillMount() as opposed to componentDidMount(), so I'm bringing the _focusStack.push into the bringFocusIntoZone helper function.

jspurlin · 2019-01-02T23:34:20Z

packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx

+        return this !== value;
+      });
+      this._returnFocusToInitiator();
+    }


This block of code is exactly the same as what's in componentWillUnmount. This should be pulled out into a helper function that both places call

The helper function that both places call is this._returnFocusToInitiator().
I wasn't sure whether to include the FocusTrapStack logic in it or not, but I think I ultimately decided against it because I felt like "returnFocusToInitiator" was doing more than what it's name describes.
I can definitely revisit this decision though. If you feel like "bringFocusIntoZone" and "returnFocusToInitiator" implies the FocusTrapStack logic, then I can bring those pieces of logic into the common function

Ok, I'm bringing the _focusStack removing logic into returnFocusToInitator() as well

LevnLime

Responded to code review comments

LevnLime · 2019-01-05T02:48:05Z

packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx

+        return this !== value;
+      });
+      this._returnFocusToInitiator();
+    }


The helper function that both places call is this._returnFocusToInitiator().
I wasn't sure whether to include the FocusTrapStack logic in it or not, but I think I ultimately decided against it because I felt like "returnFocusToInitiator" was doing more than what it's name describes.
I can definitely revisit this decision though. If you feel like "bringFocusIntoZone" and "returnFocusToInitiator" implies the FocusTrapStack logic, then I can bring those pieces of logic into the common function

LevnLime · 2019-01-07T22:51:09Z

packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx

+
+    if (!prevForceFocusInsideTrap && newForceFocusInsideTrap) {
+      // Transition from forceFocusInsideTrap disabled to enabled. Emulate what happens when a FocusTrapZone gets mounted
+      FocusTrapZone._focusStack.push(this);


I looked closer into this. We do push every time before going into bringFocusIntoZone; however, the first _focusStack.push happens in componentWillMount(), so it's not immediately noticeable.
I looked through the code and I don't think there's a reason it NEEDS to be in componentWillMount() as opposed to componentDidMount(), so I'm bringing the _focusStack.push into the bringFocusIntoZone helper function.

LevnLime · 2019-01-07T22:51:41Z

packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx

+        return this !== value;
+      });
+      this._returnFocusToInitiator();
+    }


Ok, I'm bringing the _focusStack removing logic into returnFocusToInitator() as well

packages/office-ui-fabric-react/src/components/Panel/Panel.base.tsx

jspurlin

msft-github-bot · 2019-01-09T13:36:52Z

🎉office-ui-fabric-react@v6.122.0 has been released which incorporates this pull request.:tada:

Handy links:

Burney Jia added 2 commits December 11, 2018 14:33

Bring focus into trap (and return to outside trap) when forceFocusIns…

1a6f81d

…ideTrap changes

Changes file

fc16eb0

msft-github-bot assigned kkjeer Dec 11, 2018

Vitalius1 requested a review from dzearing December 12, 2018 06:15

Vitalius1 reviewed Dec 12, 2018

View reviewed changes

packages/office-ui-fabric-react/src/components/Panel/Panel.base.tsx Outdated Show resolved Hide resolved

kkjeer requested a review from jspurlin January 2, 2019 19:31

API Changes File

9746e06

atneik reviewed Jan 2, 2019

View reviewed changes

jspurlin reviewed Jan 2, 2019

View reviewed changes

cliffkoh added the Needs: Author Feedback label Jan 7, 2019

msft-github-bot removed the Needs: Author Feedback label Jan 7, 2019

Code review comments

263451f

LevnLime force-pushed the yubojia/focusTrapZone branch from 50454e4 to 263451f Compare January 7, 2019 23:06

LevnLime commented Jan 7, 2019

View reviewed changes

Remove componentWillMount empty block

3dfa2e4

jspurlin approved these changes Jan 8, 2019

View reviewed changes

kkjeer merged commit ccbd24d into microsoft:master Jan 8, 2019

LevnLime deleted the yubojia/focusTrapZone branch January 9, 2019 23:54

sophieH29 mentioned this pull request Aug 13, 2019

feat(FocusTrapZone): upgrade to the latest version from fabric-ui microsoft/fluent-ui-react#1790

Merged

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Conversation

LevnLime commented Dec 11, 2018 • edited by fabricteam Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Focus areas to test

Known issues observed, but NOT being addressed by this change

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

size-auditor bot commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atneik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LevnLime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

msft-github-bot commented Jan 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LevnLime commented Dec 11, 2018 •

edited by fabricteam

Loading

size-auditor bot commented Jan 2, 2019 •

edited

Loading