-
Notifications
You must be signed in to change notification settings - Fork 47.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Flare] Add InteractOutside event responder surface #15791
Conversation
Details of bundled changes.Comparing: 113497c...e1ad849 react-events
Generated by 🚫 dangerJS |
This is awesome! Thank you @trueadm |
In my experience, 99% of the time on the web that you think you want "click outside" you actually want "blur". People navigating with the keyboard "click outside" by tabbing out of something and into something else. Click outside is, by definition, only handling mouse users, and a lot of the web is broken for people who can't use a mouse because of this idea of "click outside". And even for mouse users, there are some browser quirks that make it technically unfeasible as well. For example, in macOS Safari, every "click outside" implementation I've ever seen doesn't work when you click a https://codesandbox.io/s/affectionate-dust-fmkct Common cases when people think they want outside click:
So anyway, just want to let you know, "there be dragons" ... and you probably want blur. |
IMHO that's not an argument against "click outside" per se, but rather in favour of handling different interactions properly. By using "click outside" you don't necessarily have to ignore keyboard interactions - they just have to be handled too.
Could you elaborate? I'm curious.
I haven't made extensive tests, but it works on Chrome for me (using your demo).
Yes, you kinda can do that, but you have to call stopPropagation on inner element to make it really work, which kinda sucks. |
Remember this? "For example, in macOS Safari" 🥺
If you handle keyboard, you've already handled "click outside". Blur is click outside, but only if you've handled it.
Only kinda. But creating a first-class event for an inaccessible pattern completely sucks. (Not to mention it's redundant when you've implemented blur already, which you should have done in the first place). |
@ryanflorence There are a bunch of things here I guess. This implementation doesn't use With <Press>
<FixedFullScreenOverlay />
</Press>
<Portal>
<Press>
<Modal />
</Press>
</Portal> In the above example, it's very difficult to known what will fire first – the If you take the approach in this PR: <Portal>
<PressOutside>
<Modal />
</PressOutside>
</Portal> Not only is it easier to understand, under the hood the event responder is doing all the complex work to ensure that focus, keyboard and pointer events propagate and work in accordance to React's component model – not the DOM model (otherwise you'll run into issues with suspense and portals). If you want to handle the user tabbing out of a modal, dropdown or tooltip you'd use |
This is how you can handle pointers on the overlay without a new responder:
Demo: https://codesandbox.io/s/experimental-event-api-e1d0i This is also a pattern that works for any other kind of menu where the underlying document should not be interactive and an overlay is used (e.g., dropdown menus, see mobile.twitter.com) As Dominic mentioned, there are lots of additional details related to the UX we're researching, including stuff like keyboard navigation as per ARIA spec for menus, etc. So we'll be experimenting with abstractions to support the use cases we have internally and absorbing the FB accessibility patterns that currently rely on hacks using native events outside of React. |
Oh, my bad - no idea how I've missed that. Have to be really tired today 😅 @trueadm one use case which comes often in other implementations is to "ignore" some element, most commonly a "trigger" element, so a click on the trigger element only toggles the other component, but doesnt cause "click outside" handler - otherwise you might end up with closing element because of the "click outside" and you might fire click handler which triggers the element again. Not sure how to solve it as such trigger element lives outside of the handled tree, so it doesn't fit nicely with the current model which affects only wrapped subtree. |
@necolas Yeah, that was my original idea I threw around internally on that diff a few days ago but I thought that means that the overlay now needs to block the screen rather than let events pass through? Plus it means having an overlay to begin with, the approach in this PR doesn't require that (I updated the posts above). I guess an invisible overlay isn't too bad as that's how we've always dealt with this problem – but it doesn't mean we have to do it that way. :)
I just checked and I seem to get a @Andarist that sounds like a state machine problem. If you have something open that uses |
I'm also happy to scrap this event responder proposal if the general consensus is that we're happy to approach the problem with a different solution. I'm not so attached, but I do think there's a certain element of validity into how we can utilize the power of the new event API to do things that just weren't possible before with React. Treat this PR as more of a dip into R&D as to what is possible :) |
@trueadm Sorry, "click outside" is just a major trigger for me because it's responsible for a huge amount of inaccessible interactions on the web. I think I'm really just trying to say:
|
I confirm it works with |
@trueadm May I propose |
@yuchi Good shout. Renamed :) |
I would expect In firefox/safari macOS |
@ryanflorence I tried using |
Native |
I don't really know very much about |
@ryanflorence I actually highly dislike this and that's why custom implementations don't work this way. The classic one is when you've opened a select and you need to cross-check something. Say you're flying to a country and you want to check you have the right location in the booking form – I go to my email and double check – to only come back and the Just because browsers do something one way, it doesn't mean it's best in all cases.
We can handle |
I think that's inaccurate. They work this way because people haven't researched this stuff or used a keyboard on their own UI. There may be some edge case benefits to a naively implemented UI control, and you've named one of them. |
Again, this isn't about browsers. Your native OS UIs work this way too. |
I've not suggested anything that differs from native OS UIs. Rather than get into a semantics argument with you, I'll instead change the direction of this thread back to the implementation of this PR. Correct me if I'm wrong, but you suggested that |
When a browser window--or a native OS app window--loses focus, an open dropdown will close. That's what I'd expect with |
@ryanflorence That wouldn't work for dialog/modals though right? Are you saying you should be able to configure |
I just wanted to add none of these APIs are set in stone or even affect open source yet. We’re just experimenting with what might work or not, and learning from that. There will be opportunities to discuss the details of the API after we know it’s something we actually think is shippable. That may, and likely will, take months. |
A modal dialog should stick around, yeah, just like a native OS modal dialog does. I guess I'm concerned an event like this will encourage people to just slap it on a bunch of components when the interactions are all more nuanced. I'll get out of here because I think I'm just distracting the conversation, I'm thinking about specific cases that I see patterns like this applied to and you're just after a low-level thing that may or may not apply to dialog, a menu, or whatever else. Interacting outside of an element seems like an interesting event, how people will apply (and have applied it) in the past bothers me. I do believe that click and focus of an element outside should trigger this. If you need to special case "window" then that's a different discussion. |
@ryanflorence Event components in this new API aren't meant to be consumed at a high-level, by design. They are very powerful, so it gives people building rich shared components for a UI the power to do certain things that wouldn't normally be possible. Someone using these would never use them directly. They'd use I'll add a new prop |
Sounds good, please default to |
@ryanflorence Yep, I've made it default to |
Uncovered cases, which could be covered if the current implementation becomes a bit more generic.
Portals case could be handled with native DOM API with almost the same amount of code - store React event on capture phase and compare it with a real event on when it bubbles back - if targets not match - it was outside. But, from some point of view, it could be already a bit late - target element even listener was fired, and event just bubbling back from it. |
How do you envisage an API for that?
I've added
Can you explain more about this?
We've got a bunch of internal implementations that have tried to do this with Please do note: this work is experimental and focused on internal research first – where we can flesh out known issues. |
Two ways:
:Nods:, that's why I said "it could be already a bit late", and using personally to prevent browser behaviour only (ie Another strange (my) use case:
Problems:
That leads to the question - would be initial event handler or the "outer" element be called:
So - that should be customisable. The same "capture"/"bubble" phases with ability to "stopPropagation" for any event, but outside What I want:
|
@theKashey Given your use-case and looking at internal cases of internal scrollers, I agree there should be some non-default option to interact with I still don't get why you want to exclude interactions from other nodes. It seems like a code-smell. If you have another interactive element on the page that conflicts with the state machine for why the overlay/dropdown/tooltip is showing, then you should resolve that using internal state for that component rather than having to wrap them around Your other use case seems centered around nested
Yes, you don't need to click twice. We don't stop the propagation to other events, both using the new event system with |
You missed the case of joining two const Header = ({taskRef}) => (
<InteractOutside controlledBy={taskRef}>
<FocusScope controlledBy={taskRef}>
{children}
</FocusScope>
</InteractOutside>
);
const Modal = (taskRef) => (
<InteractOutside ref={taskRef} onInteractOutside={closeDialog} interactOnBlur={false}>
<FocusScope contain={true} autoFocus={autoFocus} controlledBy={taskRef}>
<DialogContainer onEscapeKey={closeDialog}>
<DialogHeader onExit={closeDialog} />
{children}
</DialogContainer>
</FocusScope>
</InteractOutside>
)
// ok, InteractOutside shall be combined into one "god controller" for this case. |
As long as you nest event responders, their effects should be grouped by default without having refs. You just ensure that you provide the close event on the upper most InteractOutside event component. They’re grouped by being children/siblings of other FocusScope/InteractOutsides event components. |
@necolas: I was able to break this example by mousing down outside the pressable, dragging the pointer over top of it, then releasing the mouse button. I would expect that not to trigger the ‘close modal’ log. |
That's probably a bug in Press |
I'm a big fan of this idea. I've come across a bunch of use cases where this would be really useful, and it's currently difficult to do this correctly in user space due to React specific concepts like portals. 👍 |
Closing as we're no longer developing this API. See #15257 (comment) |
When testing Flare internally, we ran into quite a few cases where functionality was needed to detect press events that occured outside of a pressable target region. To tackle this effectively, we deemed a new event responder surface might be ideal. This PR adds the
InteractOutside
event surface, that detects presses that occur outside of the children within the event responder. Notably these are the current supported features forInteractOutside
:onInteractOutside
interactOnScroll
(defaults tofalse
)interactOnBlur
(defaults totrue
)disabled
Here's an example of how this might work:
Interaction events that start within the
InteractOutside
responder are handled so they can't leak to outside the responder. Furthermore, interaction events that originate from outside the responder will not fireonInteractOutside
if they move to within the responder after. This implementation should account for portals, something no other implementation seems to properly do – so there's a definite benefit here.InteractOutside
can also support many children, so it's not constrained to having a single child.