Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle undo / redo keyboard events #1943

Merged
merged 3 commits into from Jul 28, 2017
Merged

Handle undo / redo keyboard events #1943

merged 3 commits into from Jul 28, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 19, 2017

Closes #627

This pull request seeks to implement Cmd/Ctrl+Z (undo) and Cmd/Ctrl+Shift+Z (redo) behaviors.

Implementation notes:

The implementation is similar to the compromise proposed by @youknowriad at #627 (comment) with some modification. Because we do not sync the TinyMCE value to state until a change event occurs, we allow TinyMCE to handle undo history while the Editable field is focused. If there is no TinyMCE undo history, we propagate the event to be handled by the editor's UNDO action behavior. This works largely because we call editor.save during the change event handler to reset undo history.

Included in these changes is a proposed EditorProvider component which leverages React context (like react-redux) to allow the Editable to communicate with its wrapping editor render hierarchy. This allows Editable to continue to be unaware of the editor or any specific editor instance, while allowing it to propagate the Undo callback without block implementer intervention. I'm curious to apply this pattern to other behaviors as well, such as focus handling.

Testing instructions:

Verify that global Undo handler works as expected:

  1. Navigate to Gutenberg > New Post
  2. Add a new text block with some content
  3. Unfocus the block editable
  4. Press Cmd/Ctrl+Z to undo
  5. Note block is removed
  6. Press Cmd/Ctrl+Shift+Z to redo
  7. Note block is restored

Verify that TinyMCE undo handler works as expected:

  1. Navigate to Gutenberg > New Post
  2. Add a new text block with some content
  3. While text block is still focused, press Cmd/Ctrl+Z to undo
  4. Note that TinyMCE's internal undo history resets text
  5. Continue to undo until there is no text remaining in text block
  6. Undo once more
  7. Note that global editor history handles undo and removes block

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jul 19, 2017
@aduth
Copy link
Member Author

aduth commented Jul 19, 2017

Rebased and force pushed with consideration for #1944

@mtias mtias added [Priority] High Used to indicate top priority items that need quick attention [Type] Task Issues or PRs that have been broken down into an individual action to take labels Jul 24, 2017
blocks/index.js Outdated
@@ -17,5 +17,6 @@ export { default as AlignmentToolbar } from './alignment-toolbar';
export { default as BlockControls } from './block-controls';
export { default as BlockDescription } from './block-description';
export { default as Editable } from './editable';
export { default as EditableProvider } from './editable/provider';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we want to make accessible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum requirement is that the editor provides the context assumed to exist by the Editable component. This could be implemented within editor and not exposed, but then the coupling between Editable and its context becomes unclear.

Personally I think it should be left here. If we want to discourage particular exports, maybe we could do so merely by omitting documentation

} else {
onUndo();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the prevent default here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the prevent default here?

It ensures that the browser default undo behavior is overridden by our custom implementation.

It's likely browser specific, but at least in Chrome I notice some differences in how undo stacks are generated with contenteditable vs. our save intervals with Editable changes.

@mtias
Copy link
Member

mtias commented Jul 27, 2017

@aduth should we make the undo / redo buttons operate the same way?

@mtias
Copy link
Member

mtias commented Jul 27, 2017

I like this, looks good to merge after fixing the conflicts. It'd also be nice to make sure the behaviour of the UI buttons and the keyboard shortcuts was the same.

@aduth
Copy link
Member Author

aduth commented Jul 27, 2017

@aduth should we make the undo / redo buttons operate the same way?

Are you referring to the ones in the header? Both those and the keyboard handlers added here operate on the same UNDO and REDO Redux actions.

@mtias
Copy link
Member

mtias commented Jul 27, 2017

They don't seem to behave equally, with the top level looking disabled sometimes when command+z still works.

@aduth
Copy link
Member Author

aduth commented Jul 27, 2017

Do you have specific steps to reproduce?

@aduth
Copy link
Member Author

aduth commented Jul 27, 2017

Even with these changes I'm finding that undo could still do for some refining, particularly in undoing / redoing changes which have been saved.

@codecov-io
Copy link

codecov-io commented Jul 27, 2017

Codecov Report

Merging #1943 into master will increase coverage by 0.01%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1943      +/-   ##
==========================================
+ Coverage   20.24%   20.26%   +0.01%     
==========================================
  Files         130      131       +1     
  Lines        4184     4205      +21     
  Branches      711      715       +4     
==========================================
+ Hits          847      852       +5     
- Misses       2812     2826      +14     
- Partials      525      527       +2
Impacted Files Coverage Δ
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/index.js 0% <0%> (ø) ⬆️
editor/actions.js 37.03% <0%> (-2.97%) ⬇️
editor/utils/undoable-reducer.js 96.77% <100%> (-3.23%) ⬇️
blocks/editable/index.js 0.47% <16.66%> (+0.47%) ⬆️
blocks/editable/provider.js 33.33% <33.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cec69e8...3eaed9a. Read the comment docs.

@mtias
Copy link
Member

mtias commented Jul 28, 2017

I am ok with getting this as is and refining.

@aduth aduth merged commit c9a619d into master Jul 28, 2017
@aduth aduth deleted the update/627-undo-keyboard branch July 28, 2017 14:02
Tug pushed a commit that referenced this pull request Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Priority] High Used to indicate top priority items that need quick attention [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants