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

Backspace should not change the type of the trailing block (probably shouldn't merge it at all) #6680

Closed
Reinmar opened this issue Apr 27, 2020 · 7 comments · Fixed by #7270
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:engine package:typing type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2020

📝 Provide detailed reproduction steps (if any)

When having the following selection:

<paragraph>[Foo</paragraph>
<paragraph>Bar</paragraph>
<heading1>]Bom</heading1>

I expect to end with this after pressing Backspace:

<paragraph>[]</paragrph>
<heading1>Bom</heading1>

Actual result

The format of the trailing block is changed to the format of the former block. Technically speaking, the heading is being merged into the first paragraph.

UX

This is especially irritating when you try to delete e.g. paragraphs between 2 headings:

I get the "Proposal" heading turned into a paragraph. What I'd expect is having the two paragraphs above deleted, one left and the heading not touched at all:

Background

The trailing block problem is a well-known DOM selection representation issue. For instance, when you triple-click a line of text to select the entire line, the selection looks like this:

<paragraph>Foo</parargaph>
<paragraph>[Trippleclicked line</parargaph>
<paragraph>]Foo</parargaph>

Unfortunately:

  • We didn't plan to select the "Foo" block. It got selected by the browser because that's the only way how it represents a "new line is selected too" situation which is a different situation than selecting just the content of this block.
  • You don't see in Chrome (FF and Safari handle this better) that the end of line is selected too.

We already handled this situation in the selection.getSelectedBlocks() method, so e.g. when yoy apply a block format to this selection, it does not affect the "Foo" block.

It'd now be good to also handle this in deleteContent() by simply ignoring the trailing block if the selection starts at its beginning.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. domain:ui/ux This issue reports a problem related to UI or UX. package:engine package:typing type:improvement This issue reports a possible enhancement of an existing feature. and removed type:bug This issue reports a buggy (incorrect) behavior. labels Apr 27, 2020
@Reinmar Reinmar added this to the iteration 32 milestone Apr 27, 2020
@oleq
Copy link
Member

oleq commented Apr 27, 2020

What does it work like in other editors?

@wwalc
Copy link
Member

wwalc commented Apr 27, 2020

Yeah, it happens to me too whenever I use shift + down arrow key to select blocks. Funny I never reported it... 

Just a note to be careful when working on it, so that the way how CKEditor 5 handles backspace did not break lists handling (if it may be in some way influenced by the fix implemented to solve this issue at all).

@niegowski
Copy link
Contributor

Selection cases

1. Vertical selection (mouse or keyboard)

Mouse dragged vertically along the block elements (or keyboard up / down arrows) makes the selection expand to the start of next block element. 
 

2. Mouse tripple-click selection

Behaves exactly as vertical selection (includes start of next block element).

3. Horizontal selection (mouse or keyboard)

Mouse dragged horizontally over the block elements (or keyboard Shift + left / right / home / end arrows) does not expand the selection to any other block element. 
 

4. DeleteCommand

It's expanding the selection by one character in the direction of the command (Backspace - "backward", Delete - "forward") and then triggering Model#deleteContent() on that selection. 

5. EnterCommand

For the expanded selection is triggering Model#deleteContent() on it with leaveUnmerged option set to true if the selection is not from start to end of an element (horizontal selection of whole paragraph). leaveUnmerged keeps current block element in place but selection is moved to the start of next block element.

6. Paste from Clipboard

Content in clipboard does not contain trailing newline and after pasting it in front of some block they are merged.

@niegowski
Copy link
Contributor

Problems

Cases 1 and 2 - the selection includes the start position of next block element

After deleting content the following block element should keep its type and attributes no matter whether it was Delete or Backspace (GDocs behave in this way).

Case 3 - the selection doesn't include next block

This case works as in GDocs and seems reasonable.

Case 4 - DeleteCommand with empty block before the selection

Since DeleteCommand is expanding selection this case is very similar to cases 1 and 2 (here is no content, but in cases 1 and 2 after removing text from the selection this case is identical).

Case 5 - EnterCommand

Selection should not jump to the start position of following block element.

Case 6 - Paste from Clipboard

The clipboard data should contain trailing block if it was selected with it while copying/cutting.

@niegowski
Copy link
Contributor

Described expected behavior is not sth GDocs do.

When having the following selection:

<paragraph>[Foo</paragraph>
<paragraph>Bar</paragraph>
<heading1>]Bom</heading1>

I expect to end with this after pressing Backspace:

<paragraph>[]</paragrph>
<heading1>Bom</heading1>

GDocs results with:

<heading1>[]Bom</heading1>

But title of this issue "Backspace should not change the type of the trailing block" is a valid point of the need of not merging those blocks as we do it currently and this is really annoying UX. 

How this currently works is a result of OT's preferred way of handling such situations, but as discussed in #4434 there is an option of "right-merging" those elements.

From cases described above there are 3 areas where we can improve:

  • Model#deleteContent should "merge right" in cases when left side of merging is empty and right one has content so that the properties of element with some content is preserved.
  • EnterCommand should not move caret to the next block if this wasn't intended/expected.
  • Model#getSelectedContent seems to be to aggressive when cleaning the document fragment while handling copy/cut.

@niegowski
Copy link
Contributor

The Model#deleteContent issue is addressed in PR #7270.

I think there should be followups for:

  • EnterCommand 
  • Model#getSelectedContent

@niegowski
Copy link
Contributor

After f2f talk with @Reinmar - there is one more use case:

Due to behavior of Enter (which is consistent with GDocs and Pages) after removing whole paragraphs hitting Enter creates another header block but in this scenario paragraph would be more reasonable so it seems it's better to not merge them. 

So more user friendly behavior would be (despite it's not consistent with other editors):

jodator added a commit that referenced this issue Jun 4, 2020
Fix (engine): Backspace will not change the type of the trailing block. Closes #6680.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:engine package:typing type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants