Undo / redo (keyboard shortcuts) affecting the whole document rather than the focussed TextControl

[rmorse]

Describe the bug
When using the TextControl component, pressing ctrl+z is expected to undo what you've written inside the component, and this works in most scenarios such as the sidebar.

What I've noticed is that a TextControl generated from within the editor never gets these keyboard events passed down it - so pressing ctrl+z results in the editors undo state being modified, while the current TextControl I'm using doesn't get affected at all. This is occuring for my TextControls in Popovers + Modals, as well as reproducible with built in components.

To reproduce

1. In a block, type some text, and create a link from it
2. Enter a URL in the inline popup UI
3. Before submitting, press ctrl-z
4. Notice the document performs an undo, and the current focussed TextControl does nothing (if you've just started a new document, the whole thing disseapears, but if you have multiple undo levels before you created the link, you will see it step through them)
5. Right clicking inside the URL TextControl, shows the browser undo/redo and they work as expected

Expected behavior
Pressing ctrl-z or ctrl-y inside a TextControl should allow for the native browser implementation of undo/redo within a text input field, and the document should not go back an undo level.

Screenshots

Desktop:

• Windows 10
• Chrome, Edge, Firefox

Additional context

• Gutenberg v6.9.0

[aduth]

I agree with your expectation that Cmd+Z within that link dialog should not cause an editor-level undo.

The challenge is that the expected behavior isn't dependent on this being a TextControl. I would expect there could be TextControls which should use the default editor undo. For me, this depends on whether changes in those fields would impact editor state in any way (post properties, editor content, block attributes, etc).

For the link dialog, the value is maintained in local component state, so an "Undo" action shouldn't be handled by the editor.

At the moment, I'm not really sure how we could know from a framework level whether those changes would be managed in editor state or the component's local state, when interpreting whether an Undo intent should be handled by the editor or the default input behavior.

[aduth]

One possibility is we simply allow an input to flag itself as opting out of the default Undo behavior. It's not as ideal as a solution which "just works" for all inputs, but per my previous comment, I'm not sure how this would be done from a framework level.

The way I might imagine it to be implemented, you could possibly achieve as a workaround today (untested):

<KeyboardShortcuts
    shortcuts={ {
        rawShortcut.primary( 'z' ): ( e ) => e.stopImmediatePropagation(),
        rawShortcut.primaryShift( 'z' ): ( e ) => e.stopImmediatePropagation(),
    } }
>
    <TextControl { /* ... */ } />
</KeyboardShortcuts>

References:

• https://developer.mozilla.org/en-US/docs/Web/API/Event/stopImmediatePropagation
• https://github.com/WordPress/gutenberg/tree/master/packages/components/src/keyboard-shortcuts
• https://github.com/WordPress/gutenberg/tree/master/packages/keycodes#rawShortcut

• gutenberg/packages/editor/src/components/global-keyboard-shortcuts/visual-editor-shortcuts.js

  Lines 38 to 43 in 3cbb291

  	<KeyboardShortcuts
  	shortcuts={ {
  	[ rawShortcut.primary( 'z' ) ]: this.undoOrRedo,
  	[ rawShortcut.primaryShift( 'z' ) ]: this.undoOrRedo,
  	} }
  	/>

A more permanent solution probably wouldn't use stopPropagation, since it has a much higher likelihood of unintended side-effects. Instead, I could imagine something like assigning a property into the event object which is checked by the default implementation of undoOrRedo in VisualEditorShortcuts (similar prior art in <IgnoreNestedEvents> component).

[rmorse]

Yeah this is pretty much what I was thinking.

How could we automatically know that a TextControl is editor affecting, or more just "affecting some setting within a modal or other UI component" - so a flag does sound effective as only the dev would know.

In another way of looking at it, a modal for example, you would expect that most TextControls within one of those should not affect the editor directly, until "OK" has been pressed - from the docs -

Modals disable all other functionality when they appear.

However this is of course not the case and suffers from this same issue.

Thanks for the tip, I did try intercepting the keyboard events to no avail, however I didn't try using <KeyboardShortcuts>

I'm away for a week now but will surely check on my return.

Thanks

[rmorse]

@aduth thanks for the workaround it certainly improves the behaviour of the input field when it's active - I used:

<KeyboardShortcuts
	shortcuts={ {
		[rawShortcut.ctrl( 'z' )]: ( e ) => e.stopImmediatePropagation(),
		[rawShortcut.ctrlShift( 'z' )]: ( e ) => e.stopImmediatePropagation(),
		[rawShortcut.primary( 'z' )]: ( e ) => e.stopImmediatePropagation(),
		[rawShortcut.primaryShift( 'z' )]: ( e ) => e.stopImmediatePropagation(),
	} }
>
	<AdvancedTextControl	/>
</KeyboardShortcuts>

However, I was hoping that wrapping this around my modal would capture the entire Modal but it seems to have some stranage behaviour.

Do you know if it should work in theory?

[aduth]

One thought that comes to mind which might affect the modal specifically: The implementation of modal is unique in that it creates a DOM element elsewhere on the page, not necessarily in the same DOM hierarchy that the React element is rendered. This can sometimes have implications on how events "bubble" to their ancestors. In your case, it might have the effect that KeyboardShortcuts never receives the event.

If you have the option, you can probably work around this by making sure that KeyboardShortcuts wraps the content within the modal, not the Modal element itself.

[aduth]

@youknowriad Since you've been working on keyboard shortcuts refactoring lately (#19100, etc), I'm curious if you might have some thought here on how we might handle the requirements expressed in this issue in mind of the new keyboard shortcuts package?

To manually exempt certain inputs from the global keybind behavior, my first thought (#18755 (comment)) was something like a wrapping component which would "absorb" and prevent the default behavior from taking place. But with the new store, I wonder if it's something where we would instead programmatically disable a shortcut while the input has focus. Having seen some "disabling" behavior as of the recent #19341, I thought this might be something we could hook in to, but I guess it's not something which is derived from the state, but rather in how the specific keyboard shortcut hooks are rendered?

[youknowriad]

How to detect which input to exclude or not is the most difficult question to answer here.

Using our APIs we can only limit the event to a certain container, even if we find a way to have some smart rules like: An input iinside the block list DOM or sidebar can trigger undo/redo, it's not obvious that it's the right behavior. An input in a popover for instance can also be considered valid for undo/redo behavior.

[aduth]

Right, at this point, I'm not convinced we can be doing anything "smart" about detecting which inputs to exclude. But at least for the purpose of implementing a container, I had wondered if you might have some idea for how this might interoperate with (or be implemented as part of) keyboard-shortcuts or the useKeyboardShortcut or useShortcut hooks.

We could simply stop propagation, as per the possible workaround mentioned in #18755 (comment). I'm not really a fan of this idea, since it could have many unintended side effects, and I had wondered if we could attach some semantic meaning to this requirement to "temporarily disable global key handling".

Specifically, I was thinking one of:

• Assign a custom property on the event, to be evaluated in these shortcut handlers (example). This could be in useKeyboardShortcut, useShortcut, or maybe even the event handlers associated with undo/redo.
• Temporarily unregister all/a subset of registered key handlers, either in response to a specific key event or while the focused (active) element is inside the container

Since the global keyboard shortcuts for undo/redo are now "named" as part of #19100, it seems like we could use this as some reference for the specific behaviors we want to neutralize.

[youknowriad]

I actually think a way to opt-out (stop propagation) is better. like suggested here #18755 (comment) or using useKeyboardShortcut. If this becomes common, we could potentially expose an auto-opt-out hook

import { useShortcutOptOut } from '@wordpress/keyboard-shortcuts';
useShortcutOptOut( shortcutName, target );

[aduth]

This is still an active bug. #19488 had been attached as fixing the issue, but I've closed it since I'm not convinced this is the best solution to the broader problem. I believe an approach in preventing bubbling may be necessary, but more recently I've been thinking it should be at the level of the popover component, in which it is treated as a separate event bubbling context.

[Michael-Z-Freeman]

Yes please fix this. If this is the same issue then occasionally it's made me lose text inside a block. I've had a few sentences in a block and then I type something. I don't like it and hit CTRL-Z and the whole block was removed (or it was certainly all the text in the block, I have not got this to test in front of me here). Then even "redo" could not restore the lost text. Bad. This is not accepted undo behaviour. The undo should happen inside the block and not lose entire lumps of text with no option to even restore them with redo. Apart from this I love blocks :)

[rmorse]

I've been having more of a think on this one, as another project I'm working on now has run into this.

As mentioned, the issue is present in popovers that are built in to Gutenberg - and undo/redo does seem like an issue.

But the larger issue is all keyboard shortcuts are still active, so targetting specific keyboard shortcuts probably won't work (eg, what about ctrl-b for bold) - they just shouldn't be enabled in certain scenarios and even though we wouldn't press ctrl-b when using a popover with text input, if we did, it shouldn't do anything to the document underneath.

I'm in agreement with @aduth last comment, there needs to be a way to prevent bubbling at the component level.

So far the discussion doesn't solve my original use case - which is simply using a Modal on a Gutenberg page.

Opening a modal anywhere on a Gutenberg editor is essentially buggy (with regards to activating keyboard shortcuts) - pressing keyboard shortcuts when modals are up, should not do anything, but alas, the document changes underneath.

I would settle for if the modal is focussed, to block keyboard shortcuts but so far can't find a way to block the shortcuts on anything except specific inputs (so I have to wrap each input in this #18755 (comment))
And then, I have to specifiy each and every shortcut WP is using, not practical / workable ) .
So this wouldn't solve this use case anyway...

I think we need to be able to block the global keyboard shortcuts on demand (implemented by a developer), when a container/component is focussed - and we need to disable all of them at once (or most of them, maybe keep ctrl-s for example - not sure...) and possible restore them.

After digging around, this might require us to expose a wrapper that does this for the Mousetrap library?

From what I can tell, because Gutenberg uses the Mousetrap library, its not possible to prevent bubbling etc at a compnent level , because it doesn't bubble from React events - its all handled outside of React, and on the DOM...

Another way to show this issue - with modals, not popovers:

1. Add an image block
2. Choose browse media library
3. start using shortcuts (at any point, with any element focussed / not focussed)
4. Notice the document underneath does stuff- it should not

This shows, we need to be able to control this, regardless on focus of text box / input, but other paramaters too (such as simply showing a modal, as opposed to actually focussing it)?

Hmmmm.. maybe there could be an editor / document level setting. Where we detect if "editor is active/focussed", if so, allow shortcuts, and if editor is not active, disable the shortcuts. Whether the editor is active, could then be controlled by a dev / some event system.... Just throwing out ideas....

I'm now going to try some testing intercepting DOM keyboard events (not React events) at a component level to see if I can interfer with Moustrap grabbing hold of the events before I can do - I hope I'm on the right path... Will update...

[rmorse]

Ok, so this investigation has led me to believe this issue has 2 parts:

1) How to block keyboard shortcuts
Do we need to blanket block all shortcuts in some scenarios as mentioned here - #18755 (comment) - for some uses cases such as modals?
The work around here - #18755 (comment) - has limited effect (I'll explain more in part 2) - however I think it still doesn't quite work in that we still have to explicitly map every shortcut we don't want to be enabled - as this changes over time, it's not that reliable - I think modals are a good use case for blocking all shortcuts when visible.

Possible solution - assuming we want to block all shortcuts momentarily for certain things - in useKeyboardShortcut - we add a flag - disableShortcuts before the callback. If disabled, don't execute the callbacks... however, a question would be - how to set this - through a store + dispatch for example? I'm not even sure this is the right way to approach this..

2) The special case of ctrl+z (undo/redo)
After digging around further, none of the solutions above work in the case of ctrl+z and ctrl+shift+z - this I believe is because regardless of if we manage to block the shortcut using a technique described above, the browser still preforms an undo/redo and jumps to the last input field (in Chrome anyway) that was modified, and carries out its own undo/redo. For a popover this is not a bad user experience as it might seem natural... but in the case of a Modal, you get a side effect - a textbox underneath a modal will probably be focussed by the browser, meaning our current workarounds won't work.

Possible solution - Aside from using preventDefault() (something I guess we want to avoid) - the possible solution suggested in (1) does cover about 80% of this use case, because while the flag to disable shortcuts is active, even if a textbox underneath gets focussed, the editor won't step through the whole history - I guess there is some damage control using this method, because a ctrl z can only go back a few steps (as far as the browser has recorded).

---

I think for now I've gone down the rabbit hole far enough - some feedback would be most useful - I'll try to see if I can create a demo of the issues next week - however its all fairly easy to test as outlined in steps in previous posts.

