Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#13441 closed Bug (fixed)

[Edge] Broken clipboard integration

Reported by: Olek Nowodziński Owned by: Szymon Cofalik
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.2
Component: Core : Undo & Redo Version: 4.5.0
Keywords: Cc:

Description

  1. Open samples/old/inlineall.html
  2. Select all contents of one editor.
  3. CTRL+C.
  4. Focus another editor.
  5. CTRL+V.

Expected: Contents pasted, undo enabled.

Actual: Contents pasted, undo disabled.

Then:

  • Repeat 5. as many times as you wish. Undo still disabled.
  • Type a letter. Undo disabled.
  • Type another letter, undo is enabled but snapshots make no sense.

Change History (20)

comment:1 Changed 3 years ago by Piotr Jasiun

Priority: NormalHigh

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

The problem probably lies in this block of code:

editable.on( 'beforepaste', function( evt ) {
	// Do not prevent event on CTRL+V and SHIFT+INS because it blocks paste (#11970).
	if ( evt.data && !evt.data.$.ctrlKey && !evt.data.$.shiftKey )
		preventBeforePasteEventNow();
}, null, null, 0 );

This event is registered for IE only. On older versions of IE clipboard events had .ctrlKey, .altKey and .shiftKey properties. Since Edge, the ClipboardEvent is implemented correctly and it does not have those properties. Thus, on Edge, undo manager callbacks are always prevented for both paste and beforePaste events.

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

comment:5 Changed 3 years ago by Piotr Jasiun

I think we should give Edge a try and thread it like a normal browser.

comment:6 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Pushed branch:t/13441.

From what I've manually tested, it seems like Edge handles events properly. I've changed if ( CKEDITOR.env.ie ) to if ( clipboard.mainPasteEvent == 'beforepaste' ) so those statements better represent what is really going on. I am not sure if there is a need for a special flag, as mainPasteEvent is beforePaste only of IE<=11.

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

comment:7 Changed 3 years ago by Piotr Jasiun

Status: reviewreview_failed

Solution looks and works good, but tests are missing. Please add both unit and manual tests.

comment:8 Changed 3 years ago by Szymon Cofalik

Status: review_failedassigned

comment:9 Changed 3 years ago by Szymon Cofalik

I pushed manual test to branch:t/13441.

I struggled to get unit test working but got an error that browser is preventing from "software" paste. Do we really need a unit test for this - keeping in mind that this is a fix for single browser + testing this in unit test does not reflect real scenarios in 100% ?

comment:10 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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

Do we really need a unit test for this - keeping in mind that this is a fix for single browser + testing this in unit test does not reflect real scenarios in 100% ?

I don't think we need such a test. It won't be accurate anyway and so basic bugs in clipboard are very well visible, so we will catch them just like now.

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

Status: reviewreview_failed

The solution looks right. There are few issues left though:

  • mainPasteEvent
    • CKEDITOR.env.ie && !CKEDITOR.env.edge looks like a more semantic decision.
    • Please update documentation to include information about Edge.
  • Various existing comments might need attention since we use clipboard.mainPasteEvent == ... instead of CKEDITOR.env.ie. I wonder if talking about Edge in the context of IE is the right thing to do. In env.js we use ie: !!( edge || trident ) but the ticket shows that Edge acts unlike other MS browsers. So the term "non–IE" used for Edge is both valid and invalid. We should discuss the matter to avoid ambiguous comments.
  • /tests/tickets/13441/1.*
    • The scenario should check whether undo is actually working like "press CTRL+z and expect sth".
    • The TC should be as simple as possible:
      • It looks like a single editor is enough.
      • "Unexpected result" is obsolete if "Expected result" clearly defines what supposed to happen.
      • Less startup data in the editor is better. A single sentence or synthetic foobars are enough.
    • If you really want to go with multiple editors, please separate them with meaningful headers corresponding with the scenario ("First", "Second", etc.) or at least use some CSS outlines (or both). They look like a single block of text.
    • Bold "Expected result" words. It's easier to read and focus on essentials that way.

comment:13 Changed 3 years ago by Szymon Cofalik

Status: review_failedassigned

comment:14 Changed 3 years ago by Szymon Cofalik

I pushed a commit to branch:t/13441

I updated clipboard/plugin.js: tweaked comments and changed CKEDITOR.env.ie < 12 to !CKEDITOR.env.edge. I also deleted one if statement that was same as the statement above (see line 612).

I also tweaked tests but I want to stick to three editors:

  • this setup strictly represents description from the ticket
  • second part of the test is not affected by first part of the test

comment:15 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Good job!

Merged git:b88638a into master.

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

I’ve just tested Edge’s integration with clipboard and it’s very good. Unfortunately due to broken clipboard API implementation we still need to use pastebin and copybin, but it seems that they work bidirectional (the Copy/Cut buttons work) and on paste there's the paste dialog displayed like on Chrome. This is very different behaviour than in older IEs, so I'm happy that the existing code handles Edge so well.

One bug I noticed is that using copybin to copy widgets scrolls the viewport, but we'll handle this in a new ticket.

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

Reported #13577 for the problems with viewport scrolling.

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

Summary: [Edge] Issues with undo[Edge] Broken clipboard integration
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy