Update heading block on ENTER key to create empty paragraph instead of heading #10382

andrewserong · 2018-10-07T11:10:33Z

Resolves #10326

Description

This PR tweaks the behaviour for the heading block when a user presses ENTER at the beginning of a heading (as mentioned in #10326).

This allows the ENTER key pressed at the beginning of a heading to create an empty paragraph above it. Also, when the ENTER key is pressed at the end of a heading, an empty paragraph block is created below, instead of another heading block.

I realise this could be a matter of personal preference, but to me this feels like a more natural behaviour when writing long-form prose, where the default block would seem to be the paragraph, and I assume a user creating headings would want to switch back to a paragraph block instead of creating multiple heading blocks.

How has this been tested?

Unit tests and linting is passing locally. Tested in Chrome 69 and Safari 11.1.1 on macOS 10.13.5.

Screenshots

Types of changes

Extending the logic of the onSplit method in the heading component.

Checklist:

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

packages/editor/src/components/rich-text/index.js

ellatrix · 2019-02-07T17:32:12Z

Worth noting that this bug is also addressed in #11005, but it's cool if I can be done without it.

gziolo · 2019-02-08T11:51:53Z

@andrewserong do you plan to continue working on this PR? It looks like the changes which @iseulde referenced are much broader but you would have to validate yourself how much overlap there is.

andrewserong · 2019-02-09T00:29:20Z

@gziolo apologies for the late reply — I'll take a look over this PR again later on today. I've just rebased master into the branch (since the PR is pretty old now), and will compare against the PR @iseulde mentioned.

andrewserong · 2019-02-09T05:36:12Z

@gziolo and @iseulde — I've fixed up this PR, updating the order of onReplace and insertBlocksAfter (looks like the behaviour of onReplace might have changed slightly since I initially submitted the PR).

Looks like the tests are passing now, but I'm also aware that I haven't added e2e tests for this specific behaviour. It looks like particular blocks aren't targeted in the existing e2e tests, so I wasn't sure if behaviour specific to the heading block should be covered or not?

The other consideration is that this PR works with the current unstableOnSplit method, but will be superseded by the changes @iseulde has made to onSplit. If you'd like a quick fix for this bug, I'm happy to do any further changes you might like on this PR, otherwise, also happy for you to close it and wait for the revised onSplit prop in #11005.

ellatrix · 2019-02-12T16:53:04Z

I think it's ok to add the fix, but we should definitely add e2e tests. :)

… instead of heading (WordPress#10326) The onSplit method for the heading block will now replace an empty heading block with an empty paragraph block for the 'before' content. This allows the ENTER key pressed at the beginning of a heading to create an empty paragraph above it. Also, when the ENTER key is pressed at the end of a heading, an empty paragraph block is created below, instead of another heading block. The RichText component now also calls event.stopImmediatePropagation() before splitContent() to prevent TinyMCE from throwing an error. This avoids a similar error as in WordPress#4580.

Since my last commit, the behaviour of onReplace seems to have changed a little bit. This commit ensures that an empty paragraph is added before a heading if a heading is 'split' with no before text, but with some after text (e.g. if the carat is at the beginning of a heading and the user presses ENTER) Also removed unnecessary `event.stopImmediatePropagation()`.

andrewserong · 2019-02-13T13:22:11Z

Thanks @iseulde! — I've added e2e tests now to cover the initial bug report in #10326, so the issue should be adequately covered when this fix is superseded by your revised onSplit prop. cc: @gziolo

This ensures that if a user types a heading, then moves the caret to the beginning of the line and presses ENTER, that a paragraph block is created above. Also ensures that when a user presses ENTER at the end of a heading, a paragraph block is created below.

gziolo · 2019-02-13T15:45:16Z

Nice addition with tests. Let’s merge it as is after some testing in that case 👍

packages/e2e-tests/specs/splitting-merging.test.js

…s check (WordPress#10326)

andrewserong · 2019-02-13T21:46:01Z

Thanks for the feedback @iseulde! I've moved the tests up to the heading block tests, and removed the extra active class check.

ellatrix · 2019-02-14T09:15:09Z

Something strange happens when merging back:

Put the caret in front of the heading text.
Press enter. An empty paragraph is inserted before the heading block. The caret stays in place. All good.
Now press backspace to merge the blocks.
- Expected: remove the empty paragraph block and the caret should stay in place.
- Actual: removes the paragraph block, but also transforms the heading block into a paragraph block and the caret is put at the end...

ellatrix · 2019-02-14T09:16:18Z

I could live with it being transformed to a paragraph block, but the caret moving to the end is very unexpected. This doesn't happen in master.

ellatrix · 2019-02-14T09:18:02Z

packages/e2e-tests/specs/blocks/heading.test.js

@@ -5,6 +5,8 @@ import {
 	clickBlockAppender,
 	getEditedPostContent,
 	createNewPost,
+	insertBlock,
+	pressKeyTimes,
 } from '@wordpress/e2e-test-utils';

 describe( 'Separator', () => {


Not introduced in this PR, but this title is not correct. 😅

andrewserong · 2019-02-14T10:07:45Z

I could live with it being transformed to a paragraph block, but the caret moving to the end is very unexpected. This doesn't happen in master.

Oh, thanks for spotting that! Yeah, that's not meant to happen. I'll take another look and see if I can work out what's going on...

andrewserong · 2019-02-14T14:58:06Z

I'm a little bit stumped by this issue — it feels to me like it's got to do with how onReplace behaves. I've run out of time for working on it tonight, but I'll take another look over the weekend.

andrewserong · 2019-02-17T06:40:01Z

I've done a bit more investigating, and I think there might be an issue that exists on Master that affects the behaviour here. If we have an empty paragraph block that already exists, and press backspace at the beginning of a heading block, the paragraph block above is removed, and the caret sits at the beginning of the heading as we would expect. It looks like this issue only occurs on paragraph blocks that have been created above by the heading block when it's split. So it looks like there's something different about the block that was 'created' by the heading block onSplit than a standard paragraph block — even though the code editor view of the block remains the same.

The issue that I noticed on Master occurs if you create a heading, then move the caret to the beginning of the line, and then press ENTER. The heading moves down to a paragraph block as we would expect, but the empty heading block now has two <h2> tags in it, which isn't usually noticeable, but if you're using the 2019 theme, you can see two horizontal lines in the Heading block where there should be just one:

My hunch at the moment is that whatever it is about this block that gets created from the Heading block when a user hits ENTER is causing it to not do what we expect it to when we want to merge those blocks again. If that makes sense!

Since this issue is affecting this PR, I'm happy to keep investigating when I have time, but I don't want to hold anything up if there's a desire to get this one merged in. Let me know, and I'm happy to open a new issue for this.

ellatrix · 2019-04-19T15:17:13Z

@andrewserong Thank you for the work you've done here! I've refactored selection state in #14640 and now #14765 will revise the onSpit prop entirely and fix this issue as well, so I'm going to close this PR.

andrewserong · 2019-04-20T07:43:32Z

@ellatrix That's great to hear, thanks for letting me know!

chrisvanpatten added [Feature] Blocks Overall functionality of blocks [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Oct 8, 2018

gziolo requested review from mcsf and ellatrix February 1, 2019 09:22

gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019

gziolo reviewed Feb 7, 2019

View reviewed changes

packages/editor/src/components/rich-text/index.js Outdated Show resolved Hide resolved

gziolo modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 8, 2019

andrewserong force-pushed the update/heading-block-switch-to-paragraph-on-enter branch from f6c830e to 0bf392e Compare February 9, 2019 00:21

andrewserong requested review from aduth, ajitbohra, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners February 9, 2019 00:21

andrewserong added 2 commits February 13, 2019 21:27

andrewserong force-pushed the update/heading-block-switch-to-paragraph-on-enter branch from e512f16 to ca48320 Compare February 13, 2019 12:57

andrewserong requested review from nerrad and ntwb as code owners February 13, 2019 12:57

andrewserong force-pushed the update/heading-block-switch-to-paragraph-on-enter branch from ca48320 to 69fc7cc Compare February 13, 2019 13:36

ellatrix reviewed Feb 13, 2019

View reviewed changes

packages/e2e-tests/specs/splitting-merging.test.js Outdated Show resolved Hide resolved

ellatrix reviewed Feb 13, 2019

View reviewed changes

packages/e2e-tests/specs/splitting-merging.test.js Outdated Show resolved Hide resolved

Move heading e2e tests to the heading block, remove extra active clas…

1c398b6

…s check (WordPress#10326)

andrewserong force-pushed the update/heading-block-switch-to-paragraph-on-enter branch from 8cc648a to 1c398b6 Compare February 13, 2019 21:30

ellatrix reviewed Feb 14, 2019

View reviewed changes

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

Tug mentioned this pull request Mar 26, 2019

Enter on Heading block creates another Heading block. wordpress-mobile/gutenberg-mobile#734

Closed

daniloercoli mentioned this pull request Mar 26, 2019

Fix for Enter.key pressed at the end of Heading block that does create another Heading block wordpress-mobile/gutenberg-mobile#787

Merged

ellatrix closed this Apr 19, 2019

andrewserong deleted the update/heading-block-switch-to-paragraph-on-enter branch April 20, 2019 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update heading block on ENTER key to create empty paragraph instead of heading #10382

Update heading block on ENTER key to create empty paragraph instead of heading #10382

andrewserong commented Oct 7, 2018 •

edited

Loading

ellatrix commented Feb 7, 2019

gziolo commented Feb 8, 2019

andrewserong commented Feb 9, 2019

andrewserong commented Feb 9, 2019

ellatrix commented Feb 12, 2019

andrewserong commented Feb 13, 2019

gziolo commented Feb 13, 2019

andrewserong commented Feb 13, 2019

ellatrix commented Feb 14, 2019

ellatrix commented Feb 14, 2019

ellatrix Feb 14, 2019

andrewserong commented Feb 14, 2019

andrewserong commented Feb 14, 2019

andrewserong commented Feb 17, 2019

ellatrix commented Apr 19, 2019

andrewserong commented Apr 20, 2019

Update heading block on ENTER key to create empty paragraph instead of heading #10382

Update heading block on ENTER key to create empty paragraph instead of heading #10382

Conversation

andrewserong commented Oct 7, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Feb 7, 2019

gziolo commented Feb 8, 2019

andrewserong commented Feb 9, 2019

andrewserong commented Feb 9, 2019

ellatrix commented Feb 12, 2019

andrewserong commented Feb 13, 2019

gziolo commented Feb 13, 2019

andrewserong commented Feb 13, 2019

ellatrix commented Feb 14, 2019

ellatrix commented Feb 14, 2019

ellatrix Feb 14, 2019

Choose a reason for hiding this comment

andrewserong commented Feb 14, 2019

andrewserong commented Feb 14, 2019

andrewserong commented Feb 17, 2019

ellatrix commented Apr 19, 2019

andrewserong commented Apr 20, 2019

andrewserong commented Oct 7, 2018 •

edited

Loading