[paaljoachim]

Could we get a status/summary of where this issue is at right now?
What is the next actionable step to take?

[maksymmamontov]

Seems to be affecting the popular plugin: Advanced Custom Fields when used as a Gutenberg block builder.

Any update on this issue?

[eliot-akira]

There's a solution implemented in the WordPress Playground project's Interactive Code Block, which runs a CodeMirror editor in a Gutenberg block.

https://github.com/WordPress/playground-tools/blob/trunk/packages/interactive-code-block/src/lib/block/without-gutenberg-keyboard-shortcuts.tsx

It's a component called WithoutGutenbergKeyboardShortcuts that stops all keyboard shortcuts from bubbling up - based on @aduth's comment in this thread, and applying it to all key combinations - as well as browser copy/cut/paste events.

In this case, wrapping CodeMirror with this component allowed the code editor's keyboard shortcuts to work without triggering Gutenberg's.

This seems to be generally useful, and it's already written to be portable. It can be copied & pasted into user projects as needed, or even bundled in the Gutenberg repo - perhaps in the @wordpress/keyboard-shortcuts package itself.

[smerriman]

This ticket has been open for nearly 4 years and is extremely frustrating. I've lost track of the number of times I've been working on a post/page, make a typo typing something into a custom field (ACF), auto-hit Ctrl-Z.. and nothing happens. Or did it? Have I lost some content that I've been writing because the undo actually changed something else in the post that I wasn't aware of? No way to know. Quickly hit ctrl-Y a few times and hope anything that broke unbroke.. and then get back to fixing the typo.

Is anyone actually going to fix this?

[stokesman]

Is anyone actually going to fix this?

I think there’s hope but keep in mind that when speaking of plugins there's probably no way to fix them by whatever is done here to resolve this issue. Still, a solution here should facilitate plugin authors in fixing their plugins. There's probably no technical reason this can't already be fixed in ACF in case you want to raise the issue there. If so, linking to this issue could help them.

Also, I've recently done some work on a potential solution. I would have already made a PR for it but I'm waiting until after the bustle of the upcoming WordPress release.

[maksymmamontov]

Any updates on what's the status of the fix?

Is someone working on this?