Skip to content

Fixes #7303 - Panel: When IsHiddenOnDismiss is true, Focus does not automatically enter Panel and is not returned when Panel is dismissed #7362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kkjeer merged 5 commits into from
Jan 8, 2019
Merged

Fixes #7303 - Panel: When IsHiddenOnDismiss is true, Focus does not automatically enter Panel and is not returned when Panel is dismissed #7362

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a6f81d
Bring focus into trap (and return to outside trap) when forceFocusIns…
Dec 11, 2018
fc16eb0
Changes file
Dec 11, 2018
9746e06
API Changes File
Jan 2, 2019
263451f
Code review comments
Jan 7, 2019
3dfa2e4
Remove componentWillMount empty block
Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/yubojia-focusTrapZone_2018-12-11-22-54.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "FocusTrapZone - Make forceFocusInsideTrap prop changes modify focus",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "yubojia@microsoft.com"
}
64 changes: 44 additions & 20 deletions packages/office-ui-fabric-react/src/components/FocusTrapZone/FocusTrapZone.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,7 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
}

public componentDidMount(): void {
const { elementToFocusOnDismiss, disableFirstFocus = false } = this.props;

this._previouslyFocusedElementOutsideTrapZone = elementToFocusOnDismiss
? elementToFocusOnDismiss
: (document.activeElement as HTMLElement);
if (!elementContains(this._root.current, this._previouslyFocusedElementOutsideTrapZone) && !disableFirstFocus) {
this.focus();
}

this._bringFocusIntoZone();
this._updateEventHandlers(this.props);
}

Expand All @@ -47,23 +39,30 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
this._updateEventHandlers(nextProps);
}

public componentWillUnmount(): void {
const { ignoreExternalFocusing } = this.props;
public componentDidUpdate(prevProps: IFocusTrapZoneProps) {
const prevForceFocusInsideTrap = prevProps.forceFocusInsideTrap !== undefined ? prevProps.forceFocusInsideTrap : true;
const newForceFocusInsideTrap = this.props.forceFocusInsideTrap !== undefined ? this.props.forceFocusInsideTrap : true;

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

@jspurlin jspurlin Jan 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@LevnLime LevnLime Jan 7, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

this._bringFocusIntoZone();
} else if (prevForceFocusInsideTrap && !newForceFocusInsideTrap) {
// Transition from forceFocusInsideTrap enabled to disabled. Emulate what happens when a FocusTrapZone gets unmounted
FocusTrapZone._focusStack = FocusTrapZone._focusStack.filter((value: FocusTrapZone) => {
return this !== value;
});
this._returnFocusToInitiator();
}

@jspurlin jspurlin Jan 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@LevnLime LevnLime Jan 5, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 LevnLime Jan 7, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

}

public componentWillUnmount(): void {
this._events.dispose();
FocusTrapZone._focusStack = FocusTrapZone._focusStack.filter((value: FocusTrapZone) => {
return this !== value;
});

const activeElement = document.activeElement as HTMLElement;
if (
!ignoreExternalFocusing &&
this._previouslyFocusedElementOutsideTrapZone &&
typeof this._previouslyFocusedElementOutsideTrapZone.focus === 'function' &&
(elementContains(this._root.current, activeElement) || activeElement === document.body)
) {
focusAsync(this._previouslyFocusedElementOutsideTrapZone);
}
this._returnFocusToInitiator();
}

public render(): JSX.Element {
Expand Down Expand Up @@ -114,6 +113,31 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
}
}

private _bringFocusIntoZone(): void {
const { elementToFocusOnDismiss, disableFirstFocus = false } = this.props;

this._previouslyFocusedElementOutsideTrapZone = elementToFocusOnDismiss
? elementToFocusOnDismiss
: (document.activeElement as HTMLElement);
if (!elementContains(this._root.current, this._previouslyFocusedElementOutsideTrapZone) && !disableFirstFocus) {
this.focus();
}
}

private _returnFocusToInitiator(): void {
const { ignoreExternalFocusing } = this.props;

const activeElement = document.activeElement as HTMLElement;
if (
!ignoreExternalFocusing &&
this._previouslyFocusedElementOutsideTrapZone &&
typeof this._previouslyFocusedElementOutsideTrapZone.focus === 'function' &&
(elementContains(this._root.current, activeElement) || activeElement === document.body)
) {
focusAsync(this._previouslyFocusedElementOutsideTrapZone);
}
}

private _updateEventHandlers(newProps: IFocusTrapZoneProps): void {
const { isClickableOutsideFocusTrap = false, forceFocusInsideTrap = true } = newProps;

Expand Down
2 changes: 1 addition & 1 deletion packages/office-ui-fabric-react/src/components/Panel/Panel.base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
elementToFocusOnDismiss,
firstFocusableSelector,
focusTrapZoneProps,
forceFocusInsideTrap,
forceFocusInsideTrap = true,
Comment thread
Vitalius1 marked this conversation as resolved.
Outdated
hasCloseButton,
headerText,
headerClassName = '',
Expand Down