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

[FF] Selection split by widgets should be merged in model to one range #4191

Closed
ma2ciek opened this issue Oct 2, 2017 · 4 comments · Fixed by ckeditor/ckeditor5-engine#1431
Labels
package:engine status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@ma2ciek
Copy link
Contributor

ma2ciek commented Oct 2, 2017

FF split the selection into two ranges because of the contenteditable=false attribute in the widget. This causes two problems. First, the selection that contains multiple ranges is not supported well yet, and second, that we can't select an image element.

@Reinmar Reinmar changed the title [FF] Split ranges by the Widget should be merged in model to one range [FF] Selection split by widgets should be merged in model to one range Oct 2, 2017
@Reinmar
Copy link
Member

Reinmar commented Oct 2, 2017

If we can do that, we would avoid many issues with handling unnecessarily multi-range selections.

Of course, if we'll fix the selection on model->view conversion, then it will be re-rendered and there's a question of what Firefox will do with it. Especially, if we'll re-render the selection while it's being made by dnd.

@scofalik
Copy link
Contributor

scofalik commented Oct 3, 2017

This isn't that easy because in FF I cannot drag selection after the image:

image

When I drag on a "Quote" the selection is not made after the image. Funny, when I drag from "Quote" backwards above the image it works correctly (in DOM - although I expected three ranges and there are two, but they cover the selected area).

I tried to overcome this issue by building a range that covers all ranges in the current selection, but FF has problems with setting selection over nested editable. Look at this fiddle: https://jsfiddle.net/kx2aatd6/2/

It doesn't work, even though you can make such range by placing selection in first paragraph and then holding Shift and clicking in the second paragraph. But even though the selection looks correct, when you, for example, hit Backspace only the range before nested editable and in nested editable are removed. So it doesn't work correctly in this case either.

Do you have any ideas how we could tackle this issue?

EDIT: Another funny thing: when you remove nested editable (just leave the #widget element) selecting through mouse drag works correctly, but selecting by code is still incorrect.

@scofalik
Copy link
Contributor

scofalik commented Oct 3, 2017

I had a wrong fiddle previously, updated it: https://jsfiddle.net/kx2aatd6/2/.

@scofalik
Copy link
Contributor

scofalik commented Oct 3, 2017

Hmm, it looks like not only Firefox has a problem with such selection... So does Chrome... Or I did something wrong?

Reinmar referenced this issue in ckeditor/ckeditor5-engine Jun 18, 2018
Feature: Introduced a selection post-fixer. Its role is to ensure that after all changes are applied the selection is placed in a correct position. Closes #1156. Closes #1176. Closes #1182. Closes ckeditor/ckeditor5-table#11. Closes ckeditor/ckeditor5-table#12. Closes ckeditor/ckeditor5-table#15. Closes ckeditor/ckeditor5#562. Closes ckeditor/ckeditor5#611.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine status:discussion 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