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] Complex selection when flashes spanning across a widget #4364

Closed
oleq opened this issue Jun 19, 2018 · 7 comments
Closed

[FF] Complex selection when flashes spanning across a widget #4364

oleq opened this issue Jun 19, 2018 · 7 comments
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@oleq
Copy link
Member

oleq commented Jun 19, 2018

A followup of ckeditor/ckeditor5-engine#1431 (comment).

kapture 2018-06-19 at 11 39 26

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

I checked how the selection post-fixer fixes the selection:

image

The fixed ranges ("before combining") are ok. The thing is, though, that they don't get combined. We select two ranges and I think that it may be breking Firefox. Those not combined ranges look reasonably, but still – Firefox starts doing weird things.

Another thing is that the whole selection is blinking. I think that we may need to stop re-rendering while it's being "done". E.g. I changed toWidget() so it stops setting cE=false. We stopped having the two-range selection problem but blinking was severe. So we'll have to fix two things here – setting a reasonable selection and avoiding blinking.

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

I made a quick fix which better merges ranges:

image

It's not valid cause it looks like this now:

		if ( combinedRanges.length > 1 ) {
			const r = new Range( combinedRanges[ 0 ].start, combinedRanges[ 1 ].end );
			combinedRanges = [ r ];
		}

But it prevented breaking the selection.

jul-04-2018 16-44-44

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

In fact... I think that we can do a quick fix here to always combine multi-range selections into a single-range selection. This only applies to Firefox and if it won't break anything in tables (which I think it won't), then we'll at least prevent breaking the selection. It will still blink, but we can handle this separately.

@jodator
Copy link
Contributor

jodator commented Jul 5, 2018

@Reinmar The proposed fix looks OK in this scenario. What about actual mutli-range selection?

I think that we cannot blindly merge mutli selection into one big selection:

  1. Select some parts of text before image:
    selection_002

  2. Merge combinedRanges:
    selection_003

ps.: Currently I'm debuging mergin only one of ranges and it looks like it doesn't work... only changing selection to one range looks like it "fixes" something.

@Reinmar
Copy link
Member

Reinmar commented Jul 5, 2018

I think that we cannot blindly merge mutli selection into one big selection:

We don't have to support multi-range selections. Only one browser supports them anyway. And we can make shortcuts as I don't think any user actually knows how to create such selections.

Also, we're talking about combining ranges if they were fixed. Not always.

@jodator
Copy link
Contributor

jodator commented Jul 5, 2018

Also, we're talking about combining ranges if they were fixed. Not always.

Yep only then it will happen so after testing it I'm OK with this change anyway.

I will make this change part of ckeditor/ckeditor5-engine#1450.

@Reinmar
Copy link
Member

Reinmar commented Jul 6, 2018

I extracted the "flashing" part to https://github.com/ckeditor/ckeditor5-engine/issues/1457.

The fact that Firefox was breaking the selection (you were losing the anchor) was fixed in ckeditor/ckeditor5-engine#1450 so I'm closing this ticket.

@Reinmar Reinmar closed this as completed Jul 6, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 19 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. 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 type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

4 participants