[react-interactions] Change unmount blur logic to a dedicated event #17291
Conversation
|
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 617fd76:
|
Size changes (stable)Details of bundled changes.Comparing: 3452706...617fd76 react-interactions
|
Size changes (experimental)Details of bundled changes.Comparing: 3452706...617fd76 react-interactions
|
|
Help me understand why we need this and what the internal confusion is? |
|
@necolas It's the same reason we needed it before – we want to know when a node (that has active document focus) gets unmounted from the document – so we can restore focus to another nearby node. The issue with overloading it with |
trueadm
requested review from
bchocho and
threepointone
and removed request for
bchocho
|
Why is it called |
|
@necolas I think a better approach would likely to be an entirely different mechanic that we can get around to thinking when we look at the event system as a whole again in the future. I've got a Quip with all these strange odd cases we've run into in regards to events this year – so it's all collected knowledge for requirements we need to handle at a later time. |
|
I don't follow why the new event is a better interim solution, or why a new callback is needed in the responder, when this seems to be a special type of blur event. |
|
@necolas This is meant for a very bespoke usage right now for accessibility. I might prefix it all with DO_NOT_USE. The alternatives also have drawbacks in that I don't want them to accessible via existing generic APIs or events – as I want this to be restricted until I know all the moving parts work right. With a new event system, none of this would be exposed inside a focus-like interface, instead the accessibility component would be able to ask ReactDOM to let it know directly. |
|
Ok. Plenty of flare stuff has leaked into production dependencies already, including across platforms, so I'm not convinced what got merged is going to be restricted any better than alternatives that don't expand the API surface. Do you want us to stamp flare patches with the expectation that we'll remove the entire system in the future? |
|
I'm not disagreeing with your comments, I'm just saying that these can all be addressed in follow ups. What do you think the better strategy here is for us to apply in a follow up? |
This PR clears up some confusion internally and aligns the behavior of an unmounted node with focus to a specific unique event. This means we can also dispatch a new specific event for this action from the Focus Responder – removing the issues that might arise from mixing it with the existing
blurandfocuslogic. I've opted to call the eventdetachedvisiblenodeoronDetachedVisibleNode– but better names are surely out there!