[Flare] Add Focus within support #15848
Conversation
Details of bundled changes.Comparing: e9d0a3f...d438ac9 react-events
Generated by 🚫 dangerJS |
necolas
left a comment
necolas
left a comment
There was a problem hiding this comment.
This feature is probably worth incorporating but not by changing when onFocus and onFocusChange get called. Instead, I think we should create a new callback – onFocusWithinChange
|
We'd also need an |
|
My thought on that was you probably need either one or the other, not both. If your child is directly focusable, then you want focus events for that immediate child, otherwise you might want focus events for descendants. That's why I did it as a prop rather than a whole new set of events. But willing to change it if you think it's clearer. |
|
I think this feature needs to be able to provide an equivalent to Here's an example of how I think it should work: https://codesandbox.io/s/experimental-event-api-n32y8. It doesn't rely on changing the propagation behaviour of the responder, changing the behaviour of existing callbacks, or introducing a If you need more complex information about which elements are focused and whether they were focused with the keyboard, you should use multiple |
|
What if you want to combine focus within and focus visible? i.e. in the date picker example I showed above, the focus ring gets bolder on focus visible. But the immediate child of the The current PR allows that by changing the behavior of the events to apply to descendants so that I can handle FWIW, I don't think this is directly possible in CSS either (combining :focus-visible and :focus-within). |
That's not what the patch is doing though; it's changing what the current API means entirely without providing If you want to change the styles on your combobox based on whether one of its children is visibly focused you can track that state across multiple |
|
It's opt in, but sure, it does change the behavior when you opt in. What about passing both pieces of info to a single callback? i.e. |
|
Sorry, hit the close button by accident. |
|
At one point I kicked around the idea of making the "change" event handlers take a second argument for meta data. A bit like this: It was focus visibility that got me thinking about doing this. But the state of focus visibility can change independent of focus, i.e., mouse click after keyboard focus. That leaves the UI stuck in the focus visible state when it shouldn't be. (You can see this bug on mobile.twitter.com, e.g., the top nav links) There would be a similar limitation to an API like |
|
Yeah... maybe |
|
It would still need |
|
Focus visibility is global state, so I think The downside to this API as you said is that it might be called a lot more than needed for a particular usecase, e.g. if you don't use focus visibility or focus within state. You seem pretty convinced that separate callbacks is the way to go, so I'm happy to go that way as well. Just wanted to make sure the alternatives were explored. |
Yes focus visibility is global but we're interested in whether a particular component should visibly display focus state. And that's what
Not quite, because
I think I've pointed out the problems with the alternative suggestions. The combobox can even be implemented with the existing |
devongovett
force-pushed
the
focus-within
branch
2 times, most recently
from
3ae71cd to
db96a1c
Compare
devongovett
force-pushed
the
focus-within
branch
from
db96a1c to
01dc492
Compare
|
Ok I've updated the PR to implement |
devongovett
changed the title
necolas
left a comment
necolas
left a comment
There was a problem hiding this comment.
Thanks for updating the PR
|
I think this implementation has a bug with |
|
I don't think you need to track isFocusVisibleWithin separately on state either |
|
@devongovett Are you planning to update this PR or should we pick it up from here? |
|
Yeah sorry, just haven't had a chance yet. I'll try to get to it in the next couple days. |
|
|
Latest patch seems to have regressed how focus is calculated https://codesandbox.io/s/experimental-event-api-03xu4 You can see how broken the focus interactions are now in this gif.
Please keep the logic around all the existing events unchanged. |
| expect(onFocusChange).toHaveBeenCalledWith(false); | ||
| }); | ||
|
|
||
| it('is not called after "blur" and "focus" events on nested descendants', () => { |
There was a problem hiding this comment.
This is incorrect behaviour
There was a problem hiding this comment.
Why? It is verifying that onFocusChange is not called when focusing a descendant rather than the immediate child of the <Focus> component?
There was a problem hiding this comment.
Oh I missed that it wasn't starting with the root being focused. Given the UX is quite broken by the patch it's odd that no tests failed.
There was a problem hiding this comment.
Yeah, I'll look into it and add more tests.
This comment has been minimized.
This comment has been minimized.
FocusWithin is implemented as a separate responder, which keeps both focus responders simple and allows for easier composition of behaviours. Close facebook#15848
FocusWithin is implemented as a separate responder, which keeps both focus responders simple and allows for easier composition of behaviours. Close facebook#15848
|
Closing this as I've implemented focus-within as a separate responder in #16152. |
FocusWithin is implemented as a separate responder, which keeps both focus responders simple and allows for easier composition of behaviours. Close facebook#15848
FocusWithin is implemented as a separate responder, which keeps both focus responders simple and allows for easier composition of behaviours. Close facebook#15848
7 participants
This adds a
withinprop to theFocusevent component, which makes events fire when descendants are focused and blurred in addition to the immediate child. This allows additional flexibility when building more complex components. Thewithinname aligns with the CSS :focus-within pseudo class.For example, in a Date Picker component I'm working on, the focus ring CSS class is added to an outer element when one of the descendent elements is selected. Doing it with one
Focuscomponent wrapper is easier than wrapping each individual focusable element, propagating the focus state (and focus visible state) upwards and maintaining state manually on the common parent.As you can see in the DOM structure above, the div with
role="combobox"gets the focus ring class applied to it, but the individual divs withrole="spinbutton"are actually focusable. This PR allows me to wrap the entire combobox in aFocuscomponent with thewithinprop set in order to apply the focus ring class rather than each individual spinbutton.