Fix FocusScope StrictMode issues#3878
Conversation
LFDanLu
force-pushed
the
focusscope_strict_mode_fix
branch
from
d9a63bf to
ff373a7
Compare
LFDanLu
changed the title
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
generally looks good, one small comment
| let buttonGroup = getByTestId(buttonGroupId); | ||
| expect(buttonGroup).not.toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical')); | ||
|
|
||
| if (process.env.STRICT_MODE) { |
There was a problem hiding this comment.
can we solve this with the util we worked on instead? I'd prefer the results be the same
There was a problem hiding this comment.
Hm a bit tricky due to how setUp is called in the useEffect in ButtonGroupWithRefs which means our mock implmementations aren't actually set until the first overflow measurements happen in ButtonGroup. Digging now
There was a problem hiding this comment.
I opted to remove the first expect that this test originally had since it was an assertion made before any of our mock widths even were applied (aka widths were undefined for the button/buttongroup). That felt a bit unrealistic for the test and would require me to set the mock widths to 0 for StrictMode to preserve that assertion's logic so I removed it instead
fix case where deleting a row via actionbar didnt restore focus to the new first row.
since strict mode unmounts and rerenders the dialog, we will always run into the "unmounted while open" error in the tests if defaultOpen or isOpen is true. Since I think we cant distingush a true unmount case vs strict modes unmount case, I opted to suppress the warning in the test instead
in strict mode, the layouteffect cleanup triggers even when the focusscope hasnt unmounted. This makes activeScope inaccurate and breaks the parentScope for future nested focusscopes (e.g. nested dialog trigger case)
the previous expect was relying on the fact that ButtonGroup first measurement happened before we mock the widths and positions of the buttons + buttongroups. That doesnt work well in strict mode and didnt feel reflective of browser behavior so I removed that expect
LFDanLu
force-pushed
the
focusscope_strict_mode_fix
branch
from
a68b999 to
0c72b09
Compare
| let {addParentToFocusScopeTree: addParentToTreeMap} = useContext(FocusContext) || {}; | ||
|
|
||
| // Call to add current scope to Tree if it hasn't been added yet. Adds parent scope to Tree if it doesn't exist. | ||
| let addParentToFocusScopeTree = useCallback(() => { |
There was a problem hiding this comment.
Should this be named addSelfToFocusScopeTree? I was confused by the name for a minute.
|
I managed to get the simplification we were hacking on before break to work: #3898 |
3 participants
Closes
An alternative refactor for FocusScope has been started in
focusscope_strict_refactor_all_thingsthat changes how the focusscope tree is constructed. This branch should serve as a new baseline✅ Pull Request Checklist:
📝 Test Instructions:
Note: test off the branch locally for strictmode. You'll have to pull in the changes in #3870 (provides a strictmode checkbox toggle) or from #3837 in order to test the storybook with strict mode. Without either of those changes, the strict mode storybook always crashes with the
Rendered more hooks than during the previous render.error.Turn on StrictMode and do a test sweep. There shouldn't be anymore FocusScope related crashes (aka
TypeError: Cannot read properties of undefined (reading 'parent')). Components that render collections may rendering issues stillRunning the strict mode tests will yield a mixed bag of passes/fails due to other strict mode bugs.
🧢 Your Project:
RSP