Make the shortcut provider optional #53942
Conversation
youknowriad
requested review from
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel and
kevin940726
as code owners
youknowriad
force-pushed
the
update/make-shortcuts-provider-optional
branch
from
28fd16a
to
a7151ed
Compare
|
Size Change: +163 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
| @@ -35,16 +34,14 @@ export default function App() { | |||
| } | |||
|
|
|||
| return ( | |||
| <ShortcutProvider style={ { height: '100%' } }> | |||
There was a problem hiding this comment.
It seems like this code was relying on ShortcutProvider having a div. Not sure if we should not add a div, but if things work I guess we can keep this change.
There was a problem hiding this comment.
Yeah I think this height was there to ensure the editor was full height and the removal of the height is probably going to have the same effect. Of course checking that the site editor is still full height.
There was a problem hiding this comment.
These end to end test failure may be relevant:
- [chromium] › editor/various/shortcut-focus-toolbar.spec.js:17:2 › Focus toolbar shortcut (alt + F10) › Focuses correct toolbar in default view options in edit mode
Error: expect(received).toBeFocused() - [chromium] › editor/blocks/heading.spec.js:183:2 › Heading › should change heading level with keyboard shortcuts
There was a problem hiding this comment.
The change seems good but I'm able to reproduce the end to end test failures with manual testing.
On Mac option + f10 (may need to press fn) on trunk focus the toolbar in this branch it does not. On Mac in trunk control + option + (1,2,3,4,5,6) changes heading in this branch, it does not.
|
So it's actually the iframe that is preventing the events from propagating to the parent event handler. In trunk, there's the same problem for real DOM events but React does some trickery to bubble these as react events (when using onKeyDown props...). I wonder if we should just find a way to bubble these in the iframe instead of forcing a provider. |
|
It turns out that we were already bubbling some event types through the iframe, I've added the keydown events to the list, let's see if it fixes the tests. At least it fixed the focusToolbar in my tests. |
|
Flaky tests detected in 687c653. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6013649089
|
There was a problem hiding this comment.
The useShortcut function will need to updated, too, because the method on the context object is no longer delete, but remove.
There are two additional nice-to-have improvements on useShortcut:
- set the
callbackRef.currentvalue inuseEffectrather than during render. That's the canonical recommended approach to do this typical task. - the
isDisabledprop should default tofalse. Then theisDisabledvalue inside the function doesn't alternate betweenundefinedandfalse, possibly rerunning the effect even though it doesn't need to.
| }; | ||
| }, [ name, isDisabled ] ); | ||
| }, [ name, isDisabled, shortcuts ] ); |
There was a problem hiding this comment.
It good that shortcuts ended up being a dependency. If the context value changes, then all shortcuts will be removed from the old registry and added to the new one, automatically.
|
I've noticed that a lot e2e test failures were because events were triggering twice now (it started happening when I added the bubbling of keydown events from the iframe to the parent component) and I've fixed most of these by just adding So I'm wondering:
|
|
I'll try to debug this. I suspect that it has something to do with how React attaches DOM listeners. When there is a React element with a React event listener: <div onKeydown={ handleKeydown } />then the real DOM listener is not attached on the That affects the order in which handlers are called during the capture and bubbling phases. |
|
Some findings. The e2e failure occurs when a custom completer is active, it looks like this: 
Now, when I press Escape, the completer popover disappears, and the paragraph element now loses focus. Before this PR, the This is caused by the newly introduced bubbling of the And it has nothing to do with the global I'll continue searching for the code that causes the focus loss. |
| @@ -71,44 +71,51 @@ export default function BlockTools( { | |||
| const clientIds = getSelectedBlockClientIds(); | |||
| if ( clientIds.length ) { | |||
| event.preventDefault(); | |||
| event.stopPropagation(); | |||
There was a problem hiding this comment.
Why are we stopping propagation? This is usually a sign that something is wrong. You're preventing components up the tree from listening to this event.
There was a problem hiding this comment.
I'm explaining this here, #53942 (comment) I think we have a bug in the event bubbling code that make the event handler trigger twice for some reason unless the event propagation is stopped. I don't understand why yet.
| @@ -70,7 +70,7 @@ function bubbleEvents( doc ) { | |||
| } | |||
| } | |||
|
|
|||
| const eventTypes = [ 'dragover', 'mousemove' ]; | |||
| const eventTypes = [ 'dragover', 'mousemove', 'keydown' ]; | |||
There was a problem hiding this comment.
We should definitely not add more event names to this list. Ideally we should removing this bubbling entirely.
There was a problem hiding this comment.
If we're totally against this, the alternative would be to add a ShortcutProvider to the Iframe component to catch the keyboard shortcuts events. The problem though is that right now ShortcutProvider also serves as a registry for event handlers useShortcut calls, so we need to have two kind of providers:
- One that keeps track of active event handlers.
- One that only catches event and try to match them with active event handlers.
So, the offending focus call is on this line: It's handler for the "Clear selection" keyboard shortcut that's registered, on the Esc key, here: Now when we started forwarding the |
|
I don't understand form the code what exactly ShortcutProvider is being replaced with. In any case, we need a div so that the shortcuts are scoped to that div (shortcuts shouldn't be global). So either we need to move this into BlockEditorProvider or BlockTools . BlockTools (it already has a div that has an onKeyDown listener for other shortcuts) could work too because InspectorControls actually contains a slot from within the block, so shortcuts should continue to work in there. |
I disagree with this take, some shortcuts don't need a div, like command palette. Maybe some block editor shortcuts need a div but right now in trunk that div is a random div containing the whole page and not something scoped to the block editor. |
|
@jsnajdr interesting find. The question is: in "trunk" that event handler "core/block-editor/unselect" is also supposed to be triggered when you hit "Escape" (it's just caught differently using a global React listener and not DOM listener). So I'd say that probably there's some code that "prevents bubbling" or "prevents default" already in place but that code only run when the auto-complete is open. So IMO, the problem is still the same as I said above: the event is kind of triggered twice, one of them is canceled/preventDefault/preventPropagation by the autocomplete but then the autocomplete closes and the second event is not cancelled causing the selection clear. |
|
Ok after a discussion with @ellatrix we figured that iframe event bubbling results in "two React event handlers" being triggered because of React's internal behavior and the new event the bubbling causes. and This kind of forces to rethink this PR a bit. My current plan is the following:
|
|
Thanks @youknowriad! |
|
Pushed a commit to implement the proposed solution (kept the iframe bubbling temporarily). I'm not entirely satisfied for a few reasons:
|
youknowriad
force-pushed
the
update/make-shortcuts-provider-optional
branch
from
717376a
to
5c6c3ed
Compare
youknowriad
force-pushed
the
update/make-shortcuts-provider-optional
branch
from
5c6c3ed
to
bd15218
Compare
|
@ellatrix After thinking a bit here. I actually didn't feel great with the new "scope" API especially because of the weird refs passed around that it forces us to do. (Explained a bit better above) So I decided to give the initial approach of this PR a second chance: iframe event bubbling but keeping the scoping you had in place in BlockTools and similar using "local" event handlers. I managed to actually fix the problem we had with iframe event bubbling creating two react synthetic events, it's actually as simple as "prevent propagation" within the bubbling function. The solution is here #54080 I've kept it on a separate PR/branch for us to compare but I'm personally more confident about #54080. it's less changes and keeps a very similar logic to trunk, only change is to switch the default event handler to an event handler on the body of the outer document. |
|
Closing this one in favor of #54080 |
Partially reverts #34539
Related #53874
What and why?
Gutenberg can be used as a platform/framework to build block editors. Mainly thanks to the @wordpress/block-editor package. That said, the experience today is not as straightforward as it can be. There can be a lot of small gotchas and hacks you need to do in order to achieve the desired result. One of these small things is the need to manually render
ShortcutProvidercomponent, this PR bundles this behavior within the BlockEditorProvider component, that way custom block editors don't have to render this extra component.The original PR says that it only impacts "focus loss" behavior but that is not true, if you navigate in the browser between different pages in WP-Admin, focus is moved to the "body" when you enter new pages and some keyboard shortcuts are expected to work in that context directly without the need to focus any part of the UI. For instance the global command palette shortcut. This means that the current PR also fixes a bug.
Testing Instructions
1- Check that the different keyboard shortcuts still work as intended. I'm honestly not 100% confident about the impact here but I believe it's a good improvement.