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

Should deleteForward change the direction of model#deleteContent() merge blocks? #3115

Closed
Reinmar opened this issue Apr 23, 2018 · 11 comments
Closed
Labels
package:typing resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 23, 2018

Let's see the two following cases:

TC1: <h1>Foo</h1><p>[]Bar</p> + delete (backspace)
TC2: <h1>Foo[]</h1><p>Bar</p> + forward delete

Now, we could handle these cases differently. Right now, in both of them the result will be <h1>Foo[]Bar</h1>. However, perhaps the user's intention is a bit different when pressing the backward and forward delete keys.

When using the editor I'm expecting that forward delete will merge the current block "to the right", so it will keep that block (<p>Foo[]Bar</p>). We should perhaps check some other editors to see if they handle forward delete differently.

@scofalik
Copy link
Contributor

I was actually thinking about the exact same thing like 5 minutes ago. When thinking about scenarios in OT, it just slipped my mind if maybe user has different intentions in those two scenarios. I don't have a case for it in OT, I just thought about it.

Can you give examples of cases where there are two possible intentions? In the given example, I don't see what different effect the user might have wanted to achieve.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 2, 2018

There's a special case reported in https://github.com/ckeditor/ckeditor5-engine/issues/1566. If we'll decide to modify forward delete behaviour by doing "merge right" (as I described in https://github.com/ckeditor/ckeditor5-engine/issues/1566#issuecomment-426200627) we may fix this general issue reported here and the special case reported in https://github.com/ckeditor/ckeditor5-engine/issues/1566.

@f1ames
Copy link
Contributor

f1ames commented Oct 2, 2018

Checked how this works in Microsoft Word 2016, for both TC1 and TC2 the result is <h1>Foo[]Bar</h1>:

test1 msw

In special case mentioned in https://github.com/ckeditor/ckeditor5-typing/issues/146#issuecomment-426201678, the header remains a header when deleted forward to an empty paragraph:

test2 msw

and the position of the caret (if in header or paragraph) doesn't matter in this case:

test3 msw

@scofalik
Copy link
Contributor

scofalik commented Oct 2, 2018

Yeah when I re- read this issue I actually did not understand what was the problem. Keeping the previous element (header in this case) in all scenarios is the only reasonable expectation.

@jodator
Copy link
Contributor

jodator commented Feb 21, 2019

I think that for empty elements it might remove the block instead of merging:

peek 2019-02-21 16-46

The above feels buggy.

@scofalik
Copy link
Contributor

scofalik commented Feb 21, 2019

Okay, so I think that the behavior could be to always favor the non-empty element:

<p>[]</p><h2>Foo bar</h2> -> delete key -> <h2>Foo bar</h2>
<h2>[]</h2><p>Foo bar</p> -> delete key -> <p>Foo bar</p>
<h2></h2><p>[]Foo bar</p> -> backspace key -> <p>Foo bar</p>

Of course, this works only when the first element is empty. If the first element is not empty I think that it should keep it's formatting (paragraph / heading) no matter what is the next element and no matter if backspace or delete was used. So basically the below element is always merged to the above element.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 14, 2019

One more TC: #1630

@Reinmar
Copy link
Member Author

Reinmar commented Sep 16, 2019

One more TC: #895

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:typing labels Oct 9, 2019
@zehawki
Copy link

zehawki commented Feb 18, 2020

Thats a complex issue name and I for one would not have matched it to what I reported a while ago on #6282 :-)

Note that the behavior I'm reporting / existing is a subset of the current issue you are discussing - I'm only thinking of the case where the entire line (previous element) has been deleted, the tags should be deleted as well.

Copy pasting from that issue for easy reference:

The para text takes the formatting of the previous line even though it was deleted.

Here's a gif showing whats going on:

captured

I looked into the HTML as its being generated and see the issue:

After the line is deleted, the formatting tags are not removed along with the line, so the HTML contains:
<h2>&nbsp;</h2><p>Like all the great things on earth traveling teaches us by example. Here are some of the most precious lessons I’ve learned over the years of traveling.</p>
And then when delete is pressed, the next para text takes the h2 tag.

This is a very unintuitive behavior and something I've not seen in any other editor. To cross check I performed exactly the same in Word, Google docs, Quill and clearly they handle this as expected.

Froala does the same thing as CK, BUT it has a saving grace - when the heading line is deleted and another backspace is pressed, the line formatting disappears.

I strongly feel this behavior should be changed since its quite unexpected & puzzling for the end users (who reported this issue to me) and I find it equally puzzling :-)

@Reinmar
Copy link
Member Author

Reinmar commented Feb 28, 2020

This issue is blocked by #6355.

@Reinmar Reinmar modified the milestones: nice-to-have, iteration 32 May 7, 2020
@niegowski niegowski self-assigned this May 21, 2020
@Reinmar Reinmar modified the milestones: iteration 32, next, iteration 33 May 25, 2020
@niegowski niegowski modified the milestones: iteration 33, backlog May 26, 2020
@niegowski niegowski removed their assignment May 26, 2020
@Reinmar
Copy link
Member Author

Reinmar commented May 27, 2020

I tested GDocs and Pages and can't find differences between fwd and backward deleting, so I guess it doesn't make sense to introduce them.

Also #6680 will actually resolve the biggest issues with how backspace works currently.

@Reinmar Reinmar closed this as completed May 27, 2020
@Reinmar Reinmar added resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). and removed squad:red labels May 27, 2020
@Reinmar Reinmar removed this from the backlog milestone May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:typing resolution:invalid This issue is invalid (e.g. reports a non-existent bug or a by-design behavior). status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

7 participants