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: Fix focus/keyboard issues with multi-select #3297

Merged
merged 5 commits into from
Nov 2, 2017

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Nov 1, 2017

Description

#3253 introduced a couple of issues. See:

This PR reverts the offending commit from #3253, cherry-picks @iseulde's work in #3222, and adds a particular guard against a TypeError reading focus.collapsed.

Testing

  • Try multi-selecting blocks with both arrows and pointer.
  • Try deleting selections of multiple blocks.
  • Try undoing such deletions.
  • Try navigating areas of the page with the keyboard, e.g. the toolbar of a block, the focusable elements of the sidebar; try the same when multiple blocks are selected.
  • Try selecting multiple blocks with the keyboard, then, with the reverse motion on the keys, coming back to a single-block selection. As of this writing, the ability to keep moving with the keys after returning to a single-block selection is lost. This is likely because d5fdc8c is just a patch that works around the issue, which needs to be properly tackled.

Types of changes

Bug fix.

Checklist:

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

@mcsf mcsf added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Nov 1, 2017
@mcsf mcsf requested review from ephox-mogran and ellatrix November 1, 2017 17:23
@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #3297 into master will increase coverage by 0.07%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3297      +/-   ##
==========================================
+ Coverage   31.05%   31.13%   +0.07%     
==========================================
  Files         232      232              
  Lines        6521     6524       +3     
  Branches     1163     1164       +1     
==========================================
+ Hits         2025     2031       +6     
+ Misses       3770     3767       -3     
  Partials      726      726
Impacted Files Coverage Δ
editor/block-settings-menu/index.js 0% <ø> (ø) ⬆️
editor/writing-flow/index.js 0% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
components/icon-button/index.js 100% <100%> (ø) ⬆️
components/button/index.js 90.9% <90.9%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35fcb32...d5fdc8c. Read the comment docs.

@ephox-mogran
Copy link
Contributor

As mentioned in a DM (and as @iseulde had considered), we could make the reducer for multi-select switch focus to {} or state.focus (if set) instead of null if the selection start and end were the same. This would mirror how insertBlocks and replaceBlocks, and selectBlock works (focus is initially {}). However, you would lose specific focus things in the block, but maybe that's not important. I think this is an important issue to get fixed (losing your cursor when you condense your selection again), but I'm happy to approve this not crashing fix. which probably should take priority ;)

P.S. It's a shame it doesn't container the other fix in #3280 because that is a bug waiting to happen. But it can be done separately as well.

@ephox-mogran
Copy link
Contributor

@mcsf and @iseulde , it's probably time to revisit this. It is a frustrating user experience that you lose your selection as soon as you condense your multi-block selection back to just one block. It means that you can't multi-select down, change your mind, and then multi-select up.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 20, 2017

@ephox-mogran, totally agree. It's one of my personal pet peeves when using Gutenberg.

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...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants