FocusTrapZone: Refactor trapping behavior#8216
FocusTrapZone: Refactor trapping behavior#8216JasonGore merged 7 commits intomicrosoft:masterfrom JasonGore:jg/ftz-refactor-trapping
Conversation
… focus boundary approach.
|
|
||
| const bumperStyle: React.CSSProperties = { | ||
| pointerEvents: 'none' | ||
| }; |
There was a problem hiding this comment.
@dzearing I could use your eyes on this on potential side effects / issues with this style. I tried various other methods:
display: none
This breaks onFocus callbacks
display: contents
This breaks onFocus callbacks
position: absolute, left: -9999px, top: -9999px
This was causing weird browser scrolling issues when changing focus
|
| @@ -16,6 +15,10 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem | |||
| private static _focusStack: FocusTrapZone[] = []; | |||
There was a problem hiding this comment.
@dagoff I saw this was added in #1111 which said it was added to prevent an infinite loop and I'm wondering if it's still needed. Was the infinite looping related to key handling? I did some brief experimentation of removing it with this new trapping behavior and I couldn't trigger any infinite loops.
There was a problem hiding this comment.
The original trapping behavior (before I added focusStack) was that a FocusTrapZone would listen to all focus events, then if a focus event fired, the FocusTrapZone would check to see if the new focus target was inside the Zone. If it wasn't, the FocusTrapZone would forcibly focus something inside of it self. This would fire off another focus event, but the target would be in the Zone, so it wouldn't cause a loop if there was only one Zone. This worked fine when only 1 FocusTrapZone was on the page. With 2 or more, it was easy to end up in a state where they would fight for focus forever.
focusStack was an attempt to fix this. In short, each FocusTrapZone would push itself onto the stack at mount time and only the Zone on top of the stack is allowed to try to steal focus (or clicks).
| // If the default has been prevented, do not process keyboard events. | ||
| if (ev.isDefaultPrevented()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
There might be cases when a consumer might want to do something special in specific cases. For example, if TAB normally navigates linearly across focusable elements, but in some case(s) the consumer wants to move focus to a different element.
Removing this will break Office Online
There was a problem hiding this comment.
FocusTrapZone does not modify or affect focus behavior within it. It does not control focus within its contents anymore, so there's no longer any FTZ behavior to "stop" here.
If consumers of FTZ want to modify focus behavior, they can still do that. Nothing in FTZ is preventing key or focus handlers from working.
There was a problem hiding this comment.
Yeah, after talking offline, getting on the same page, and since the divProps are still getting applied to the div _onKeyboardHandler isn't needed and things should continue to work
* Refactor FocusTrapZone trapping behavior from key handler approach to focus boundary approach. * Change files. * Remove unnecessary file. * Minor cleanup. * Bundle size optimization. * Prevent browsers from focusing on bumpers in cases where viewport does not contain them.
|
🎉 Handy links: |
|
🎉 Handy links: |
7 participants
Pull request checklist
$ npm run changeDescription of changes
The purpose of this PR is to refactor FocusTrapZone (FTZ) focus trapping behavior to resolve outstanding and recurring issues as described in #8055 and fixes multiple open issues.
The refactor trapping behavior has been fixed from a key-handler based approach to a focus boundary approach. The fundamental difference is that now FTZ does not care how individual elements handle navigation within its boundaries. Now FTZ only cares when navigation attempts to leave FTZ boundaries. This fixes multiple issues regarding navigation issues with FTZ content. (There are some exceptions to this approach regarding nested FTZs and FocusZones.)
While refactoring trapping behavior, I also encountered the following issues / areas of improvement, which this PR also addresses:
This PR fixes multiple open FTZ navigation issues. I've also made sure recently fixed issues have not regressed. I've set up some CodePens running against an ngrok host (forgive me if these aren't working, I'll try to keep the server running. This has also made me realize it'd be nice to have PR bundles hosted for CodePens in general... #8217)
EDIT: It's really hard to keep these CodePens up and running because ngrok keeps timing out at some point. If you want to review them just let me know and I'll make sure they're running for a bit.
#7839 still seems to be an issue but I think this is PR is a really good step and should be rereviewed in the context of #8055 later. (i.e. I need an FTZ break.)
Focus areas to test
Unit test coverage increased for FTZ and focus utilities.
Microsoft Reviewers: Open in CodeFlow