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

Rich Text: Treat ctrl+d as delete #15019

Conversation

jblz
Copy link
Contributor

@jblz jblz commented Apr 17, 2019

Description

Currently, pressing the delete button (fn+delete on the MacBook keyboard) at the end of a Paragraph / Rich-Text block results in a subsequent block merging with the selected one as shown in this screencap:

gutenberg-delete-end-of-rich-text

On the Mac (& in some other environments), pressing ctrl+d in a text-editor is functionally the same as pressing delete. Inside a block, this works appropriately (presumably by the OS control the browser is using): content to the right of the cursor is deleted.

However, the gutenberg conditional which handles deletions is not currently returning true, so onDeleteKeyDown does not get called.

This change adds ctrl+d to the list of key combinations which result in onDeleteKeyDown being called.
It also adds ctrl+h to achieve the opposite on the starting boundary of the text box (which merges in the other direction as per the "backspace" functionality (now called delete on the Apple keyboard)

How has this been tested?

  • Run locally in Docker
  • Create some Paragraph blocks
  • Place the cursor in the middle of the block
    • Press ctrl+d
    • The text following the cursor is deleted
  • Place the cursor at the end of a block with a subsequent Paragraph block
    • Press ctrl+d
    • The two paragraphs are merged and the text following the cursor is the start of the next block (behavior should be the same as pressing the "proper" delete key)
  • Repeat the above with a non-text block (like an image block) following the Paragraph under test
    • Nothing should happen -- the non-text block should stay where it is

Types of changes

New feature

Checklist:

  • My code is tested (manually, but it could use a unit test!)
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@jblz jblz requested review from gziolo and talldan as code owners April 17, 2019 13:39
keyCode === DELETE ||
keyCode === BACKSPACE ||
// On Apple devices, treat control + d as a delete event
( isAppleOS && ctrlKey && keyCode === 68 )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would be ok to apply to all environments, but I don't have a non-Apple computer on which to test.

Copy link
Member

Choose a reason for hiding this comment

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

It would be cool if we could look at the input event instead and detect deleteContentForward and deleteContentBackward. This method would be more robust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try those out, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! The benefit of using that is that the OS doesn't matter. You could even have custom shortcuts and it would still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like InputEvents don't get fired at the boundaries :-/

I tested like this:
document.querySelector( '.editor-rich-text__editable' ).addEventListener( 'input', e => console.log( e ) );

Events fire when inserting & deleting actually happens, but not if the keypress events don't actually result in an input change.

Is there another way we can access those properties?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could insert something to see what happens, but for now I'm ok with just looking at the keydown event.

Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like const D = 'D'.charCodeAt( 0 );?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated w/ this approach (& to add ctrl+h support)

@ellatrix
Copy link
Member

Looks like there is also ctrl+h for deleting to the left. Do you know of any other combination? Is this behaviour documented somewhere? Does this work for any OS, or only MacOS?

@jblz
Copy link
Contributor Author

jblz commented Apr 17, 2019

Thanks for taking a look, @ellatrix!

Looks like there is also ctrl+h for deleting to the left. Do you know of any other combination?

I'm just seeing that -- I don't know of any others. I use ctrl+d very often (since fn+delete requires two hands on modern Macbooks), so run up against this a lot. I never use ctrl + H since I can just press delete (né "backspace") with one hand.

Is this behaviour documented somewhere?

This is all I could find for official documentation on the Apple side: https://support.apple.com/en-us/HT201236

Does this work for any OS, or only MacOS?

I'm pretty confident it works elsewhere, but I don't currently have an environment to test.

@ellatrix
Copy link
Member

Control-H: Delete the character to the left of the insertion point. Or use Delete.

jblz added 2 commits April 17, 2019 11:48
* Support ctrl+H
* New fn isReverseDeleteKeyboardEvent for the above
* Use String.prototype.charCodeAt and save value to constants
keyCode === DELETE ||
keyCode === BACKSPACE ||
// On Apple devices, treat control + d & control + h as delete events
( isAppleOS && ctrlKey && [ D, H ].includes( keyCode ) )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I'm not sure if this should be generally applied regardless of OS.

We should either take out the isAppleOS check or apply it to isReverseDeleteKeyboardEvent as well. Open to suggestions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and removed it for now so this is easier to test on other environments.

@ellatrix
Copy link
Member

I've been thinking about this a bit more. Could a more general solution perhaps be to fire the proper key events if the OS/Browser doesn't?

@swissspidy swissspidy added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label May 28, 2019
@guarani
Copy link
Contributor

guarani commented Oct 27, 2020

Looks like this needs to be rebased with master and conflicts resolved to be testable on mobile since this PR pre-dates the monorepo changes.

@mcsf
Copy link
Contributor

mcsf commented Feb 18, 2021

Closing as stale. Feel free to reopen with a refresh and thoughts on #15019 (comment).

@mcsf mcsf closed this Feb 18, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants