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

Writing Flow: Ensure default block on removal / replacement #9977

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 17, 2018

Closes #9626

This pull request seeks to ensure that a default block is inserted (and selected) when all other blocks are removed. This helps assure there is no focus loss, and preserves expected native behavior of typing within an editable field, where pressing backspace from the beginning of an otherwise empty field should not incur a focus loss.

Note that as of #9808, in these cases where only a default empty paragraph exists, the saved content will not effectively include the paragraph, but will instead fall back to an empty string, as it's inferred now that a post containing only the default unmodified block is equivalent to being empty.

Testing instructions:

Verify that if all blocks are removed, a default block is inserted at the beginning of the post and that focus is preserved.

  1. Navigate to Posts > Add New
  2. Click the Writing Prompt to insert a new paragraph
  3. Press Backspace
  4. Note that the caret is still focused within a paragraph

Ensure end-to-end tests pass:

npm run test-e2e

@aduth aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Sep 17, 2018
@aduth aduth added this to the 4.0 milestone Sep 17, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Everything worked as expected on my tests 👍 And the code changes look good to me. They contain nice code improvements.
I wonder If we should add a smilar behaviour for nested blocks, e.g: when we delete the last paragraph inside a column the focus is lost, maybe we should ensure a similar behaviour there.

@@ -181,4 +181,34 @@ describe( 'splitting and merging blocks', () => {

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'should ensure always a default block', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Probably as a follow up after this PR when dealing with this file we should rename the file and the description, as right now it is testing other behaviors like 'should delete an empty first line', and this test case that are not directly related with 'splitting and merging blocks'.

@aduth
Copy link
Member Author

aduth commented Sep 18, 2018

Re: Nested blocks, it was discussed a bit in #9626. I'd probably agree we don't want focus loss at all, so it may be sensible there as well. A difference might be that in a nested context, the default block might not always be allowed, so not only do we need to account for that, we'd need to decide what to do as a fallback.

It's also something I considered briefly for this as well, at least as far as the default block not necessarily being the paragraph. For example, if the default block was an image, would it make sense to add a default image block when all other blocks are removed? I don't know that this would be a common case (having image block as default would have many other odd consequences already). But worth pointing out that there's something about the paragraph and this expectation of how the block listing is meant to mimic the behaviors of a native textarea input / word processor.

Which is all to say: Maybe, but we might need to discuss further, and I'd suggest deferring it to a subsequent pull request. Perhaps we can keep #9626 open and continue the discussion there.

@aduth aduth merged commit 6990448 into master Oct 5, 2018
@aduth aduth deleted the update/ensure-default branch October 5, 2018 14:06
@mtias
Copy link
Member

mtias commented Oct 9, 2018

Nice improvement, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants