fix: Scope keydown event listener so that it is added to the FocusZone itself instead of the window#24175
Merged
khmakoto merged 3 commits intomicrosoft:masterfrom Aug 2, 2022
Conversation
…e itself instead of the window.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 020c8ad:
|
Asset size changes
Baseline commit: 9100e12e984aaa523b996be5ab11db0ec687b497 (build) |
Collaborator
📊 Bundle size report🤖 This report was generated against 9100e12e984aaa523b996be5ab11db0ec687b497 |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 784 | 795 | 5000 | |
| Breadcrumb | mount | 2244 | 2276 | 1000 | |
| Checkbox | mount | 2231 | 2239 | 5000 | |
| CheckboxBase | mount | 1964 | 1973 | 5000 | |
| ChoiceGroup | mount | 3933 | 3957 | 5000 | |
| ComboBox | mount | 770 | 841 | 1000 | |
| CommandBar | mount | 8890 | 8821 | 1000 | |
| ContextualMenu | mount | 9289 | 9289 | 1000 | |
| DefaultButton | mount | 963 | 957 | 5000 | |
| DetailsRow | mount | 3052 | 3032 | 5000 | |
| DetailsRowFast | mount | 3063 | 3036 | 5000 | |
| DetailsRowNoStyles | mount | 2884 | 2923 | 5000 | |
| Dialog | mount | 2288 | 2255 | 1000 | |
| DocumentCardTitle | mount | 158 | 152 | 1000 | |
| Dropdown | mount | 2798 | 2808 | 5000 | |
| FocusTrapZone | mount | 1548 | 1566 | 5000 | |
| FocusZone | mount | 1483 | 1518 | 5000 | |
| IconButton | mount | 1411 | 1414 | 5000 | |
| Label | mount | 333 | 317 | 5000 | |
| Layer | mount | 2689 | 2687 | 5000 | |
| Link | mount | 438 | 448 | 5000 | |
| MenuButton | mount | 1217 | 1230 | 5000 | |
| MessageBar | mount | 1838 | 1848 | 5000 | |
| Nav | mount | 2641 | 2625 | 1000 | |
| OverflowSet | mount | 951 | 995 | 5000 | |
| Panel | mount | 1778 | 1827 | 1000 | |
| Persona | mount | 853 | 836 | 1000 | |
| Pivot | mount | 1143 | 1137 | 1000 | |
| PrimaryButton | mount | 1083 | 1082 | 5000 | |
| Rating | mount | 6599 | 6631 | 5000 | |
| SearchBox | mount | 1108 | 1120 | 5000 | |
| Shimmer | mount | 2176 | 2219 | 5000 | |
| Slider | mount | 1688 | 1699 | 5000 | |
| SpinButton | mount | 3949 | 3943 | 5000 | |
| Spinner | mount | 400 | 391 | 5000 | |
| SplitButton | mount | 2514 | 2527 | 5000 | |
| Stack | mount | 468 | 479 | 5000 | |
| StackWithIntrinsicChildren | mount | 1891 | 1875 | 5000 | |
| StackWithTextChildren | mount | 4662 | 4681 | 5000 | |
| SwatchColorPicker | mount | 9074 | 8967 | 5000 | |
| TagPicker | mount | 1995 | 2019 | 5000 | |
| TeachingBubble | mount | 73132 | 71880 | 5000 | |
| Text | mount | 406 | 395 | 5000 | |
| TextField | mount | 1179 | 1191 | 5000 | |
| ThemeProvider | mount | 962 | 966 | 5000 | |
| ThemeProvider | virtual-rerender | 640 | 629 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1396 | 1382 | 5000 | |
| Toggle | mount | 732 | 722 | 5000 | |
| buttonNative | mount | 104 | 101 | 5000 |
behowell
approved these changes
Aug 2, 2022
Comment on lines
6
to
8
| // HEADS UP: this test is intentionally in a separate file aside from the rest of FocusZone tests | ||
| // As it is testing ref counting on a global `window` object it would interfere with other FocusZone tests | ||
| // which use ReactTestUtils.renderIntoDocument() which renders FocusZone into a detached DOM node and never unmounts. |
Contributor
There was a problem hiding this comment.
This comment appears to no longer be correct. Can it be updated or deleted?
Member
Author
There was a problem hiding this comment.
Yup, I've just deleted it.
2 tasks
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current Behavior
FocusZonecurrently attaches an event listener on keydown to the window, so that whenever thetabkey is pressed all outerFocusZone'sindexes are updated (an outerFocusZoneis aFocusZonethat is not within another one). This ensures tab indexes are always correct, but can cause a big performance dip since the keydown function is called even on elements that are not withinFocusZones.New Behavior
This PR fixes the issue described above by attaching the listeners to the
FocusZoneelements instead of the window. This means that there is the potential of more event listeners being on the page (one per outerFocusZonevs one for the window), but scopes the listeners to be called only when the event happens within one of theseFocusZones. This has a performance benefit because indexes are now updated only in the scope ofFocusZones, so tabbing on any element outside of them doesn't incur this cost. It is also safe because indexes shouldn't need to be updated if interacting with something outside of aFocusZoneeither way.Related Issue(s)
Fixes #23785
A further improvement is left out of this PR but suggested in #24174.