Size Change: +163 B (0%)

Total Size: 1.51 MB

_{compressed-size-action}

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.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6013649089
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

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 stopPropagation call on the event handler.

So I'm wondering:

• Why are we catching the event twice, it's almost like the KeyboardShortcutContext is registered twice even though I'm not using ShortcutProvider at all so in theory, we should only have the "global listener".

• Maybe the iframe bubbling code has a bug that forwards the event twice?

Any idea @jsnajdr @ellatrix

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 div, but on the React root element, and React uses event.target to figure out that it should call the handleKeydown function.

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 document.activeElement was the p element all the time, now the focus goes to the body element after Escape. Further typing doesn't go into the paragraph block, but the e2e test expects that it goes.

This is caused by the newly introduced bubbling of the keydown events. When I disable the frameElement.dispatchEvent call, the bug disappears.

And it has nothing to do with the global keydown listener for shortcuts. If I disable the listener, the bug is still there.

I'll continue searching for the code that causes the focus loss.

I'll continue searching for the code that causes the focus loss.

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 keydown event to the iframe's parent, the event is caught by the BlockTools component, which is the iframe's parent.

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 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).

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 BlockTools is relying on its own react event handler and does not use the "Shortcuts" package default one (which is a DOM one), so it creates the double events issue.

This kind of forces to rethink this PR a bit. My current plan is the following:

• Replace iframe event bubbling with a ShortcutEventProvider that catches all events and forward them to the registered handlers.
• Find a way to keep the global event handler on body.
• Potentially find a way to keep the scoped event handlers (the event handler of BlockTools is scoped), maybe by passing a "ref" as a scope argument to useShortcut call.

Thanks @youknowriad!

Pushed a commit to implement the proposed solution (kept the iframe bubbling temporarily). I'm not entirely satisfied for a few reasons:

• I don't like passing "ref objects" as arguments to hooks because these are dependent on component lifecycle for instance if the element unmounts, the ref is no longer valid.
• I noticed that several of our block editor shortcuts (removal, inserter after, insert before, duplicate) are actually duplicated in the code base between two or more places. I suspect that it's because we have scoping in place, these shortcuts would have worked if they were not scope. Maybe there's another way to handle these shortcuts or share their implementation. The existing BlockActions component was meant to share this kind of logic but it's failing entirely. (probably out of scope of this PR though).
• I like that this PR basically allows us to "remove" the __unstableUseShortcutEventMatch API.

@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