Skip to content

Introduced focusAsync for cheaper element focusing, and made FocusTrapZone utilize it#4306

Merged
dzearing merged 4 commits intomicrosoft:masterfrom
mtennoe:mariust/focusAsync
Mar 22, 2018
Merged

Introduced focusAsync for cheaper element focusing, and made FocusTrapZone utilize it#4306
dzearing merged 4 commits intomicrosoft:masterfrom
mtennoe:mariust/focusAsync

Conversation

@mtennoe
Copy link
Copy Markdown
Member

@mtennoe mtennoe commented Mar 19, 2018

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

We have experienced that using FocusTrapZone becomes expensive if you focus immediately after rendering your wrapped components (depending on content complexity of course), as the focus() it does triggers a recalculation in the browser. This change introduces a focusAsync helper function that the FocusTrapZone will use, which sets focus on the next browser repaint (using requestAnimationFrame). This causes the focus to be a lot cheaper, as it prevents unnecessary browser recalculations

Focus areas to test

(optional)

@mtennoe mtennoe requested a review from dzearing March 19, 2018 15:06
@mtennoe
Copy link
Copy Markdown
Member Author

mtennoe commented Mar 19, 2018

@dzearing - There is a dependency on office-ui-fabric-react to the newest utilities build with this change. Should both be marked as minor versions, instead of patches, due to that?

@mtennoe mtennoe requested review from christiango and jspurlin March 19, 2018 16:18
expect(lastFocusedElement).toBe(buttonD);
// Pressing tab should go to a.
ReactTestUtils.Simulate.keyDown(buttonD, { which: KeyCodes.tab });
window.requestAnimationFrame(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hope this works in ie11. might be worth checking.

Copy link
Copy Markdown
Member Author

@mtennoe mtennoe Mar 20, 2018

Choose a reason for hiding this comment

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

Yup, works like a charm on the testpage (and we have been using this successfully outside of fabric-react for some weeks)

@dzearing dzearing merged commit b152150 into microsoft:master Mar 22, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants