Make the shortcuts provider optional#54080
Conversation
|
Size Change: +1.65 kB (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 39b2f74. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6071225341
|
dde6447 to
3694756
Compare
3694756 to
dde6447
Compare
| setAutocompleterUI( null ); | ||
| event.preventDefault(); | ||
| // This prevents the block editor from handling the escape key to unselect the block. | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
These remaining stopPropagation calls were necessary because if I don't do this the "clear block selection" shortcut triggers as well which causes the block to unselect and lose focus.
What I don't understand is why we didn't have these in place in "trunk" because even in trunk, it seems that the handler of the "unselect" shortcut should trigger because we're just hitting "Escape", I'm not sure why it's not called on trunk. It seemed logical for me that we add these when dialogs are open and we only want to close the dialogs.
There was a problem hiding this comment.
In that case we should check event.defaultPrevented for clearing selection. Did we forget that there?
There was a problem hiding this comment.
We are checking that mmm, I guess it's the defaultPrevented value that might be different between trunk and this branch since we're now using a different event object. Let me check that.
There was a problem hiding this comment.
Ok, I think it's solved now :)
|
Ok I've fixed everything here. I think this should be ready to land now. |
| return useRefEffect( ( body ) => { | ||
| const { defaultView } = iframeDocument; | ||
| const { frameElement } = defaultView; | ||
| const eventTypes = [ 'dragover', 'mousemove' ]; |
There was a problem hiding this comment.
So we're not bubbling keydown in the end?
There was a problem hiding this comment.
Is this refactoring helping with anything in particular or is it a remnant from an earlier iteration?
There was a problem hiding this comment.
No, the refactoring is not doing anything I can restore the previous "bubbleEvents" here but we need the inner "bubbleEvent" function to be shared so we can use it when bubbling the keydown events as a React event handler.
| window.KeyboardEvent, | ||
| frameElement | ||
| ); | ||
| } } |
There was a problem hiding this comment.
Why are we listening for the React event instead of native event?
There was a problem hiding this comment.
That's the only way to basically prevent the "double event bubbling" properly. If I use stopPropagation on a regular event handler and I trigger the event on the frame, it never reached the upper event handlers in my tests.
There was a problem hiding this comment.
There's something weird about this, but I guess we can investigate later.
packages/block-editor/src/components/writing-flow/use-tab-nav.js
Outdated
Show resolved
Hide resolved
ellatrix
left a comment
There was a problem hiding this comment.
I don't particularly like bubbling more event types, but it's a fine temporary solution. I'd love to explore adding the event handlers to the iframe in addition, instead of bubbling the events, but we can try it again later. :)
|
Personally I think it's weird that event handlers bubble through the iframe if you're using React event handlers but don't bubble if you're using a DOM event handler. So in that sense, I like that this PR brings consistency and bubbles them similarly. I wouldn't mind if we manage to make everything work without relying on events bubbling though. |
Yep, that's because of the portal. If you type in the block inspector, the events bubble up through the block instead of the inspector panel, even though the DOM events follow a different path. It's how React decided to make portals work. |
|
I was a bit surprised that this change wasn't mentioned in the changelog for the keyboard-shortcuts package, as it breaks some of my stuff. @youknowriad I would greatly appreciate your advice. Specifically, the change to So it seems like a possible solution would be:
Edit: see my failing workflow run here https://github.com/swissspidy/preferred-languages/actions/runs/6297082131/job/17093335078 |
|
@swissspidy Indeed, that was an oversight, it was not meant to break usage with ShortcutProvider. So we either have to have "current" in both or drop "current" entirely. I'll open a follow-up PR to fix that. Sorry for the breakage. That said, for your own usage (also while we fix the issue), you might want to drop the provider and see if everything works properly. |
That's what I tried, but then things break when running on WordPress 6.3 due to the externalized dependencies. Only when I would bundle all the dependencies it would work. |
| if ( event instanceof frame.ownerDocument.defaultView.MouseEvent ) { | ||
| const rect = frame.getBoundingClientRect(); | ||
| init.clientX += rect.left; | ||
| init.clientY += rect.top; | ||
| } |
There was a problem hiding this comment.
I'm just looking into the issue over in #55074:
Why is this guarded against a check for event instanceof frame.ownerDocument.defaultView.MouseEvent? I notice if I change this to if ( frame ) { I can get the drag position of the block drag chip (i.e. dragging from the inserter over into the editor canvas) working properly again. So it seems this is returning false for some situations where we still want the offset to be applied, so that drag positioning will be correct. In this example, I think it might mean drag events that originated from outside the iframe?
There was a problem hiding this comment.
I don't really know, this code was there long before my PR. I'm guessing we should do this for all events that have "clientX" and "clientY" (we may want to exclude keyboard events for instance...)
There was a problem hiding this comment.
Thanks for confirming, I'll have a play around 🙂
Alternative to #53942
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. (Actually, most of these are e2e tested)