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

RichText: own undo signalling #10650

Merged
merged 4 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@iseulde
Member

iseulde commented Oct 16, 2018

Description

This is a proposal for the RichText component to handle undo signalling in favour of listening to the TinyMCE change event.

RichText will signal that an undo level should be created:

  • When the user changes the value, except when it comes from the input event (typing).
  • When there is over a second pause between input events (typing).

Why?

It's not always clear when the change event exactly triggers and we do not always want it to create an undo level. The change event also no longer accounts for formatting changes, as these are no longer done by TinyMCE. It's easier to just do it ourselves so we can fine-tune and not rely on the TinyMCE event.

How has this been tested?

Test undoing and redoing changes from RichText components.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@iseulde

This comment has been minimized.

@iseulde iseulde force-pushed the try/own-richtext-undo branch 2 times, most recently from bfc721d to 7c9ffa8 Oct 31, 2018

@iseulde

This comment was marked as resolved.

Member

iseulde commented Nov 1, 2018

@aduth @mcsf This branch now has the undo timeout consistently, but I cannot figure out what exactly the problem is. I can also reproduce it locally in headless mode, but as soon as I turn headless off, it does not occur at all, even at slowo=0. It seems there's only one extra undo level being created, if I undo once more in the test it passes. I've not been able to figure it out further.

@aduth

This comment was marked as resolved.

Member

aduth commented Nov 1, 2018

Moves the mouse or touches the screen.

Why this? I don't like the idea of every single RichText binding to mousemove. That's easily thousands of callbacks in a single cursor gesture.

@iseulde

This comment was marked as resolved.

Member

iseulde commented Nov 2, 2018

Why this? I don't like the idea of every single RichText binding to mousemove. That's easily thousands of callbacks in a single cursor gesture.

Yeah that's true. We want to create undo levels as soon as typing is interrupted. Here listening for mousedown instead will probably suffice.

@iseulde iseulde force-pushed the try/own-richtext-undo branch from b1b60d4 to 0233a47 Nov 2, 2018

@iseulde iseulde force-pushed the try/own-richtext-undo branch 3 times, most recently from 7cf30f6 to aa4740d Nov 2, 2018

@iseulde iseulde requested review from WordPress/gutenberg-core, mcsf and aduth Nov 2, 2018

@iseulde iseulde added this to the 4.3 milestone Nov 2, 2018

@iseulde iseulde requested a review from WordPress/gutenberg-core Nov 2, 2018

@iseulde iseulde referenced this pull request Nov 5, 2018

Merged

Make it possible to undo prefix transforms #11497

4 of 4 tasks complete
Show resolved Hide resolved packages/editor/src/components/rich-text/index.js Outdated
Show resolved Hide resolved packages/editor/src/components/rich-text/remove-browser-shortcuts.js Outdated
await pressWithModifier( META_KEY, 'z' );
expect( await getEditedPostContent() ).toMatchSnapshot();

This comment has been minimized.

@aduth

aduth Nov 9, 2018

Member

We should ideally test caret position here too. Because it resets to the beginning of the field unexpectedly. Apparently this also occurs in the latest plugin release, so not strictly a regression.

This comment has been minimized.

@iseulde

iseulde Nov 12, 2018

Member

It's always been like this. We'll probably need to save selection to the editor state. Seems like material for a separate PR.

Show resolved Hide resolved test/e2e/specs/blocks/__snapshots__/quote.test.js.snap Outdated
Show resolved Hide resolved test/e2e/specs/__snapshots__/undo.test.js.snap Outdated
@aduth

This comment has been minimized.

Member

aduth commented Nov 9, 2018

The Undo behavior feels particularly heavy-handed (destroying all of a paragraph I've written if I don't move my mouse), but I guess this aligns with the previous behavior as well.

@youknowriad youknowriad modified the milestones: 4.3, 4.4 Nov 9, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 12, 2018

The Undo behavior feels particularly heavy-handed (destroying all of a paragraph I've written if I don't move my mouse), but I guess this aligns with the previous behavior as well.

It should behave roughly the same as before. With this change we can also fine tune it ourselves. Ideas for other triggers are welcome. Not sure what else we can do, maybe a typing pause could trigger an undo level? I don't think we can rely on punctuation as that's not universal.

