Opened 3 years ago

Closed 3 years ago

#13386 closed Bug (fixed)

[Edge] Issues with selection and editing images

Reported by: Olek Nowodziński Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.3
Component: Core : Selection Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

  1. Open old/replacebycode.html.
  2. Right-click the image in the contents. The image–specific menu item is missing.
  3. Double click the image.
  4. Dialog opens but dialog fields are unset/default.

It looks like Edge's selection doesn't go well images.

Attachments (1)

13386.html (1.4 KB) - added by Olek Nowodziński 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by Piotrek Koszuliński

Description: modified (diff)
Milestone: CKEditor 4.5.0CKEditor 4.5.1
Summary: [Spartan] Issues with selection and editing images[Edge] Issues with selection and editing images

We are still unable to work on such issues due to Edge's instability.

comment:2 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:3 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:4 Changed 3 years ago by Szymon Cofalik

I pushed a fix to branch:t/13386.

The fix is two-part. First of all, I changed one behavior so IE Edge works like Webkit. In previous IEs, clicking the image in contenteditable selected it, now it does not, so we use same mechanism for selecting like in Chrome etc.

Unfortunately, after applying this fix the selection flickered. You could drag the image, double click it or right click it and it worked but when you just clicked it, it was selected for very short time and it automatically unselected after browser fired all events. I have not found any place in code that might cause this so I assumed this is some kind of Edge behavior. This is fixed in https://github.com/ckeditor/ckeditor-dev/blob/feb64e521173f6e7d9a7be64eaf4c8e01782d89a/core/editable.js#L1066

If you happen to know what could cause this flickering, I'd appreciate a hint.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:5 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Changed 3 years ago by Olek Nowodziński

Attachment: 13386.html added

comment:6 Changed 3 years ago by Olek Nowodziński

Owner: changed from Szymon Cofalik to Olek Nowodziński
  1. I found out that the problem with disappearing selection is, in fact, a native one (see attachment:13386.html​). Older IEs display resize handlers once clicked an image. But Edge does nothing and I guess that this is the consequence of the move described on Edge blog:

    Our investigation revealed that the problem Medium was encountering was due to a non-standard legacy feature involving object-selection, which shipped in Internet Explorer 5. [...] It gave the user the option to re-size any element which had layout on first click, and on second click would allow them to edit, if the element were editable. [...] With this we decided to remove nearly 1500 lines of C++ code from the browser’s engine, as well as any dependencies.

  2. I simplified the solution by @scofalik in branch:t/13386b. I discovered that an extra click listener is obsolete if selection.selectElement() is called in tiemout on mousedown in Edge.
  3. There's an annoying Permission denied thrown while opening the Image dialog (#13574).

comment:7 Changed 3 years ago by Szymon Cofalik

#13554 should fix this and many other problems with Permission denied while opening dialog. I rebased on master and cherry-picked the commit from t/13554 and it seems that Permision denied are gone.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:9 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.2CKEditor 4.5.3
Status: reviewreview_failed

See http://jsfiddle.net/g5z9u4x8/2/

There's no need for any timeout. You simply need to prevent a default action of that click to be able to make a selection. This is pretty logical. Of course, there's an inconsistency between engines, but I was struggling with reporting it, because comparing FF, Chrome, IE11 and Edge everything works differently on every single browser. On FF for instance, clicking selects an image, on Chrome it doesn't, but event does not have to be cancelled, on IE11 you have the control selection and on Edge you don't have it, but clicking does nothing.

As preventing default action sounds rather well, we can add it on Edge only, or we can think about adding it on all browsers, but I'm a little bit worried about some issues with focus. Preventing a default action may prevent focusing the editor if it wasn't focused before clicking. Therefore, this must be checked (manual test with inline editor is needed).

comment:10 Changed 3 years ago by Szymon Cofalik

Owner: changed from Olek Nowodziński to Szymon Cofalik
Status: review_failedassigned

I'll try to finish this ticket and let Olek do some reviews :)

comment:11 Changed 3 years ago by Szymon Cofalik

Unfortunately: preventing mouseup or click did not removed faulty flickering and preventing mousedown also prevents native DnD.

comment:12 Changed 3 years ago by Szymon Cofalik

I've pushed branch:t/13386.

I changed .env.version to .env.edge and listening on click to mouseup. These changes were dictated by the fact that long mousedown did result in image losing selection (on both 13386 and 13386b but it is impossible to fix on 13386b).

Edit: I also rebased with master and did minor change in test's .md file.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:13 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

comment:15 Changed 3 years ago by Piotrek Koszuliński

I pushed branch:t/13386 with two small improvements. Can you confirm that the timeout which I removed isn't needed?

comment:16 Changed 3 years ago by Szymon Cofalik

It seems that it is indeed not needed.

comment:17 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with git:52ebb38.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy