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

sophieH29 · 2019-06-17T11:25:16Z

BREAKING CHANGES

Renamed prop forceFocusInsideTrap to forceFocusInsideTrapOnOutsideFocus

Improvements

Added props forceFocusInsideTrapOnComponentUpdate - false by default
- once componentDidUpdate called in FocusTrapZone and current focused element is outside the FTZ's container, it will grab focus and put it on the inner element
To make it work for lazy loaded content there are 2 options:
1. trigger componetDidUpdate by using state change when new content is loaded:

   const ButtonExampleEmphasis = () => {
  const loader = <Loader />;
  const [content, setContent] = React.useState(loader);

  setTimeout(() => {
    if (content === loader) {
      setContent(
        <Button content="has focus?" onClick={() => alert("I was clicked")} />
      );
    }
  }, 3000);

  return (
    <Popup
      accessibility={popupFocusTrapBehavior}
      trigger={<Button content="Click Me!" primary />}
      content={{ content }}
    />
  );
};

put the focus inside the Popup manually after content is loaded

…aded

…ardust-ui/react into fix/focus-trap-zone-lazy-load # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

codecov · 2019-06-17T11:50:10Z

Codecov Report

Merging #1505 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   73.21%   73.24%   +0.02%     
==========================================
  Files         822      822              
  Lines        6194     6200       +6     
  Branches     1782     1802      +20     
==========================================
+ Hits         4535     4541       +6     
  Misses       1654     1654              
  Partials        5        5

Impacted Files	Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`72.38% <100%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae15a8...98c3635. Read the comment docs.

bmdalex · 2019-06-17T14:53:10Z

packages/react/src/lib/accessibility/FocusZone/FocusTrapZone.tsx

+    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()


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.

@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.

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?

@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.

@sophieH29 can we make this behavior configurable via prop?

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

@layershifter agree, updated

codecov · 2019-06-17T15:39:51Z

Codecov Report

Merging #1505 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   73.21%   73.24%   +0.02%     
==========================================
  Files         822      822              
  Lines        6194     6200       +6     
  Branches     1782     1802      +20     
==========================================
+ Hits         4535     4541       +6     
  Misses       1654     1654              
  Partials        5        5

Impacted Files	Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`72.38% <100%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae15a8...98c3635. Read the comment docs.

codecov · 2019-06-17T15:39:55Z

Codecov Report

Merging #1505 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   73.21%   73.24%   +0.02%     
==========================================
  Files         822      822              
  Lines        6194     6200       +6     
  Branches     1782     1802      +20     
==========================================
+ Hits         4535     4541       +6     
  Misses       1654     1654              
  Partials        5        5

Impacted Files	Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`72.38% <100%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae15a8...98c3635. Read the comment docs.

codecov · 2019-06-17T15:41:15Z

Codecov Report

Merging #1505 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   73.21%   73.24%   +0.02%     
==========================================
  Files         822      822              
  Lines        6194     6200       +6     
  Branches     1782     1802      +20     
==========================================
+ Hits         4535     4541       +6     
  Misses       1654     1654              
  Partials        5        5

Impacted Files	Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`72.38% <100%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae15a8...98c3635. Read the comment docs.

codecov · 2019-06-17T15:42:42Z

Codecov Report

Merging #1505 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   73.21%   73.24%   +0.02%     
==========================================
  Files         822      822              
  Lines        6194     6200       +6     
  Branches     1782     1802      +20     
==========================================
+ Hits         4535     4541       +6     
  Misses       1654     1654              
  Partials        5        5

Impacted Files	Coverage Δ
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`72.38% <100%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae15a8...98c3635. Read the comment docs.

codecov · 2019-06-17T15:42:51Z

Codecov Report

Merging #1505 into master will decrease coverage by 0.01%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
- Coverage   73.17%   73.16%   -0.02%     
==========================================
  Files         828      828              
  Lines        6219     6227       +8     
  Branches     1802     1804       +2     
==========================================
+ Hits         4551     4556       +5     
- Misses       1663     1666       +3     
  Partials        5        5

Impacted Files	Coverage Δ
packages/react/src/components/Popup/Popup.tsx	`68.15% <ø> (ø)`	⬆️
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx	`70.58% <82.35%> (-0.51%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b788cf4...9640a60. Read the comment docs.

…ardust-ui/react into fix/focus-trap-zone-lazy-load

# Conflicts: # CHANGELOG.md

jurokapsiar · 2019-06-21T12:37:00Z

packages/react/src/lib/accessibility/FocusZone/FocusTrapZone.tsx

  }

  render(): JSX.Element {
-    const { className, forceFocusInsideTrap, ariaLabelledBy } = this.props
+    const { className, forceFocusInsideTrapOnOutsideFocus, ariaLabelledBy } = this.props


should we use these new props in popupFocusTrapBehavior?

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

packages/react/test/specs/lib/FocusTrapZone-test.tsx

Co-Authored-By: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

fix(FocusTrapZone): Set focus into zone when inner content is lazy lo…

307982f

…aded

sophieH29 added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. accessibility All the Accessibility tasks and bugs should be tagged with this. labels Jun 17, 2019

sophieH29 requested review from dvdzkwsk, kuzhelov, layershifter, levithomason, miroslavstastny and mnajdova as code owners June 17, 2019 11:25

sophieH29 self-assigned this Jun 17, 2019

sophieH29 and others added 4 commits June 17, 2019 13:27

Update PopupExample.shorthand.tsx

6d2621c

Update PopupExample.shorthand.tsx

339691b

update changelog

a24b57c

bmdalex reviewed Jun 17, 2019

View reviewed changes

sophieH29 and others added 2 commits June 17, 2019 17:29

Merge branch 'master' into fix/focus-trap-zone-lazy-load

98c3635

Document lazy loaded content case.

83d147a

sophieH29 and others added 4 commits June 18, 2019 10:25

Merge branch 'master' into fix/focus-trap-zone-lazy-load

c61f9bd

Merge branch 'fix/focus-trap-zone-lazy-load' of https://github.com/st…

cd522a9

…ardust-ui/react into fix/focus-trap-zone-lazy-load

Merge branch 'master' into fix/focus-trap-zone-lazy-load

0276766

Merge branch 'master' into fix/focus-trap-zone-lazy-load

4816d4c

# Conflicts: # CHANGELOG.md

jurokapsiar reviewed Jun 21, 2019

View reviewed changes

Merge branch 'master' into fix/focus-trap-zone-lazy-load

f79d78a

Merge branch 'master' into fix/focus-trap-zone-lazy-load

1f979ab

layershifter reviewed Jun 24, 2019

View reviewed changes

packages/react/test/specs/lib/FocusTrapZone-test.tsx Outdated Show resolved Hide resolved

sophieH29 and others added 5 commits June 24, 2019 15:03

Update packages/react/test/specs/lib/FocusTrapZone-test.tsx

9b25f72

Co-Authored-By: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

Merge branch 'master' into fix/focus-trap-zone-lazy-load

705edec

update changelog

78b16c5

Merge branch 'master' into fix/focus-trap-zone-lazy-load

ac2c48e

small improvement

9640a60

layershifter approved these changes Jun 25, 2019

View reviewed changes

sophieH29 merged commit 0a9e616 into master Jun 25, 2019

delete-merged-branch bot deleted the fix/focus-trap-zone-lazy-load branch June 25, 2019 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sophieH29 commented Jun 17, 2019 •

edited by layershifter

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

bmdalex Jun 17, 2019

sophieH29 Jun 17, 2019

jurokapsiar Jun 18, 2019

sophieH29 Jun 18, 2019 •

edited

Loading

layershifter Jun 18, 2019

sophieH29 Jun 18, 2019

sophieH29 Jun 21, 2019

codecov bot commented Jun 17, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

jurokapsiar Jun 21, 2019

sophieH29 Jun 21, 2019

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

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

Conversation

sophieH29 commented Jun 17, 2019 • edited by layershifter Loading

BREAKING CHANGES

Improvements

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2019

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

codecov bot commented Jun 17, 2019

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophieH29 commented Jun 17, 2019 •

edited by layershifter

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

sophieH29 Jun 18, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading