Skip to content
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

Jump to bottom

Always use getDocument in FocusZone and FocusTrapZone #10187

Merged
merged 9 commits into from
Aug 20, 2019
Merged

Always use getDocument in FocusZone and FocusTrapZone #10187

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d853c2
Merge pull request #1 from OfficeDev/master
sophieH29 Nov 9, 2018
a4fd887
Merge pull request #2 from OfficeDev/master
sophieH29 Nov 13, 2018
3df47ad
merge upstream master
Aug 19, 2019
c4766bf
Use getDocument instead document in FocusZone and FocusTrapZone
Aug 19, 2019
e99a94f
Change files
Aug 19, 2019
7fd9dac
improvements after CR comments
Aug 19, 2019
36d9ad0
FTZ improvement
Aug 19, 2019
9f466ea
return document for SSR case
Aug 20, 2019
febceb0
check document for SSR case
Aug 20, 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
8 changes: 8 additions & 0 deletions change/office-ui-fabric-react-2019-08-19-17-03-13-sohuts-use-getdocument-in-focuszone.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "patch",
"comment": "Use getDocument instead document in FocusZone and FocusTrapZone",
"packageName": "office-ui-fabric-react",
"email": "sohuts@microsoft.com",
"commit": "c4766bf28c3c6fe6021e2a0f277524bcc64064b4",
"date": "2019-08-19T15:03:13.879Z"
}
20 changes: 12 additions & 8 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 @@ -6,6 +6,7 @@ import {
getFirstTabbable,
getLastTabbable,
getNextElement,
getDocument,
focusAsync,
initializeComponentRef,
on
Expand Down Expand Up @@ -64,10 +65,11 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl

public componentWillUnmount(): void {
// don't handle return focus unless forceFocusInsideTrap is true or focus is still within FocusTrapZone
const doc = getDocument(this._root.current);
if (
!this.props.disabled ||
this.props.forceFocusInsideTrap ||
!elementContains(this._root.current, document.activeElement as HTMLElement)
!elementContains(this._root.current, doc!.activeElement as HTMLElement)
) {
this._returnFocusToInitiator();
}
Expand Down Expand Up @@ -162,7 +164,8 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl
// even when it's not. Using document.activeElement is another way
// for us to be able to get what the relatedTarget without relying
// on the event
relatedTarget = document.activeElement as Element;
const doc = getDocument(this._root.current);
relatedTarget = doc!.activeElement as Element;
}

if (!elementContains(this._root.current, relatedTarget as HTMLElement)) {
Expand Down Expand Up @@ -211,9 +214,8 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl

FocusTrapZone._focusStack.push(this);

this._previouslyFocusedElementOutsideTrapZone = elementToFocusOnDismiss
? elementToFocusOnDismiss
: (document.activeElement as HTMLElement);
const doc = getDocument(this._root.current);
this._previouslyFocusedElementOutsideTrapZone = elementToFocusOnDismiss ? elementToFocusOnDismiss : (doc!.activeElement as HTMLElement);
if (!disableFirstFocus && !elementContains(this._root.current, this._previouslyFocusedElementOutsideTrapZone)) {
this.focus();
}
Expand All @@ -226,12 +228,13 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl
return this !== value;
});

const activeElement = document.activeElement as HTMLElement;
const doc = getDocument(this._root.current);
const activeElement = doc!.activeElement as HTMLElement;
if (
!ignoreExternalFocusing &&
this._previouslyFocusedElementOutsideTrapZone &&
typeof this._previouslyFocusedElementOutsideTrapZone.focus === 'function' &&
(elementContains(this._root.current, activeElement) || activeElement === document.body)
(elementContains(this._root.current, activeElement) || activeElement === doc!.body)
) {
this._focusAsync(this._previouslyFocusedElementOutsideTrapZone);
}
Expand Down Expand Up @@ -277,7 +280,8 @@ export class FocusTrapZone extends React.Component<IFocusTrapZoneProps, {}> impl
}

if (FocusTrapZone._focusStack.length && this === FocusTrapZone._focusStack[FocusTrapZone._focusStack.length - 1]) {
const focusedElement = document.activeElement as HTMLElement;
const doc = getDocument(this._root.current);
const focusedElement = doc!.activeElement as HTMLElement;
Copy link
Member

@JasonGore JasonGore Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think there are enough uses here that it's worthwhile to make a little helper function and call it directly while eliminating the need for intermediate local doc variables. Something like:

private getDocument() {
  return getDocument(this._root.current);
}

Copy link
Contributor Author

@sophieH29 sophieH29 Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Agree. But still, make sense to remain in places were doc variable is used multiple times within a function scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JasonGore addressed

Copy link
Member

Choose a reason for hiding this comment

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

Aren't there more replaceable instances above? I only see once case where it's actually used more than once.

Copy link
Contributor Author

@sophieH29 sophieH29 Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I replaced all document.* usage with _getDocument().*.In 2 places in FocusZone and and 1 place in FocusTrapZone I saved it to variable const doc = this._getDocument(); and used as doc.* because it is used more than once within the function scope. Let me know if it does make sense


if (!elementContains(this._root.current, focusedElement)) {
this.focus();
Expand Down
10 changes: 7 additions & 3 deletions packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu

let parentElement = getParent(root, ALLOW_VIRTUAL_ELEMENTS);

while (parentElement && parentElement !== document.body && parentElement.nodeType === 1) {
const doc = getDocument(this._root.current);
while (parentElement && parentElement !== doc!.body && parentElement.nodeType === 1) {
if (isElementFocusZone(parentElement)) {
this._isInnerZone = true;
break;
Expand Down Expand Up @@ -500,7 +501,9 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu
return;
}

if (document.activeElement === this._root.current && this._isInnerZone) {
const doc = getDocument(this._root.current);

if (doc!.activeElement === this._root.current && this._isInnerZone) {
// If this element has focus, it is being controlled by a parent.
// Ignore the keystroke.
return;
Expand Down Expand Up @@ -947,8 +950,9 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu

private _getOwnerZone(element?: HTMLElement): HTMLElement | null {
let parentElement = getParent(element as HTMLElement, ALLOW_VIRTUAL_ELEMENTS);
const doc = getDocument(this._root.current);

while (parentElement && parentElement !== this._root.current && parentElement !== document.body) {
while (parentElement && parentElement !== this._root.current && parentElement !== doc!.body) {
if (isElementFocusZone(parentElement)) {
return parentElement;
}
Expand Down