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: 'Down' key at bottom creates new paragraph #3973

Merged
merged 4 commits into from Dec 14, 2017

Conversation

Projects
None yet
5 participants
@mcsf
Contributor

mcsf commented Dec 13, 2017

Description

Keyboard-only alternative to #3623 (see #3078).

  • When at the bottom of the editor's content, pressing the down arrow focuses a brand new paragraph (or whichever is the default block type).
  • The initial motivation was the fact that, whenever relying on the keyboard for quick editing and insertion of blocks, it becomes cumbersome to manually break out of most blocks and create a new one.
  • Blocks like Paragraph, Heading or List allow the user to split into a new empty block by pressing Enter at the edge of the block, but this isn't the case for other blocks.
  • However, pressing Enter at the edge of a Quote block just adds a line within the citation field of that Quote. And while Enter does work for e.g. Image, right now this isn't visually clear (see #3951 for improvements).
  • We could try to generically pick up these Enter events and append an empty block, but maybe there is value in adding this -based mechanism—regardless of Enter improvements.

How Has This Been Tested?

  1. Add a Quote
  2. Fill fields quote and citation
  3. From the citation field, expect to press Enter to move on to a new paragraph (or indeed any block thanks to / autocompletion, which I use with a passion).
  4. Notice how Enter adds a line break within citation, so press Backspace to undo
  5. Press , notice how the caret has moved into a new empty block.

Screenshots (jpeg or gifs if applicable):

gutenberg-writing-flow-down-arrow

Types of changes

Writing flow enhancement.

Checklist:

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

@mcsf mcsf added the Writing Flow label Dec 13, 2017

@mcsf mcsf requested review from youknowriad and jasmussen Dec 13, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 13, 2017

Contributor

I love it! All of these little flow improvements really add up, and this is a nice one. I still think we need #3078 so you can also click at the end of a block list (after an image block for example), but this feels like it ties into that.

Looks like there's a visual regression in the blockquote citation. The font should be smaller and gray, which it is in master.

Contributor

jasmussen commented Dec 13, 2017

I love it! All of these little flow improvements really add up, and this is a nice one. I still think we need #3078 so you can also click at the end of a block list (after an image block for example), but this feels like it ties into that.

Looks like there's a visual regression in the blockquote citation. The font should be smaller and gray, which it is in master.

@youknowriad

Some questions:

Do you think we should allow this behavior on "text" blocks only (all blocks with caret) or we should use "enter" in these cases?

If we were to do so, why it's not working on empty paragraphs?

/**
* Internal dependencies
*/
import './style.scss';
import BlockDropZone from '../block-drop-zone';
import { insertBlock } from '../../actions';
import { appendDefaultBlock } from '../../actions';
import { getBlockCount } from '../../selectors';
export class DefaultBlockAppender extends Component {

This comment has been minimized.

@youknowriad

youknowriad Dec 13, 2017

Contributor

I guess this could be a functional component now :)

@youknowriad

youknowriad Dec 13, 2017

Contributor

I guess this could be a functional component now :)

This comment has been minimized.

@mcsf

mcsf Dec 13, 2017

Contributor

My thinking too, but jest wasn't happy and I left for the sake of opening the PR :)

@mcsf

mcsf Dec 13, 2017

Contributor

My thinking too, but jest wasn't happy and I left for the sake of opening the PR :)

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 13, 2017

Contributor

Do you think we should allow this behavior on "text" blocks only (all blocks with caret) or we should use "enter" in these cases?

Not sure what you mean. Do you mean should the append a default block only when the block before it is a textual block? On first thought it seems it should be the behavior for all blocks (and the implementation kind of achieves this by looking at tabbables).

If we were to do so, why it's not working on empty paragraphs?

Similarly to how #3623 had this issue with endless appending, the current implementation bails if the last tabbable is empty. One glaring issue with that is the following, which can be reproduced:

  1. Add a Quote block.
  2. Fill out quote but not citation.
  3. Keeping pressing , hoping to move into an empty appendage, but nothing happens.

Perhaps we can be smarter and look not just at the last tabbable (in this case, the footer inside Quote) but also its tabbable siblings within the block. If at least one of those is non empty, then allow the appending and switching into an empty default block.

Contributor

mcsf commented Dec 13, 2017

Do you think we should allow this behavior on "text" blocks only (all blocks with caret) or we should use "enter" in these cases?

Not sure what you mean. Do you mean should the append a default block only when the block before it is a textual block? On first thought it seems it should be the behavior for all blocks (and the implementation kind of achieves this by looking at tabbables).

If we were to do so, why it's not working on empty paragraphs?

Similarly to how #3623 had this issue with endless appending, the current implementation bails if the last tabbable is empty. One glaring issue with that is the following, which can be reproduced:

  1. Add a Quote block.
  2. Fill out quote but not citation.
  3. Keeping pressing , hoping to move into an empty appendage, but nothing happens.

Perhaps we can be smarter and look not just at the last tabbable (in this case, the footer inside Quote) but also its tabbable siblings within the block. If at least one of those is non empty, then allow the appending and switching into an empty default block.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 13, 2017

Contributor

56e22cd addresses my previous comment:

