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

Removing empty paragraph is merging and is bad UX #4434

Closed
scofalik opened this issue Oct 1, 2018 · 7 comments
Closed

Removing empty paragraph is merging and is bad UX #4434

scofalik opened this issue Oct 1, 2018 · 7 comments
Assignees
Labels
package:engine resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Oct 1, 2018

oct-01-2018 10-57-22

As you can see, there is an empty paragraph, and then there is a heading below it. Then, a user removes the empty paragraph. But under the hood, this is realized as a merge. So the heading is merged into an empty paragraph and the header becomes a paragraph.

AFAIR, I requested this "action" to be a merge because of collaboration :D. So when one user types in an empty paragraph and the other clicks Backspace in it, the text that was written by the first user is not deleted.

However, outside of a collaboration, it looks really bad. We should do something about it. It looks like a bug.

@Reinmar
Copy link
Member

Reinmar commented Oct 1, 2018

I think this is a DUP of https://github.com/ckeditor/ckeditor5-typing/issues/146.

@scofalik
Copy link
Contributor Author

scofalik commented Oct 1, 2018

I don't think so. This is a different scenario. The linked issue discusses which element should be merged into which. This issue discusses whether merge or remove should be used.

@Reinmar
Copy link
Member

Reinmar commented Oct 1, 2018

But you wrote this:

However, outside of a collaboration, it looks really bad. We should do something about it. It looks like a bug.

It looks like a bug because of https://github.com/ckeditor/ckeditor5-typing/issues/146. The used operation is a secondary thing here.

@scofalik
Copy link
Contributor Author

scofalik commented Oct 1, 2018

These two issues are not connected.

We don't want to merge in this scenario. When you press Delete in an empty paragraph, or Backspace in a header below an empty paragraph, you don't think about merging. You think about removing that blank space above your content. Merge direction is not important here cause we don't want to merge to happen anyway. Thus, the used operation is not a secondary thing.

@scofalik
Copy link
Contributor Author

scofalik commented Oct 1, 2018

From the top of my head, a solution could be implemented in MergeOperation#_execute. It could check, on execution, if the element-to-merge-into is empty. If so, it could simply remove it. But even if it was done on execution, it probably would have impact on OT.

@Reinmar
Copy link
Member

Reinmar commented Oct 2, 2018

We talked F2F and we kinda agree that:

  1. This is a special case of https://github.com/ckeditor/ckeditor5-typing/issues/146. But, this special case may actually require a different way of handling than https://github.com/ckeditor/ckeditor5-typing/issues/146 so fixing https://github.com/ckeditor/ckeditor5-typing/issues/146 without taking this case into consideration may not resolve the issue reported by @scofalik.
  2. Operations used to handle this are a secondary aspect (because this whole handling goes through deleteContent() and the UX of this method should be fixed). However, using merge will give better results in OT. So we may consider tripled rename(leftBlock) + setattrs(leftBlock,rightBlock.getAttrs()) + merge to perform a "merge right" kind of action.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the nice-to-have milestone Oct 9, 2019
@mlewand mlewand added module:model type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
@niegowski niegowski modified the milestones: nice-to-have, iteration 33 May 25, 2020
@niegowski niegowski self-assigned this May 25, 2020
@niegowski
Copy link
Contributor

Issue #6680 is addressing this (and is aware of OT and "right merge").

@niegowski niegowski added the resolution:duplicate This issue is a duplicate of another issue and was merged into it. label May 26, 2020
@Reinmar Reinmar removed this from the iteration 33 milestone May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants