Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(FocusTrapZone): Set focus into zone when inner content is lazy loaded #1505

Merged
sophieH29 merged 18 commits into from
Jun 25, 2019
Merged

fix(FocusTrapZone): Set focus into zone when inner content is lazy loaded #1505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
307982f
fix(FocusTrapZone): Set focus into zone when inner content is lazy lo…
Jun 17, 2019
6d2621c
Update PopupExample.shorthand.tsx
sophieH29 Jun 17, 2019
339691b
Update PopupExample.shorthand.tsx
sophieH29 Jun 17, 2019
a24b57c
update changelog
Jun 17, 2019
26ca423
Merge branch 'fix/focus-trap-zone-lazy-load' of https://github.com/st…
Jun 17, 2019
98c3635
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 17, 2019
83d147a
Document lazy loaded content case.
Jun 17, 2019
c61f9bd
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 18, 2019
cd522a9
Merge branch 'fix/focus-trap-zone-lazy-load' of https://github.com/st…
Jun 18, 2019
0276766
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 18, 2019
4816d4c
Merge branch 'master' into fix/focus-trap-zone-lazy-load
Jun 21, 2019
f79d78a
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 21, 2019
1f979ab
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 24, 2019
9b25f72
Update packages/react/test/specs/lib/FocusTrapZone-test.tsx
sophieH29 Jun 24, 2019
705edec
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 24, 2019
78b16c5
update changelog
Jun 24, 2019
ac2c48e
Merge branch 'master' into fix/focus-trap-zone-lazy-load
sophieH29 Jun 24, 2019
9640a60
small improvement
Jun 25, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Type `FontFaceStyle` was renamed to `FontFaceProps` @layershifter ([#1487](https://github.com/stardust-ui/react/pull/1487))
- Type `style` was renamed to `props` on `FontFace` @layershifter ([#1487](https://github.com/stardust-ui/react/pull/1487))
- Remove `boxShadowColor` variable for `Segment` component @Bugaa92 ([#1516](https://github.com/stardust-ui/react/pull/1516))
- Rename prop `forceFocusInsideTrap` to `forceFocusInsideTrapOnOutsideFocus` in `FocusTrapZone` @sophieH29 ([#1505](https://github.com/stardust-ui/react/pull/1505))

### Fixes
- Fix prop types of `Tooltip` component @kuzhelov ([#1499](https://github.com/stardust-ui/react/pull/1499))
Expand All @@ -29,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix `theme` types, remove duplication @kuzhelov ([#1508](https://github.com/stardust-ui/react/pull/1508))
- Fix `RadioGroup` first item should be tabbable by default when none of the items selected @sophieH29 ([#1515](https://github.com/stardust-ui/react/pull/1515))
- Export all accessibility behaviors @jurokapsiar ([#1538](https://github.com/stardust-ui/react/pull/1538))
- Fix `FocusTrapZone` sets focus into zone correctly for lazy loaded content @sophieH29 ([#1505](https://github.com/stardust-ui/react/pull/1505))

### Features
- Add 'poll' and 'to-do-list' icons to Teams theme @natashamayurshah ([#1498](https://github.com/stardust-ui/react/pull/1498))
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/components/Popup/Popup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ export interface PopupState {
* A Popup displays additional information on top of a page.
* @accessibility
* Do use popupFocusTrapBehavior if the focus needs to be trapped inside of the Popup.
* If Popup's content is lazy loaded and focus needs to be trapped inside - make sure to use state change to trigger componentDidUpdate,
* so the focus can be set correctly to the first tabbable element inside Popup or manually set focus to the element inside once content is loaded.
*/
export default class Popup extends AutoControlledComponent<PopupProps, PopupState> {
static displayName = 'Popup'
Expand Down
40 changes: 28 additions & 12 deletions packages/react/src/lib/accessibility/FocusZone/FocusTrapZone.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ export class FocusTrapZone extends React.Component<FocusTrapZoneProps, {}> {
ariaLabelledBy: PropTypes.string,
isClickableOutsideFocusTrap: PropTypes.bool,
ignoreExternalFocusing: PropTypes.bool,
forceFocusInsideTrap: PropTypes.bool,
forceFocusInsideTrapOnOutsideFocus: PropTypes.bool,
forceFocusInsideTrapOnComponentUpdate: PropTypes.bool,
firstFocusableSelector: PropTypes.string,
disableFirstFocus: PropTypes.bool,
focusPreviouslyFocusedInnerElement: PropTypes.bool,
Expand All @@ -58,22 +59,24 @@ export class FocusTrapZone extends React.Component<FocusTrapZoneProps, {}> {

componentDidMount(): void {
FocusTrapZone._focusStack.push(this)
const { disableFirstFocus = false } = this.props

this._previouslyFocusedElementOutsideTrapZone = this._getPreviouslyFocusedElementOutsideTrapZone()
this._bringFocusIntoZone()
this._hideContentFromAccessibilityTree()
}

if (
!this._root.current.contains(this._previouslyFocusedElementOutsideTrapZone) &&
!disableFirstFocus
) {
this._findElementAndFocusAsync()
componentDidUpdate(): void {
if (!this.props.forceFocusInsideTrapOnComponentUpdate) {
return
}

this._hideContentFromAccessibilityTree()
const activeElement = document.activeElement as HTMLElement
// if after componentDidUpdate focus is not inside the focus trap, bring it back
if (!this._root.current.contains(activeElement)) {
this._bringFocusIntoZone()
Copy link
Collaborator

@bmdalex bmdalex Jun 17, 2019

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. A component can update for many reasons and I am pretty sure it's not always expected behavior to bring focus back to the zone after every update.

Is see this behavior as risky because it can cause unexpected focus jumps so I'd recommend allowing the users to configure the focus zone to add / disable this behavior. Let's wait for other opinions.

Copy link
Contributor Author

@sophieH29 sophieH29 Jun 17, 2019

Choose a reason for hiding this comment

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

@Bugaa92 this will only work if the active element (currently focused element) is not inside the focus trap container (for any reason) - focus should also be there once it is rendered.

Copy link
Contributor

@jurokapsiar jurokapsiar Jun 18, 2019

Choose a reason for hiding this comment

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

as discusse doffline - one scenario where this might not work is if the popup opens another popup with focus trap. Then if the first popup updates, it might steal focus from the second popup. Can you please test this scenario?

Copy link
Contributor Author

@sophieH29 sophieH29 Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@jurokapsiar I tested this scenario. Since inner popup is rendered on button click in the first popup without relation to the outside state, it doesn't trigger componentDidUpdate.
Maybe there could be another case that I am missing, I see fabric-ui is doing the same thing. Folks, let me know what you think.

Copy link
Member

@layershifter layershifter Jun 18, 2019

Choose a reason for hiding this comment

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

@sophieH29 can we make this behavior configurable via prop?

Copy link
Contributor Author

@sophieH29 sophieH29 Jun 18, 2019

Choose a reason for hiding this comment

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

I've tried this case and it works

<Popup> -- with lazy load state change    
      <InnerPopup> -- with lazy load state change
              <InnerInnerPopup> -- autocontrolled Popup
                      <Input>

fyi @Bugaa92 @jurokapsiar

Copy link
Contributor Author

@sophieH29 sophieH29 Jun 21, 2019

Choose a reason for hiding this comment

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

@layershifter agree, updated

}
}

render(): JSX.Element {
const { className, forceFocusInsideTrap, ariaLabelledBy } = this.props
const { className, forceFocusInsideTrapOnOutsideFocus, ariaLabelledBy } = this.props
Copy link
Contributor

@jurokapsiar jurokapsiar Jun 21, 2019

Choose a reason for hiding this comment

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

should we use these new props in popupFocusTrapBehavior?

Copy link
Contributor Author

@sophieH29 sophieH29 Jun 21, 2019

Choose a reason for hiding this comment

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

if we want it by default then probably set this prop to "true" in FocusTrapZone. But I am not sure if want this by default, maybe better to allow a user to do it through customizing the behavior

const unhandledProps = getUnhandledProps(
{ handledProps: [..._.keys(FocusTrapZone.propTypes)] },
this.props,
Expand All @@ -93,7 +96,7 @@ export class FocusTrapZone extends React.Component<FocusTrapZoneProps, {}> {
{this.props.children}
</ElementType>

{forceFocusInsideTrap && (
{forceFocusInsideTrapOnOutsideFocus && (
<EventListener
capture
listener={this._handleOutsideFocus}
Expand Down Expand Up @@ -144,6 +147,19 @@ export class FocusTrapZone extends React.Component<FocusTrapZoneProps, {}> {
}
}

_bringFocusIntoZone = () => {
const { disableFirstFocus = false } = this.props

this._previouslyFocusedElementOutsideTrapZone = this._getPreviouslyFocusedElementOutsideTrapZone()

if (
!this._root.current.contains(this._previouslyFocusedElementOutsideTrapZone) &&
!disableFirstFocus
) {
this._findElementAndFocusAsync()
}
}

_findElementAndFocusAsync = () => {
if (!this._root.current) return
const { focusPreviouslyFocusedInnerElement, firstFocusableSelector } = this.props
Expand Down
9 changes: 7 additions & 2 deletions packages/react/src/lib/accessibility/FocusZone/FocusTrapZone.types.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ export interface FocusTrapZoneProps extends React.HTMLAttributes<HTMLDivElement>
ignoreExternalFocusing?: boolean

/**
* Indicates whether focus trap zone should force focus inside the focus trap zone
* Indicates whether focus trap zone should force focus inside the trap zone when focus event occurs outside the zone.
*/
forceFocusInsideTrap?: boolean
forceFocusInsideTrapOnOutsideFocus?: boolean

/**
* Indicates whether focus trap zone should force focus inside the trap zone on component update.
*/
forceFocusInsideTrapOnComponentUpdate?: boolean

/**
* Indicates the selector for first focusable item. Only applies if focusPreviouslyFocusedInnerElement == false.
Expand Down
28 changes: 17 additions & 11 deletions packages/react/test/specs/lib/FocusTrapZone-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class FocusTrapZoneTestComponent extends React.Component<
render() {
return (
<div>
<FocusTrapZone forceFocusInsideTrap={true} isClickableOutsideFocusTrap={false}>
<FocusTrapZone forceFocusInsideTrapOnOutsideFocus isClickableOutsideFocusTrap={false}>
<button className={'a'} onClick={this._toggleFirst}>
a
</button>
Expand All @@ -40,12 +40,18 @@ class FocusTrapZoneTestComponent extends React.Component<
</FocusTrapZone>

{this.state.isShowingFirst && (
<FocusTrapZone forceFocusInsideTrap={false} isClickableOutsideFocusTrap={false}>
<FocusTrapZone
forceFocusInsideTrapOnOutsideFocus={false}
isClickableOutsideFocusTrap={false}
>
<FocusZone data-is-visible={true}>First</FocusZone>
</FocusTrapZone>
)}
{this.state.isShowingSecond && (
<FocusTrapZone forceFocusInsideTrap={false} isClickableOutsideFocusTrap={true}>
<FocusTrapZone
forceFocusInsideTrapOnOutsideFocus={false}
isClickableOutsideFocusTrap={true}
>
<FocusZone data-is-visible={true}>First</FocusZone>
</FocusTrapZone>
)}
Expand Down Expand Up @@ -100,7 +106,7 @@ describe('FocusTrapZone', () => {

const topLevelDiv = ReactTestUtils.renderIntoDocument<{}>(
<div onFocusCapture={_onFocus}>
<FocusTrapZone forceFocusInsideTrap={false}>
<FocusTrapZone forceFocusInsideTrapOnOutsideFocus={false}>
<FocusZone direction={FocusZoneDirection.horizontal} data-is-visible={true}>
<div data-is-visible={true}>
<button className="a">a</button>
Expand Down Expand Up @@ -161,7 +167,7 @@ describe('FocusTrapZone', () => {

const topLevelDiv = ReactTestUtils.renderIntoDocument<{}>(
<div onFocusCapture={_onFocus}>
<FocusTrapZone forceFocusInsideTrap={false}>
<FocusTrapZone forceFocusInsideTrapOnOutsideFocus={false}>
<div data-is-visible={true}>
<button className="x">x</button>
</div>
Expand Down Expand Up @@ -216,7 +222,7 @@ describe('FocusTrapZone', () => {
const topLevelDiv = ReactTestUtils.renderIntoDocument<{}>(
<div onFocusCapture={_onFocus}>
<button className={'z1'}>z1</button>
<FocusTrapZone forceFocusInsideTrap={false}>
<FocusTrapZone forceFocusInsideTrapOnOutsideFocus={false}>
<FocusZone direction={FocusZoneDirection.horizontal} data-is-visible={true}>
<button className={'a'}>a</button>
<button className={'b'}>b</button>
Expand Down Expand Up @@ -283,7 +289,7 @@ describe('FocusTrapZone', () => {
const topLevelDiv = ReactTestUtils.renderIntoDocument<{}>(
<div onFocusCapture={_onFocus}>
<button className={'z1'}>z1</button>
<FocusTrapZone forceFocusInsideTrap={false}>
<FocusTrapZone forceFocusInsideTrapOnOutsideFocus={false}>
<button className={'a'} tabIndex={-1}>
a
</button>
Expand Down Expand Up @@ -353,7 +359,7 @@ describe('FocusTrapZone', () => {
const topLevelDiv = ReactTestUtils.renderIntoDocument<{}>(
<div onFocusCapture={_onFocus}>
<FocusTrapZone
forceFocusInsideTrap={false}
forceFocusInsideTrapOnOutsideFocus={false}
focusPreviouslyFocusedInnerElement={focusPreviouslyFocusedInnerElement}
data-is-focusable={true}
ref={ftz => {
Expand Down Expand Up @@ -461,11 +467,11 @@ describe('FocusTrapZone', () => {

expect(focusTrapZoneFocusStack.length).toBe(2)
const baseFocusTrapZone = focusTrapZoneFocusStack[0]
expect(baseFocusTrapZone.props.forceFocusInsideTrap).toBe(true)
expect(baseFocusTrapZone.props.forceFocusInsideTrapOnOutsideFocus).toBe(true)
expect(baseFocusTrapZone.props.isClickableOutsideFocusTrap).toBe(false)

const firstFocusTrapZone = focusTrapZoneFocusStack[1]
expect(firstFocusTrapZone.props.forceFocusInsideTrap).toBe(false)
expect(firstFocusTrapZone.props.forceFocusInsideTrapOnOutsideFocus).toBe(false)
expect(firstFocusTrapZone.props.isClickableOutsideFocusTrap).toBe(false)

// There should be now 3 focus trap zones (base/first/second)
Expand All @@ -474,7 +480,7 @@ describe('FocusTrapZone', () => {
expect(focusTrapZoneFocusStack[0]).toBe(baseFocusTrapZone)
expect(focusTrapZoneFocusStack[1]).toBe(firstFocusTrapZone)
const secondFocusTrapZone = focusTrapZoneFocusStack[2]
expect(secondFocusTrapZone.props.forceFocusInsideTrap).toBe(false)
expect(secondFocusTrapZone.props.forceFocusInsideTrapOnOutsideFocus).toBe(false)
expect(secondFocusTrapZone.props.isClickableOutsideFocusTrap).toBe(true)

// we remove the middle one
Expand Down