Perhaps we can be smarter and look not just at the last tabbable (in this case, the <footer> inside Quote) but also its tabbable siblings within the block. If at least one of those is non empty, then allow the appending and switching into an empty default block.

Contributor

mcsf commented Dec 13, 2017

56e22cd addresses my previous comment:

Perhaps we can be smarter and look not just at the last tabbable (in this case, the <footer> inside Quote) but also its tabbable siblings within the block. If at least one of those is non empty, then allow the appending and switching into an empty default block.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 13, 2017

Contributor

Not sure what you mean. Do you mean should the ↓ append a default block only when the block before it is a textual block?

Well! actually the opposite (I wrote the opposite of what I was thinking :) ). It should append a default block only when the block before is not a textual block because in a textual block (think a unique text area for everything), clicking ↓ does not create an empty line.

But I can be on board with the current behavior, just wanted to bring this up :)

Contributor

youknowriad commented Dec 13, 2017

Not sure what you mean. Do you mean should the ↓ append a default block only when the block before it is a textual block?

Well! actually the opposite (I wrote the opposite of what I was thinking :) ). It should append a default block only when the block before is not a textual block because in a textual block (think a unique text area for everything), clicking ↓ does not create an empty line.

But I can be on board with the current behavior, just wanted to bring this up :)

@youknowriad

Left some optional comments but this is nice

@@ -532,6 +532,8 @@ const mapDispatchToProps = ( dispatch, ownProps ) => ( {
},
} );
BlockListBlock.className = 'editor-block-list__block-edit';

This comment has been minimized.

@youknowriad

youknowriad Dec 13, 2017

Contributor

❤️

@youknowriad

youknowriad Dec 13, 2017

Contributor

❤️

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 13, 2017

Contributor

Looks like there's a visual regression in the blockquote citation. The font should be smaller and gray, which it is in master.

I see the regression, but I'm also seeing it in master. Can anyone confirm?

Contributor

mcsf commented Dec 13, 2017

Looks like there's a visual regression in the blockquote citation. The font should be smaller and gray, which it is in master.

I see the regression, but I'm also seeing it in master. Can anyone confirm?

Show outdated Hide outdated editor/effects.js
Show outdated Hide outdated editor/components/writing-flow/index.js
Show outdated Hide outdated editor/components/writing-flow/index.js

mcsf added some commits Dec 13, 2017

WritingFlow: 'down' appends new block after any non-empty block
Fixes the following issue introduced by the parent commits:

1. Add a Quote block.
2. Fill out quote but not citation.
3. Keeping pressing ↓, hoping to move into an empty appendage, but
   nothing happens.

Also correctly account for block-level-only tabbables, e.g. Image.
@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Dec 14, 2017

Contributor

All good tips, @aduth. Amended.

Contributor

mcsf commented Dec 14, 2017

All good tips, @aduth. Amended.

@mcsf mcsf merged commit 29b757d into master Dec 14, 2017

2 of 3 checks passed

codecov/project 37.61% (-0.72%) compared to d94d742
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mcsf mcsf deleted the update/writing-flow-down-for-paragraph branch Dec 14, 2017

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 10, 2018

Contributor

This is cool, nice work.

Contributor

mtias commented Jan 10, 2018

This is cool, nice work.

aduth added a commit that referenced this pull request Mar 2, 2018

Block: Remove unused block classname constant
Originally used for down-to-new-paragraph in #3973, removed as of #5271

aduth added a commit that referenced this pull request Mar 6, 2018

Block: Remove unused block classname constant
Originally used for down-to-new-paragraph in #3973, removed as of #5271

aduth added a commit that referenced this pull request Mar 7, 2018

Writing Flow: Move selection to block's focus handler (#5289)
* Writing Flow: Select block by own focus handler

* Block List: Mark default appender events as block-handled

* Writing Flow: ...

* Block: Move tabIndex to wrapper component

See: #2934
See: https://codepen.io/aduth/pen/MQxRME

Something needs to capture the focus event, which is skipped by button press on the block's toolbar, but bubbles up wrongly to the parent's edit wrapper (which has tabIndex) and causes parent block to select itself.

* Writing Flow: Update block focusable target

* Block: Remove unused block classname constant

Originally used for down-to-new-paragraph in #3973, removed as of #5271

* Block List: Capture focus event on block wrapper

Corresponding to move of tabIndex from inner edit element to wrapper, we also need to capture focus from wrapper. This way, when focus is applied via WritingFlow, the block appropriately becomes selected. This may also make "onSelect" of movers unnecessary.

* Block List: Remove pointer handling of onSelect

Can now rely on focus behavior captured from wrapper node to reflect selection onto blocks, even those without their own focusable elements

* Block List: Avoid select on focus if in multi-selection

Multi-selection causes focus event to be triggered on the block, but at that point `isSelected` is false, so it will trigger `onSelect` and a subsequent `focusTabbables`, thus breaking the intended multi-selection flow.

* Block Mover: Remove explicit select on block move

Focus will bubble to block wrapper (or in case of Firefox/Safari, where button click itself does not trigger focus, focus on the wrapper itself) to incur self-select.

* Block List: Singular select block on clicking multi-selected

This reverts commit 90a5dab.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment