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 all 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"
}
2 changes: 1 addition & 1 deletion packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,7 @@ class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}>, implements I
// (undocumented)
componentDidMount(): void;
// (undocumented)
componentWillMount(): void;
componentDidUpdate(prevProps: IFocusTrapZoneProps): void;
// (undocumented)
componentWillReceiveProps(nextProps: IFocusTrapZoneProps): void;
// (undocumented)
Expand Down
74 changes: 46 additions & 28 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 @@ -21,20 +21,8 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
private _hasFocusHandler: boolean;
private _hasClickHandler: boolean;

public componentWillMount(): void {
FocusTrapZone._focusStack.push(this);
}

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,25 +35,24 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
this._updateEventHandlers(nextProps);
}

public componentWillUnmount(): void {
const { ignoreExternalFocusing } = this.props;

this._events.dispose();
FocusTrapZone._focusStack = FocusTrapZone._focusStack.filter((value: FocusTrapZone) => {
return this !== value;
});
public componentDidUpdate(prevProps: IFocusTrapZoneProps) {
const prevForceFocusInsideTrap = prevProps.forceFocusInsideTrap !== undefined ? prevProps.forceFocusInsideTrap : true;
const newForceFocusInsideTrap = this.props.forceFocusInsideTrap !== undefined ? this.props.forceFocusInsideTrap : true;

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);
if (!prevForceFocusInsideTrap && newForceFocusInsideTrap) {
// Transition from forceFocusInsideTrap disabled to enabled. Emulate what happens when a FocusTrapZone gets mounted
this._bringFocusIntoZone();
} else if (prevForceFocusInsideTrap && !newForceFocusInsideTrap) {
// Transition from forceFocusInsideTrap enabled to disabled. Emulate what happens when a FocusTrapZone gets unmounted
this._returnFocusToInitiator();
}
Copy link
Copy Markdown
Contributor

@jspurlin jspurlin Jan 2, 2019

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

Copy link
Copy Markdown
Collaborator Author

@LevnLime LevnLime Jan 5, 2019

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

Copy link
Copy Markdown
Collaborator Author

@LevnLime LevnLime Jan 7, 2019

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();
this._returnFocusToInitiator();
}

public render(): JSX.Element {
const { className, ariaLabelledBy } = this.props;
const divProps = getNativeProps(this.props, divProperties);
Expand Down Expand Up @@ -114,6 +101,37 @@ export class FocusTrapZone extends BaseComponent<IFocusTrapZoneProps, {}> implem
}
}

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

FocusTrapZone._focusStack.push(this);

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;

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);
}
}

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

Expand Down
4 changes: 2 additions & 2 deletions packages/office-ui-fabric-react/src/components/Panel/Panel.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ export interface IPanelProps extends React.HTMLAttributes<PanelBase> {
ignoreExternalFocusing?: boolean;

/**
* Indicates whether Panel should force focus inside the focus trap zone
* Indicates whether Panel should force focus inside the focus trap zone.
* If not explicitly specified, behavior aligns with FocusTrapZone's default behavior.
* Deprecated, use `focusTrapZoneProps`.
* @defaultvalue true
* @deprecated Use `focusTrapZoneProps`.
*/
forceFocusInsideTrap?: boolean;
Expand Down