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

Fix Deleting multiselected blocks #2103

Merged
merged 1 commit into from
Aug 2, 2017
Merged

Fix Deleting multiselected blocks #2103

merged 1 commit into from
Aug 2, 2017

Conversation

youknowriad
Copy link
Contributor

closes #2101

The code handling this was there but not being triggered properly.

Testing instructions

  • Select all blocks (ctrl+A)
  • Hit delete or backspace: the blocks should be deleted.

@youknowriad youknowriad self-assigned this Jul 31, 2017
@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Jul 31, 2017
@jasmussen
Copy link
Contributor

Yaaaay! Love it. 👍 👍

@codecov
Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #2103 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2103      +/-   ##
==========================================
- Coverage   20.34%   20.33%   -0.01%     
==========================================
  Files         135      135              
  Lines        4233     4234       +1     
  Branches      721      720       -1     
==========================================
  Hits          861      861              
- Misses       2841     2843       +2     
+ Partials      531      530       -1
Impacted Files Coverage Δ
editor/modes/visual-editor/block-list.js 0% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️

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 dbc8680...784aad0. Read the comment docs.

@notnownikki
Copy link
Member

I was able to break it in a weird way...

I can select the blocks in such a way that I have blocks selected, and I'm editing a single block at the same time. Then, if I press backspace, I can end up merging two blocks instead of deleting them.

multiselectproblem

@youknowriad
Copy link
Contributor Author

@notnownikki Not sure how to reproduce this bug (which browser is this?).

Anyway, the fact that we're storing the selection and the multi-selection in two different reducer is not a good thing and could lead to this kind of bugs, we should make sure to improve these reducers separately (cc @iseulde )

@ellatrix
Copy link
Member

ellatrix commented Aug 1, 2017

we should make sure to improve these reducers separately

Are you saying we should keep them separate or merge them? I'll have a look at this this afternoon!

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 1, 2017

@iseulde I'm saying we should merge them so it's one way or another. We wouldn't have the possibility to have a multiselection and a single selected block in the same time if they are stored in the same reducer.

@notnownikki
Copy link
Member

@youknowriad @iseulde this is how I reproduced it. It's probably a "nikkibug" - that is, something I trigger every time, and no one else does until I point it out... 😄

Using chrome.

  1. Create three text blocks, with the text "top", "middle", and "bottom".
  2. In the bottom block, place the text cursor at the end of the word "bottom".
  3. Drag to the beginning of the word "bottom" to select it.
  4. Release the mouse button.
  5. Move the mouse cursor to the end of the word "bottom".
  6. Drag straight upward to select all three blocks.
  7. Drag the mouse cursor to the beginning of the word "top" in the top block.
  8. Drag the mouse cursor straight down to the bottom block so it is at the beginning of the word "bottom".
    You should see the word "bottom" is no longer selected, and the text cursor is at the start of the word "bottom".
  9. Continue dragging upward to select all three blocks. You should see the text cursor is still active in the bottom block, but all three blocks are selected.
  10. Press backspace, and the bottom block will merge into the block above it and lose focus.

weird selection bug

@notnownikki
Copy link
Member

Further info: this doesn't happen in Chrome 60 or Firefox, but does in Chrome 59 (exact version number: 59.0.3071.109)

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 1, 2017

Ok, So I think we should merge this and consolidate the selections reducer separately.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Works great! Awesome!

@ellatrix
Copy link
Member

ellatrix commented Aug 1, 2017

@notnownikki Could you open a separate issue the bug then? :)

@youknowriad youknowriad merged commit 24fed65 into master Aug 2, 2017
@youknowriad youknowriad deleted the fix/delete-all branch August 2, 2017 08:11
ceyhun pushed a commit that referenced this pull request Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-select: backspace or delete, should delete selection
4 participants