Make the shortcuts provider optional #54080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
requested review from
ajitbohra,
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel,
kevin940726 and
ellatrix
as code owners
|
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
|
ellatrix
reviewed
Aug 31, 2023
ellatrix
reviewed
Aug 31, 2023
ellatrix
force-pushed
the
update/make-shortcuts-provider-optional-2
branch
from
dde6447
to
3694756
Compare
youknowriad
force-pushed
the
update/make-shortcuts-provider-optional-2
branch
from
3694756
to
dde6447
Compare
youknowriad
commented
Sep 4, 2023
| @@ -182,6 +182,8 @@ export function useAutocomplete( { | |||
| setAutocompleter( null ); | |||
| 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 :)
youknowriad
changed the title
youknowriad
changed the title
|
Ok I've fixed everything here. I think this should be ready to land now. |
25 tasks
youknowriad
added
[Type] Enhancement
ellatrix
reviewed
Sep 4, 2023
| 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.
ellatrix
reviewed
Sep 4, 2023
| 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.
ellatrix
reviewed
Sep 4, 2023
packages/block-editor/src/components/writing-flow/use-tab-nav.js
Outdated
Show resolved
Hide resolved
ellatrix
approved these changes
Sep 4, 2023
|
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. |
bph
added
the
Framework
|
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. |
andrewserong
reviewed
Oct 6, 2023
Comment on lines
+41
to
+45
| 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 🙂
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)