Position caret at end of previous block for any type of block removal #8961

talldan · 2018-08-14T07:36:07Z

Description

As pointed out on #8805 - when removing a block (using either the keyboard shortcut or block settings menu), the caret ends up at the beginning of a preceding paragraph block. It feels natural that the cursor should instead be at the end of the block.

This ticket features a couple of simple changes to improve the situation, placing the cursor by default at the end of the the next selected block in the REMOVE_BLOCKS effect.

Some duplicated code is also removed.

There still seem to be a few cases where the flow is not quite right:

deleting blocks using DELETE key still selects previous block (When using the delete key to remove blocks, the caret is positioned incorrectly #8965)
deleting blocks so that the first block is the selected block behaves inconsistently - sometimes the selections is the end, sometimes the start (Selection of the first block after deletion results in inconsistent caret position #8963)

These issues still exist in master, so will create separate issues.

How has this been tested?

Test removing block(s) using block settings keyboard shortcut (event handled by BlockSettingsMenu)
Test removing block(s) using block settings menu (event handled by BlockSettingsMenu)
Test removing a multi-block selection using delete/backspace (event handled by EditorGlobalKeyboardShortcuts)
Test removing a non-RichText block like the Image Block using delete/backspace (event handled by BlockListBlock

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

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

youknowriad · 2018-08-15T13:11:39Z

Adding Andrew as a reviewer because he worked recently on the merge/split which also can be impacted by removal actions.

The changes look good to me, I just worry about unexpected side effects, I'll try to test a bit.

aduth · 2018-08-15T13:19:54Z

The general idea here seems quite sensible on a glance to me as well. Will give a deep review shortly.

youknowriad · 2018-08-15T13:20:39Z

In all cases, an e2e test covering the bug fixed by this PR would be a good idea :)

aduth · 2018-08-15T21:16:36Z

Could you look in to the failing end-to-end test? I expect it may be legitimate.

Generally speaking, I'm still thinking this looks to be a very good refactor 👍

youknowriad · 2018-08-16T14:36:36Z

I'm going to punt this to 3.7 :( because I don't think it's ready for 3.6

talldan · 2018-08-17T02:06:02Z

e2e test seems to be passing now. I'll write a test to cover this functionality today :)

talldan · 2018-08-20T03:40:35Z

Hey @aduth & @youknowriad, I've added e2e tests, sorry for the delay.

Hope they look ok, the only way I could think to test caret position was to type some additional text to make sure it was added in the correct place. I think it should adequately guard against regressions.

aduth

Thanks for the end-to-end tests. They are very thorough! In fact, I'd expected we might only need to add a test case or two to the existing splitting-merging.test.js file. Upon examination of these test cases though, deletion seems to have quite a few specific behaviors, so perhaps a standalone file is appropriate.

I've left a few remarks for you to consider.

aduth · 2018-08-20T15:35:55Z

test/e2e/support/utils.js

+ */
+export async function clickOnBlockSettingsMenuItem( buttonLabel ) {
+	await expect( page ).toClick( '.editor-block-settings-menu__toggle' );
+	const itemButton = ( await page.$x( `//button[contains(text(), '${ buttonLabel }')]` ) )[ 0 ];


Is there any way we could improve this to limit the selector to the context of the block settings menu? Currently it could match any button on the page.

Very true - I've improved the selector in a way that I think resolves this.

aduth · 2018-08-20T15:36:00Z

test/e2e/support/utils.js

+export async function clickOnBlockSettingsMenuItem( buttonLabel ) {
+	await expect( page ).toClick( '.editor-block-settings-menu__toggle' );
+	const itemButton = ( await page.$x( `//button[contains(text(), '${ buttonLabel }')]` ) )[ 0 ];
+	await itemButton.click( 'button' );


Is the argument necessary? Seems click() would occur on the button anyways.

Now removed.

aduth · 2018-08-20T15:37:46Z

test/e2e/support/utils.js

@@ -272,6 +290,17 @@ export async function clickOnMoreMenuItem( buttonLabel ) {
 	await itemButton.click( 'button' );
 }

+/**
+ * Clicks on Block Settings Menu item, searches for the button with the text provided and clicks it.


Aside: Generally speaking we might consider making these utilities less specific to the specific interaction and more toward the task (triggering a block settings menu item). Opens up future refactoring, and we want to avoid expecting a particular mode of interaction (i.e. couldn't this be implemented with a keyboard? And if not, is that a problem we should be aware of?)

Also, I don't think we need to rush to add functions to utils.js. If we're using them only in one test, it seems fine to define them in that one test, and move things as common patterns emerge.

There is already a test using the keyboard shortcut for the menu item, so it might be unnecessary to also have a version that clicks the menu item.

Anyway - I've moved the code out of utils now (for this and the modifier keys mentioned above).

aduth · 2018-08-20T15:39:05Z

test/e2e/support/utils.js

+ *
+ * @type {string}
+ */
+export const PRIMARY_ALT_MODIFIER_KEYS = process.platform === 'darwin' ? [ 'Alt', 'Meta' ] : [ 'Control', 'Alt' ];


Do we need this? Or is it sufficient to do [ 'Alt', META_KEY ] ? Same applies for the constant defined below.

talldan · 2018-08-21T10:14:57Z

If #9190 is merged first, I'll have to update the new tests to use the right shortcut.

aduth · 2018-08-21T20:18:11Z

test/e2e/specs/block-deletion.test.js

+			expect( await getEditedPostContent() ).toMatchSnapshot();
+		} );
+
+		it( 'positions the caret at the end of the second block as evidenced by typing additional text', async () => {


An it test should be standalone, and not make any assumptions about other its being run or the order in which they're run (for example, to enable parallelization of test execution). This seems like it should be part of the previous test case.

Also FWIW it's fine for a single test case to contain multiple toMatchSnapshot

aduth · 2018-08-21T20:18:45Z

test/e2e/specs/block-deletion.test.js

+	} );
+
+	describe( 'deleting the third block using the Remove Block shortcut', () => {
+		beforeAll( addThreeParagraphsToNewPost );


Basically we shouldn't want to see beforeAll( addThreeParagraphsToNewPost ); as it implies we have multiple tests operating on the same sets of blocks, which presumably are being manipulated by the individual test cases.

It should be a beforeEach to ensure that each test case has a consistently fresh (un-tampered) set of blocks to work with.

aduth · 2018-08-21T20:19:55Z

test/e2e/specs/block-deletion.test.js

+			await page.keyboard.press( 'Backspace' );
+			expect( await getEditedPostContent() ).toMatchSnapshot();
+
+			expect( await getEditedPostContent() ).toMatchSnapshot();


Unnecessarily duplicated? Nothing occurs between this and the previous toMatchSnapshot assertion?

talldan · 2018-08-22T04:32:34Z

@aduth - thanks for the feedback. I wasn't aware test cases could be parallelized. I've combined them now. It's a shame we lose the descriptiveness of the test cases, particularly in the snapshots, but so be it.

When I made that change, the clickBlockAppender utils function started failing (I'm guessing some latency issues) so I've had to add a fix there.

talldan · 2018-08-24T04:10:45Z

@aduth - If you get a moment to re-review it would be much appreciated. I believe I've addressed all the feedback.

aduth · 2018-08-24T14:08:42Z

This looks great, except I'm not fond of the changes to clickBlockAppender. If there are latency issues, it's a bug in the application. The tests should not be updated to accommodate the buggy behavior.

We should plan to understand better the issue here. What was the error? Was the default block appender not available on the initial page load after newPost ? I'm not aware of what would be occurring asynchronously to be causing this.

talldan · 2018-08-27T03:27:22Z

If there are latency issues, it's a bug in the application

The issue I had was still very unusual and I wish I could explain what caused it and why it happened in the one test case and not others. The exact line that caused the latency is this one:
https://github.com/WordPress/gutenberg/pull/8961/files#diff-0f944a8b5594b301a16651bbd429262fR60

With that line present, the next test would fail. When I commented it out the next test passed. It seems very unlikely to be an issue in Gutenberg, but I am surprised it happened—the tests aren't doing anything extraordinary.

Anyway, a bit of further investigation and I've pushed a commit that removes the waitForElement since puppeteer does that implicitly on any page.click (page.click calls through to frame manager's .click):
https://github.com/GoogleChrome/puppeteer/blob/master/lib/FrameManager.js#L610

expect.toClick seems to use an elementHandle directly and that doesn't perform any implicit waiting:
https://github.com/smooth-code/jest-puppeteer/blob/master/packages/expect-puppeteer/src/matchers/toClick.js#L5

Perhaps if we don't expect any latency issues we should top using page.click.

- See REMOVE_BLOCKS effect in editor store's effect.js file. Final action when removing blocks is to handle selection of the previous block

- Replace `expect( page ).toClick` with `page.click`. The former won't show errors when used in a before/after. The latter also seems to better handle latency issues, the former was failing when used too soon after a page load. - Add waitForSelector to ensure the block appender is render before attempts to click it.

…rst step of a call to page.click

youknowriad · 2018-08-30T08:59:28Z

This does seem ready to me, would you mind having a final look @aduth?

aduth · 2018-08-30T12:47:51Z

Anyway, a bit of further investigation and I've pushed a commit that removes the waitForElement since puppeteer does that implicitly on any page.click (page.click calls through to frame manager's .click):
https://github.com/GoogleChrome/puppeteer/blob/master/lib/FrameManager.js#L610

The changes look good. It's not immediately obvious to me by the link where Puppeteer is waiting for the element's presence? The await is just as a signal that it's asynchronous. The waitForElement would literally poll the page continuously and only resolve once the element has been found. The former assumes the element exists when it's called, the latter doesn't, despite both being asynchronous and using await.

Anyways, since the code looks good and tests are passing, I'm assuming it's a non-issue now.

aduth

Looks great. Thanks for the patience in the delays working through the tests for an otherwise-simple change 😄

jasmussen · 2018-08-31T09:34:48Z

@talldan This is really really nice, thanks so much for working on this.

Since you've touched this, there's one other thing I would love fixed that's tangential to this.

Right now, if you select all blocks on a page and delete them, you've lost focus. It would be nice if when deleting all blocks on the page you simply set focus on the first appender. In other words, steps to reproduce:

Insert a bunch of blocks. Try the demo content.
Press ⌘A to select all blocks (⌘A twice if your caret is in text first)
Press del to delete those blocks.

Notice how after deleting those blocks, focus is lost — it would be nice if focus was simply restored to the appender:

aduth · 2018-08-31T14:27:54Z

@jasmussen This seems like a very reasonable expectation. Do we / should we have an issue tracking it?

jasmussen · 2018-08-31T14:42:41Z

@aduth I could swear I'd created one, but turns out I was thinking of #4915. Would you like me to create a new one?

talldan · 2018-09-05T10:32:30Z

@jasmussen A new issue would be good 👍

talldan added [Type] Enhancement A suggestion for improvement. [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Aug 14, 2018

talldan added this to the 3.6 milestone Aug 14, 2018

talldan self-assigned this Aug 14, 2018

talldan requested a review from noisysocks August 14, 2018 07:36

This was referenced Aug 14, 2018

Set focus on prior block after deleting multiple blocks #3558

Closed

Selection of the first block after deletion results in inconsistent caret position #8963

Closed

youknowriad requested a review from aduth August 15, 2018 13:10

youknowriad force-pushed the update/caret-position-when-removing-blocks branch from b4c9018 to 1644fe3 Compare August 15, 2018 13:34

youknowriad modified the milestones: 3.6, 3.7 Aug 16, 2018

designsimply mentioned this pull request Aug 17, 2018

When using the delete key to remove blocks, the caret is positioned incorrectly #8965

Closed

talldan force-pushed the update/caret-position-when-removing-blocks branch 2 times, most recently from 7edc8ba to 508177a Compare August 20, 2018 03:19

aduth reviewed Aug 20, 2018

View reviewed changes

aduth reviewed Aug 21, 2018

View reviewed changes

talldan added 2 commits August 28, 2018 13:05

Position caret at end of previous block for any type of block removal

28165de

Remove duplicate selectBlock dispatch

2f602f6

- See REMOVE_BLOCKS effect in editor store's effect.js file. Final action when removing blocks is to handle selection of the previous block

talldan added 10 commits August 28, 2018 13:05

Add e2e tests for various ways to delete blocks

722677b

Fix xpath selector for clicking on block settings menu item in e2e tests

b422271

Remove unecessary param

dcb784e

Use explicit key combination in test instead of predefining in utils

b5d07a5

Move clickOnBlockSettingsMenuItem function into the test file

f715b38

Remove duplicate snapshot assertion

03d400b

Ensure tests only have a single it statement per describe

522ab74

Remove waitForSelector since puppeteer does this implicitly as the fi…

7214eef

…rst step of a call to page.click

Update shortcut for remove block in block-deletion test

ef01a14

talldan force-pushed the update/caret-position-when-removing-blocks branch from dfc9709 to ef01a14 Compare August 28, 2018 05:21

aduth approved these changes Aug 30, 2018

View reviewed changes

aduth merged commit eac95b5 into master Aug 30, 2018

aduth deleted the update/caret-position-when-removing-blocks branch August 30, 2018 12:50

talldan mentioned this pull request Aug 30, 2018

Revert shortcut change in block-deletion e2e test #9477

Merged

jasmussen mentioned this pull request Sep 5, 2018

Prevent focus loss when deleting all blocks #9626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Position caret at end of previous block for any type of block removal #8961

Position caret at end of previous block for any type of block removal #8961

talldan commented Aug 14, 2018 •

edited

Loading

youknowriad commented Aug 15, 2018

aduth commented Aug 15, 2018

youknowriad commented Aug 15, 2018

aduth commented Aug 15, 2018

youknowriad commented Aug 16, 2018

talldan commented Aug 17, 2018

talldan commented Aug 20, 2018

aduth left a comment

aduth Aug 20, 2018

talldan Aug 21, 2018

aduth Aug 20, 2018

talldan Aug 21, 2018

aduth Aug 20, 2018

aduth Aug 20, 2018

talldan Aug 21, 2018

aduth Aug 20, 2018

talldan Aug 21, 2018

talldan commented Aug 21, 2018

aduth Aug 21, 2018

aduth Aug 21, 2018

aduth Aug 21, 2018

aduth Aug 21, 2018

talldan commented Aug 22, 2018 •

edited

Loading

talldan commented Aug 24, 2018

aduth commented Aug 24, 2018

talldan commented Aug 27, 2018

youknowriad commented Aug 30, 2018

aduth commented Aug 30, 2018

aduth left a comment

jasmussen commented Aug 31, 2018

aduth commented Aug 31, 2018

jasmussen commented Aug 31, 2018

talldan commented Sep 5, 2018

Position caret at end of previous block for any type of block removal #8961

Position caret at end of previous block for any type of block removal #8961

Conversation

talldan commented Aug 14, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

youknowriad commented Aug 15, 2018

aduth commented Aug 15, 2018

youknowriad commented Aug 15, 2018

aduth commented Aug 15, 2018

youknowriad commented Aug 16, 2018

talldan commented Aug 17, 2018

talldan commented Aug 20, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan commented Aug 22, 2018 • edited Loading

talldan commented Aug 24, 2018

aduth commented Aug 24, 2018

talldan commented Aug 27, 2018

youknowriad commented Aug 30, 2018

aduth commented Aug 30, 2018

aduth left a comment

Choose a reason for hiding this comment

jasmussen commented Aug 31, 2018

aduth commented Aug 31, 2018

jasmussen commented Aug 31, 2018

talldan commented Sep 5, 2018

talldan commented Aug 14, 2018 •

edited

Loading

talldan commented Aug 22, 2018 •

edited

Loading