[Edge] Broken clipboard integration

[Olek Nowodziński]

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.

[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.

[Piotr Jasiun]

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

[Szymon Cofalik]

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.

[Piotr Jasiun]

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

[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% ?

[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.

[Olek Nowodziński]

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.
  • ​https://github.com/cksource/ckeditor-dev/blob/9b9c099503835b6243a8db57ac52d051bc32bb65/plugins/clipboard/plugin.js#L964
  • ​https://github.com/cksource/ckeditor-dev/blob/9b9c099503835b6243a8db57ac52d051bc32bb65/plugins/clipboard/plugin.js#L1008
  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.

[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

[Olek Nowodziński]

Good job!

Merged ​git:b88638a into master.

[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.

[Piotrek Koszuliński]

Reported #13577 for the problems with viewport scrolling.