@iseulde iseulde force-pushed the try/own-richtext-undo branch from 41ed725 to 1cfd664 Nov 13, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 13, 2018

@aduth I went for adding an undo level after a one second pause in input. We can adjust this value if it's too high or low.

@iseulde iseulde requested a review from WordPress/gutenberg-core Nov 13, 2018

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@iseulde iseulde force-pushed the try/own-richtext-undo branch from 77da162 to 78c1a1c Nov 15, 2018

@iseulde iseulde modified the milestones: 4.5, 4.4 Nov 15, 2018

@@ -612,7 +573,7 @@ export class RichText extends Component {
// The input event does not fire when the whole field is selected and
// BACKSPACE is pressed.
if ( keyCode === BACKSPACE ) {
this.onChange( this.createRecord(), true );

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

I don't understand why this change has been made.

This comment has been minimized.

@iseulde

iseulde Nov 15, 2018

Member

We used to skip applying changes back to the live DOM, and so did we here. Since #11595 we no longer do that, but it hasn't been updated here. I've removed it here while revising the parameters for this.onChange.

await clickBlockAppender();
await page.keyboard.type( 'before pause' );
await new Promise( ( resolve ) => setTimeout( resolve, 1000 ) );

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

I wish we could emulate this without actually slowing our test run by one full second.

@@ -114,6 +99,9 @@ class EditorGlobalKeyboardShortcuts extends Component {
bindGlobal
shortcuts={ {
[ rawShortcut.primary( 's' ) ]: this.save,
[ rawShortcut.primary( 'z' ) ]: flow( preventDefault, onUndo ),

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

By moving this into bindGlobal, we're now preventing default behavior for all inputs in the page, not just RichText, right? We'd then rely on all of those inputs to be fully controlled by state values governed by history? I'm not sure I feel particularly confident we can trust that.

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Example:

  1. Navigate to Posts > Add New
  2. Insert a reusable block
  3. Click Edit in the Reusable Block's heading
  4. Make some changes to the title
  5. Press Cmd+Z

In 4.3: Input undoes changes
In this branch: The block is removed (presumably undo'ing the block addition to the editor)

This comment has been minimized.

@iseulde

iseulde Nov 15, 2018

Member

Which input fields on the page would not be included in our history? Maybe it would be better then for those to opt-in vs opt-out? Currently undoing any changes with the browser actually adds to the history, not removes.

This comment has been minimized.

@iseulde

iseulde Nov 15, 2018

Member

Why is the title not part of the history?

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Non-exhaustive set of others:

  • Embed placeholder URL input
  • Table placeholder columns / rows inputs
  • Media placeholders URL input
  • Post permalink
  • Categories search

(Discovered via onSubmit= Find in Files search)

This comment has been minimized.

@iseulde

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Yeah, for now I think we might just need to have two KeyboardShortcuts for handling these keys, one top-level but not applied to inputs, the other specific to when a RichText is selected.

tl;dr I think that's what we had in the prior revision you're referring to?

This comment has been minimized.

@iseulde

iseulde Nov 15, 2018

Member

Right, I just restored your revision :)

@iseulde iseulde force-pushed the try/own-richtext-undo branch from 04430fc to dc4f688 Nov 15, 2018

@aduth

aduth approved these changes Nov 15, 2018

const HANDLED_SHORTCUTS = [
rawShortcut.primary( 'z' ),
rawShortcut.primaryShift( 'z' ),
rawShortcut.primary( 'y' ),

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Not necessary here, but if we're going to support Cmd+Y , we should probably do so consistently between RichText and the top-level handler.

This comment has been minimized.

@iseulde

iseulde Nov 15, 2018

Member

We're only removing the browser shortcut here, not adding any support. But maybe we should add support everywhere?

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Hmm, in that case, is the line even necessary if we're not handling it anyways?

@iseulde iseulde force-pushed the try/own-richtext-undo branch from dc4f688 to 9dba9e5 Nov 15, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 15, 2018

Thanks a lot @aduth for the review!

@iseulde iseulde merged commit 9763f92 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/own-richtext-undo branch Nov 15, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

RichText: own undo signalling (WordPress#10650)
* Own undo history

* Adjust e2e test

* Add undo level after input timeout

* Remove _withoutApply
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment