Create FocusScope nodes top down and add them bottom up#3898
Conversation
devongovett
force-pushed
the
simplify-focusscope-strict
branch
from
b1792c2 to
a309df4
Compare
devongovett
requested review from
LFDanLu and
snowystinger
and removed request for
LFDanLu
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
I'm not so sure this is equivalent, this fails one more strict mode test compared to the base branch
I can't easily tell which one, would need to compare junit XML entries or something
|
hmm? as far as I can tell all the tests pass. what are you seeing? |
|
Base PR has 639 failures in strictmode across all components. This PR has 640. |
|
runInBand actually fails worse, no summary for either base branch or this PR, so no junit.xml report. will need to find some other way of getting the summary of just the tests and not the extra info and in the same order |
|
How is junit involved here? When I run |
|
junit is just the reporting, gets logged for every jest run. was just looking for an easy way to compare the test on this branch vs on the base. (to see why it's 640 failures instead of 639) w/o runInBand though, I see this branch fails one more test than the base. I just haven't spent more time than the above steps to determine which one is the extra failing test on this branch do you not see any failures when you run |
|
Oh I see 649 failing. I copied the test output to a file and ran this: cat a.txt | grep '●' | sort > a-sorted.txt
cat b.txt | grep '●' | sort > b-sorted.txt
diff a-sort.txt b-sort.txtHaven't figured out why it fails. |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
LGTM, did some extra testing around nodeToRestore and it seems to work as expected
* tentative change to fix FocusScope strict mode * cleanup * step back some optimization until we fix all issues * properly assign new top level scope parent fix case where deleting a row via actionbar didnt restore focus to the new first row. * fix table crud case * add optimizations to fit assumptions * fix ButtonGroup strictmode test * fixing focuscope test for strict mode * making DialogTrigger tests not fail due to strict mode 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 * ensure activeScope is accurate 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) * fixing tabs tests * removing extraneous parentScope calculation * more cleanup * updating test for strict mode and non strict mode consistency 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 * Create FocusScope nodes top down and add them bottom up (#3898) Co-authored-by: Devon Govett <devongovett@gmail.com>
3 participants
Simplification of #3878.
It works by creating the tree nodes during render (top down), and passing them down via context. In a layout effect (bottom up), it adds itself to its parent and to the tree. If the parent is already in the tree, then we consider
activeScopeas the parent.