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

RichText: fix buggy enter/delete behaviour #11287

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 31, 2018

Description

Fixes #6021, #11348, #10484 and #9905.

In master, keep enter pressed in a paragraph, and observe blocks with too many BR elements, or nested paragraph elements, or BR elements around existing text. This is caused by attaching the keydown handler too late (during TinyMCE setup), resulting in default browser behaviour executing. The solution is to attach handler on the content editable element with React as soon as the element is created.

Also removes waitForRichTextInitialization entirely from all tests, because the contentEditable area is immediately available for input.

How has this been tested?

When keeping enter pressed in a paragraph, it should create a bunch a clean, empty paragraphs.

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.

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] In Progress Tracking issues with work in progress labels Oct 31, 2018
@ellatrix ellatrix modified the milestones: 4.3, WordPress 5.0 Oct 31, 2018
@ellatrix ellatrix requested review from youknowriad and a team October 31, 2018 08:54
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Oct 31, 2018
@ellatrix ellatrix modified the milestones: WordPress 5.0, 4.3 Oct 31, 2018
@ellatrix ellatrix added the [Type] Bug An existing feature does not function as intended label Oct 31, 2018
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I've added a few review comments, but it'd be nice to have someone else more familiar with these files also have a look.

Can confirm it solved #11348, so would be good to get this merged fairly soon.

// registered by TinyMCE) from also handling this event.
event.stopImmediatePropagation();
}
if ( this.onDeleteKeyDown( event ) ) {
Copy link
Contributor

@talldan talldan Nov 1, 2018

Choose a reason for hiding this comment

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

I found it a bit misleading that there's a function called 'onDeleteKeyDown' that handles backspace and delete, but then there's also logic below this that handles backspace and delete. The control flow could be tidied up a bit to avoid having a function with side-effects within an if statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I mostly left this as is to minimise the diff. Perhaps better to just move it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I might leave it as-is, because it's already a diff-mess moving other things around and not the easiest to review. We can consolidate these pieces separately.

*
* @param {number} keyCode The key code that has been pressed on the keyboard.
* @param {number} $1.keyCode The key code that has been pressed on the
Copy link
Contributor

Choose a reason for hiding this comment

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

$1.keyCode could just be event.keyCode

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? event is not named anywhere. $1 indicates the first argument.

@@ -223,6 +247,59 @@ export default class TinyMCE extends Component {
}
}

onKeyDown( event ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be nice to name this onEditorKeyDown to further differentiate it from the onKeyDown prop.

* @return {Promise} Promise resolving once RichText is initialized, or is
* determined to not be a container of the active element.
*/
export async function waitForRichTextInitialization() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love it

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Works for me. I admit the code changes are a bit obscure but more because this area of the code is not the easiest to follow.

I notice what we can consider a bug or at least a difference in behavior between current formats and previous ones, when you put the caret in the middle of a word and click "bold", it used to make the whole word "bold", not it does nothing.

@ellatrix
Copy link
Member Author

ellatrix commented Nov 1, 2018

I notice what we can consider a bug or at least a difference in behavior between current formats and previous ones, when you put the caret in the middle of a word and click "bold", it used to make the whole word "bold", not it does nothing.

Yes, it's been like this since the rich text value merge. We can polish this, it's been brought up to me by others. See e.g. #11065 (comment). I don't consider this a bug, but just a difference in behaviour with the new value, it's a bit obscure to me when exactly it should behave how. This can be easily added though, we can detect if the caret is in a word, then make everything around it bold until we hit a non word character. Also note that e.g. Google Docs behaves the same as current master: nothing happens if nothing is selected. Let's discuss separately.

I admit the code changes are a bit obscure but more because this area of the code is not the easiest to follow.

If there's anything obscure left, please let me know, maybe I can clarify the changes I tried to keep the diff to a minimum, but it does require some logic (that undoes TinyMCE behaviour) to be moved from RichText to TinyMCE.

@youknowriad
Copy link
Contributor

@iseulde I actually think the code is a bit clearer this way, so yes, let's ship. It's more global to RichText where's it's not always obvious how all these methods work together. (For instance, what does createRecord and applyRecord do, they probably need some JSDocs). Also sometimes event handlers do very different things at the same time depending on the event, this is made harder because of how we separate code in React in general. I expect it will be better later (probably with hooks and making TinyMCE less important), so nothing really to worry about now.

@ellatrix
Copy link
Member Author

ellatrix commented Nov 1, 2018

Yeah, I agree we should generally improve the readability of the RichText component in general. We could attempt this in a separate PR without making any other changes. Thanks for the review!

@ellatrix ellatrix merged commit a38c816 into master Nov 1, 2018
@ellatrix ellatrix deleted the try/rich-text-keyboard-events branch November 1, 2018 09:21
@ellatrix ellatrix mentioned this pull request Nov 1, 2018
4 tasks
daniloercoli added a commit that referenced this pull request Nov 1, 2018
…rnmobile/port-quote-block-step-1

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Add removed periods to block descriptions. (#11367)
  Remove findDOMNode usage from the inserter (#11363)
  Remove deprecated componentWillReceiveProps from TinyMCE component (#11368)
  Create file blocks when dropping multiple files at once (#11297)
  Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168)
  Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180)
  Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342)
  Remove the Cloudflare warning (#11350)
  Image Block: Use source_url for media file link (#11254)
  Enhance styling of nextpage block using the Hr element (#11354)
  Embed block refactor and tidy (#10958)
  Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347)
  Fix RTL block alignments (#11293)
  RichText: fix buggy enter/delete behaviour (#11287)
  Remove code coverage setup (#11198)
  Parser: Runs all parser implementations against the same tests (#11320)
  Stop trying to autosave when title and classic block content both are empty. (#10404)
  Fix "Mac OS" typo + use fancy quotes consistently (#11310)
  Update documentation link paths (#11324)
  Editor: Reshape blocks state under own key (#11315)
  ...

# Conflicts:
#	gutenberg-mobile
youknowriad pushed a commit that referenced this pull request Nov 6, 2018
* Fix buggy enter/delete behaviour in RichText

* Fix setting selection on merge when value stays the same

* Re-add horizontal navigation comment
@ellatrix ellatrix mentioned this pull request Nov 8, 2018
4 tasks
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 [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning on pressing ENTER fast
3 participants