fix(useAccessibility): make callbacks referentially stable #12185
Conversation
This file contains 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
layershifter
requested review from
dzearing,
ecraig12345,
JasonGore,
jdhuntington,
kenotron,
levithomason,
miroslavstastny,
mnajdova,
pompomon and
silviuaavram
as code owners
msft-github-bot
added
the
Fluent UI react-northstar (v0)
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 49b255f5808c36413b36866c0ead9dcfa41d3252 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
pompomon
reviewed
Mar 4, 2020
pompomon
reviewed
Mar 4, 2020
| const definition = getAccessibility(debugName, behavior, mapPropsToBehavior(), rtl, actionHandlers); | ||
| if (!slotHandlers.current[slotName]) { | ||
| slotHandlers.current[slotName] = (e, ...args) => { | ||
| const accessibilityHandler = definition.keyHandlers[slotName].onKeyDown; |
There was a problem hiding this comment.
keyHandlers for a slot can be undefined. That was previously checked on line 38 but you don't have the check in the new code.
Also the previous contract was that the onKeyDown listener was added to an element only if it was handling anything. And that could change based on props change. Now the listener is added and never removed.
I think it would be better to be unstable in these edge cases and add/remove the listener when needed.
You can use ButtonBehavior to test both issues.
- It does not add the listener by default -> first issue.
- If you rerender with
as='div', it is supposed to add the listener, if you remove theaslater, it should remove the listener -> second issue.
Co-Authored-By: Roman Sudarikov <pompomon@users.noreply.github.com>
Co-Authored-By: Roman Sudarikov <pompomon@users.noreply.github.com>
layershifter
commented
Mar 6, 2020
| @@ -11,7 +11,7 @@ export interface ReactAccessibilityBehavior extends AccessibilityDefinition { | |||
| } | |||
|
|
|||
| export type AccessibilityKeyHandlers = { | |||
| [slotName: string]: AccessibilityHandlerProps; | |||
| [slotName: string]: AccessibilityHandlerProps | undefined; | |||
There was a problem hiding this comment.
It can be undefined, yep
layershifter
commented
Mar 6, 2020
| @@ -2,6 +2,7 @@ import * as React from 'react'; | |||
|
|
|||
| // useLayoutEffect() produces a warning with SSR rendering | |||
| // https://medium.com/@alexandereardon/uselayouteffect-and-ssr-192986cdcf7a | |||
| const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect; | |||
| const useIsomorphicLayoutEffect = | |||
| typeof window !== 'undefined' && process.env.NODE_ENV !== 'test' ? React.useLayoutEffect : React.useEffect; | |||
There was a problem hiding this comment.
Without process.env.NODE_ENV !== 'test' it produces warning for tests because we use JSDom which has window for sure
layershifter
changed the title
pompomon
approved these changes
Mar 6, 2020
miroslavstastny
approved these changes
Mar 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pull request checklist
$ yarn changeDescription of changes
This PR makes returned
onKeyDownbygetA11yPropsfromuseAccessibility()hook stable between rerenders, so it will stop to break memoized component (PureComponent,React.memo()) which can be used viaasprop or astriggerinPopup&Tooltip.Microsoft Reviewers: Open in CodeFlow