-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Stop propagation on withConstrainedTabbing
#26612
Conversation
d37360a
to
2085279
Compare
// Stops nested withConstrainedTabbing components from always focusing | ||
// the outermost component. | ||
event.stopPropagation(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there!
i've been thinking a bit here, it seems we're relying on React event handlers for this HoC. the issue with that is that it has unexpected behaviors when using "Portals":
1- It will catch events from portals even if these portals are not in the same DOM tree.
2- it won't catch events from portals that are on the same DOM tree but not on the same React tree. (could happen with slots for instance)
I think the current solution solves 1 but doesn't solve 2.
So in reality for this HoC, we only care about the DOM tree and not the React tree. So I believe a better fix might be to change the event handler to use a ref
to attach a DOM event directly and not rely on the onKeyDown
prop.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for taking a look! 🙂
What you said makes sense. However, the problem I'm trying to solve isn't related to portals, it just happens to be that withConstrainedTabbing
is used most often with portals. Take this simple example:
const InnerTabbing = withConstrainedTabbing( () => {
return (
<div>
<Button>1</Button>
<Button>2</Button>
<Button>3</Button>
</div>
);
} );
const TabbingExample = withConstrainedTabbing( () => {
const [ showInner, setShowInner ] = useState( false );
return (
<div>
<Button onClick={ () => setShowInner( ( prev ) => ! prev ) }>
Toggle
</Button>
{ showInner && <InnerTabbing /> }
</div>
);
} );
I would expect the constrained tabbing to wrap around to the numbered buttons, not the toggle button. And that's what this PR solves—with the changes here, the tabbing would wrap around just the numbered buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I'd personally say, that this example should probably never exist because even if tabbing is wrapped inside the buttons, you'll create a bad interaction:
- you can tab from outside to inside "InnerTabbing" and then you get stuck there. I believe that's an a11y problem.
So IMO, both ways are broken interactions in this case, in fact, I believe it's better to go out in this case and just ignore the "inner constrained tabbing" to avoid "tab traps".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. I may have oversimplified my example a little too much—I wanted to show just the withConstrainedTabbing
part in isolation and didn't include any ways to exit the tabbing. Most places in the codebase use withConstrainedTabbing
alongside withFocusReturn
and some way to unmount the component so that focus can return to wherever it was before. My example should have done the same.
If you're adding this component in a real-world scenario, you'd probably quickly realize that you have to add a way to exit as well since you're already modifying how tabbing is handled. But, just as withConstrainedTabbing
can be used without a portal, exiting the loop can be done without withFocusReturn
even if that is the most common way to do it.
My worry is that someone might unknowingly nest a component that uses withConstrainedTabbing
inside another component that uses withConstrainedTabbing
like was done with the color picker (#26115) but outside of the portal context. Changing the code to use DOM events instead of React's events may fix the specific case for portals, but it doesn't solve it generally everywhere.
Furthermore, I'd personally expect a component like this to use React events over DOM events anyway—matching the structure of the components in code rather than figuring out where in the DOM the code will render and tracing the events that way. Although, people's expectations on that will probably go both ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, I'd personally expect a component like this to use React events over DOM events anyway
Why do you think so? For me, it's the exact opposite because this component is specifically about focus handling which is very DOM related and taking React tree into consideration here doesn't really make sense to me.
If you're adding this component in a real-world scenario, you'd probably quickly realize that you have to add a way to exit as well since you're already modifying how tabbing is handled.
I think adding a way to exit (say escape) is not a good solution here, because you get into the tab trap using "tab" and not by opening something or doing a more explicit action which for me indicates that it's a tab trap https://www.w3.org/TR/UNDERSTANDING-WCAG20/keyboard-operation-trapping.html and shouldn't be possible at all. A developer adding withConstrainedTabbing nested will quickly discover that it doesn't work (which is good because what he did is wrong).
A solution might be to actually trigger a warning or an error if we're able to detect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't it be confusing for folks who land on the official documentation that this behaves differently?
We can easily clarify that at the very top of our affected focus-related utilities.
Yes, and we'd have to make that clarification in the docs. My point was that using React events is more natural/expected than using DOM events, even with portals.
Basically I found that for all the hooks/code that deals with focus management and web a11y, it's always better to rely on DOM events.
I still don't quite follow why using DOM events is any better than React events for focus management and web a11y. @youknowriad are there any specific examples you can show me to help me better understand? I appreciate your patience—it just doesn't seem right for me to do something without understanding it first.
I think adding a way to exit (say escape) is not a good solution here, because you get into the tab trap using "tab" and not by opening something or doing a more explicit action which for me indicates that it's a tab trap https://w3.org/TR/UNDERSTANDING-WCAG20/keyboard-operation-trapping.html and shouldn't be possible at all. A developer adding withConstrainedTabbing nested will quickly discover that it doesn't work (which is good because what he did is wrong).
A solution might be to actually trigger a warning or an error if we're able to detect it.
If we do go down the DOM event path, it seems that this HoC should really be built-in to the portal (maybe as a boolean constrainTabbing
prop or something) so it can't be used in a context that isn't supported (like my earlier example). The DOM events option only works in contexts where you're rendering outside of the tree that the component lives. I'd much prefer that to trying to detect misuse and printing a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't quite follow why using DOM events is any better than React events for focus management and web a11y. @youknowriad are there any specific examples you can show me to help me better understand? I appreciate your patience—it just doesn't seem right for me to do something without understanding it first.
Say you're inside a "menu" and you have buttons rendered in that menu. some of these buttons might be rendered in the same React tree, but you also have a "Slot" on that menu to allow plugins to add their buttons there. You want the tabbing to work properly inside that menu, to go from one button to another properly and not skip the ones that come from the slot. If you use React events, you won't catch the events that come from these buttons because they'll bubble to the React tree where they are actually rendered (the plugin code). This is true for all keyboard interaction events.
This is already what we do in our code base in a lot of places and this is not the first HoC that suffered from this issue. I can cite WritingFlow component for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad Thanks for the more specific example—it helped me know where to look. This is more complicated than it seems at surface level, so I started throwing together some more complete examples on CodeSandbox so I didn't just have to theorize about things. Then with Thanksgiving here in the US and a couple other things, I didn't get around to completing those until now. Sorry for the wait.
I came up with six scenarios, and I learned a few things when I was making them up. The full details are in the README.md of the CodeSandbox.
tl;dr What I have (React events) actually works for every scenario1. Tabbable elements are queried with @wordpress/dom
and the container catches the keypresses just fine either way, so event bubbling isn't actually a factor. Checking if the tabbables includes the event target is still necessary either way.
At this point, I'm mostly indifferent to which method is used. On one hand React events are simpler, "conventional wisdom" suggests that they should be preferred, and they work just fine. (Feel free to fork the CodeSandbox if you have an example which would break it.) But on the other hand, since the tabbable elements are queried from the DOM tree instead of the React tree, it might be more consistent to also rely on DOM events.
I'd appreciate it if you could look at the examples to make sure I didn't miss anything. Then, if you still prefer the DOM method, I'll copy over the code from the CodeSandbox to the PR. Thanks!
1There's some bugs described in the CodeSandbox README.md, but they're only tangentially related to withConstrainedTabbing
and happen even when using DOM events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi There! thanks for following-up. I forked your example to demonstrate the issues. See the fail.js file, you can try to switch the import of withConstrinedTabbing HoC. When you use the DOM, tab don't reach toggle 5 as expected but if you do use "virtual" tabbing is not contained anymore.
https://codesandbox.io/s/constrained-tabbing-forked-zqlsp?file=/src/fail.js
I believe in your original examples you forgot bubblesVirtually
in your Slots (this prop is actually what triggers the portalling behavior, we have two different slot implementations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, since our last discussion we already refactored this HoC to a hook using useConstrainedTabbing (and it's dom based), so I believe this issue is probably solved.
Closed by #27544 |
Description
Fixes #26115
Nested
withConstrainedTabbing
components were causing focus to more to the outermost parent component, for example, in the custom text color options from the paragraph toolbar.How has this been tested?
Try tabbing through nested popovers such as Paragraph > More rich text controls > Text color > Custom color.
Screenshots
Before:
After:
Types of changes
Bug Fix
Checklist: