Fix mobile paste #1229

Comandeer · 2017-11-25T18:23:52Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

I've added back our beloved paste dialog. It's shown only if user touches the button (via recognising touchend event).

There are missing language files entries that should be added before merging. For now only en locale is covered.

Closes #595.

mlewand

Just minor comments, as I had a quick look.

That was a really quick turnaround, nice!

We're missing integration with context menu options.

mlewand · 2017-11-28T23:16:25Z

plugins/clipboard/plugin.js

+					}
+
+					CKEDITOR.tools.array.forEach( editor._.pasteButtons, function( name ) {
+						var pasteButton = editor.ui.get( name ),


This function might return undefined, for instance you could have clipboard plugin, but no "Paste" (or related) button in the toolbar.

In such case editor._.pasteButtons should be empty or undefined.

Please check /tests/plugins/clipboard/manual/missingbutton manual test in t/595-nobutton branch.

f1ames · 2017-12-13T13:43:48Z

I extracted context menu integration to #1347.

f1ames · 2017-12-13T16:17:54Z

Before merging/closing this ticket #1347 should be covered and merged here.

mlewand · 2018-02-07T09:43:52Z

Rebased onto latest 4.9.0 branch. Changed the base of this PR accordingly.

mlewand

I have updated tags in tests, API docs etc. Generally good job, it works reliably.

The good

I see that at the same time you took the chance to remove old Opera-related timeouts. Good idea, any setTimeout removed is a win for us, and reduced complexity. 👍

Missed things

Change paste dialog message, it says about keyboard. Well it should either about the touch, or just about the native ways to do that.

Notices/discussion

Talking about cleanup, this evt.cancel() call doesn't have much sense as the only place firing the pasteDialogCommit event is paste dialog and it doesn't care whether event was canceled or not. WDYT?
Doesn't work on Edge/IE11 but it's fine as we do not officially support them. So that's not an issue.

mlewand · 2018-02-07T09:46:03Z

plugins/clipboard/dialogs/paste.js

+			this.customTitle = null;
+
+			// Reset commited indicator.
+			this._.commited = false;


commited -> committed

The fix has to be applied across entire PR.

mlewand · 2018-02-07T09:48:58Z

plugins/clipboard/plugin.js

@@ -427,12 +440,19 @@
 		 * an upcoming major release.
 		 */
 		editor.getClipboardData = function( callbackOrOptions, callback ) {


Function signature once again changed, now we're using options once again…

That's a messy situation and we should adjust the API docs correctly. I mean that the options argument is again supported, etc.

I've just dropped handling of options as it was used only to set custom title of paste dialog, which wasn't even working.

mlewand · 2018-02-07T09:51:20Z

plugins/clipboard/plugin.js

+						if ( pasteButton ) {
+							var buttonElement = CKEDITOR.document.getById( pasteButton._.id );
+
+							buttonElement.on( 'touchend', function() {


I was thinking about optimizing this listener, and putting it once on the entire toolbar and just then check what button was pressed. However after playing a bit with it I start to think that it's too much of effort for the benefit. So I guess it's fine the way it is.

mlewand · 2018-02-07T09:51:50Z

plugins/clipboard/plugin.js

+						}
+					} );
+
+					delete editor._.pasteButtons;


Let's not remove this member from the editor, so it could be reused by other plugins if needed this workarounds for mobile.

mlewand · 2018-02-07T09:52:29Z

plugins/clipboard/plugin.js

+ * Internal event to open the Paste dialog window.
+ *
+ * @private
+ * @event pasteDialog


We should note that pasteDialog event was not available in 4.7.0 - 4.8.0 versions.

mlewand · 2018-02-07T09:53:59Z

plugins/clipboard/plugin.js

+		 * @param {String} name Name of the button.
+		 * @param {Object} definition Definition of the button.
+		 */
+		addPasteButton: function( editor, name, definition ) {


addPasteButton should return created CKEDITOR.ui.button instance, so that we could use it for instance with Balloon Toolbar.

Due to the super convoluted way of accessing button instance from our UI, we decided to not introduce this change. Info gathered from editor._.pasteButtons should be enough to create buttons for Balloon Toolbar.

Comandeer · 2018-02-07T12:13:12Z

Talking about cleanup, this evt.cancel() call doesn't have much sense as the only place firing the pasteDialogCommit event is paste dialog and it doesn't care whether event was canceled or not. WDYT?

It was a part of whole dialog logic from the beginning and revert of dialog deletion brought it back. Honestly I also don't see much sense in it, but I was just afraid to touch it 😄 However it seems that removing it does not change anything.

Comandeer · 2018-02-07T12:26:28Z

Actually I located the issue: without this evt.cancel, paste event is called twice. So it's better to leave it untouched.

Comandeer · 2018-02-07T13:02:59Z

I've added unit test for evt.cancel, fixed existing tests and add changelog entry.

This reverts commit 4f79efc.

This reverts commit 95fdd37.

…urity dialog.

mlewand · 2018-02-07T16:10:24Z

Thanks for the research. I have rebased onto latest major. There was an issue that on IE unit tests would trigger a security dialog (request for allowing the paste). I have stubbed the document.execCommand for that particular TC and it works OK.

I'll wait with a final R+ for #1347.

Paste dialog context menu integration for mobile

mlewand

LGTM

Unfortunately, this will reintroduce all the clipboard plugin that we wanted to remove in 4.7.0 - but we can't break pasting on mobiles.

mlewand reviewed Nov 28, 2017

View reviewed changes

mlewand mentioned this pull request Nov 29, 2017

Fix pasting (paste/paste plain text/paste from word) experience for mobile browsers #595

Closed

f1ames mentioned this pull request Dec 13, 2017

Fix mobile paste - context menu integration #1347

Closed

f1ames force-pushed the t/595 branch from 90e1d81 to 1cf6b43 Compare December 13, 2017 13:48

f1ames changed the base branch from major to master December 13, 2017 13:48

f1ames force-pushed the t/595 branch from 54eaa3e to 0f00c6a Compare December 13, 2017 16:16

mlewand self-requested a review January 29, 2018 11:02

mlewand force-pushed the t/595 branch from 0f00c6a to 3f6221d Compare February 7, 2018 09:42

mlewand changed the base branch from master to major February 7, 2018 09:42

mlewand suggested changes Feb 7, 2018

View reviewed changes

Comandeer and others added 14 commits February 7, 2018 17:08

Revert "Removed paste dialog."

c5c2388

This reverts commit 4f79efc.

Update language meta data and default language.

6326e94

Add touch detection on paste button and forcing dialog.

1eee96a

Removed securityMsg area from dialog.

1061585

Introduce CKEDITOR.plugins.addPasteButton.

4caa9fc

Hide notification when dialog is forced.

ca59b69

Revert "Removed dialog specific tests."

f0f26e5

This reverts commit 95fdd37.

Update unit tests.

77906c2

Move info about commited state into dialog itself.

a126117

Add and update unit tests.

0868a18

Add manual test.

90aed9f

Tests added manual test for pasting without toolbar a button.

bdb0244

Handle removed buttons.

466ecc5

Tests: removed paste buttons tests.

3442edd

mlewand and others added 14 commits February 7, 2018 17:08

Feature is retargetted to 4.9.0.

a7c2fa4

Rephrased API docs for addPasteButton in the clipboard plugin.

a7012e0

Tests: corrected pasting manual tests.

66687c8

Fix typo in committed.

57fda2c

Preserver editor's private member pasteButtons.

64739d2

Add note about non-availability of events in 4.7.0-4.8.0 versions.

05fc137

Drop handling of options in editor.getClipboardData.

64197d0

Make paste message gre… more generic.

c26cf47

Update tests names.

0003fa1

Fix test of event structure.

bfae678

Add unit test for paste events count.

cbdf823

Add changelog entry.

0da6ca7

Tests: disabled mobile fix check for IE/Edge. Minor corrections.

adc9503

Tests: stubbed document.execCommand so that IE does not display a sec…

0c23d22

…urity dialog.

mlewand force-pushed the t/595 branch from 802e8de to 0c23d22 Compare February 7, 2018 16:09

f1ames and others added 10 commits February 7, 2018 17:18

CKEDITOR.menu improvements.

2a66a29

Integrate context menu paste item with 'touchend' event.

56f5bf9

Tests: context menu integration manual test.

ba6e498

Tests: context menu integration unit tests.

cc901f9

Tests: context menu unit tests.

1e3d648

Tests: fix unit tests for IE/Edge.

53fa754

Tests: manual tests adjustments.

0029403

Tests and docs adjustments.

ee064a3

Use 'editor.contextMenu' to access active context menu instance.

43722dc

Merge pull request #1355 from ckeditor/t/595-1347

256553f

Paste dialog context menu integration for mobile

mlewand self-requested a review February 7, 2018 16:26

mlewand approved these changes Feb 7, 2018

View reviewed changes

mlewand merged commit be89c08 into major Feb 7, 2018

mlewand deleted the t/595 branch February 7, 2018 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mobile paste #1229

Fix mobile paste #1229

Comandeer commented Nov 25, 2017

mlewand left a comment

mlewand Nov 28, 2017

Comandeer Nov 28, 2017

mlewand Nov 28, 2017

f1ames commented Dec 13, 2017

f1ames commented Dec 13, 2017

mlewand commented Feb 7, 2018

mlewand left a comment

mlewand Feb 7, 2018

mlewand Feb 7, 2018

Comandeer Feb 7, 2018

mlewand Feb 7, 2018

mlewand Feb 7, 2018 •

edited

Loading

mlewand Feb 7, 2018

mlewand Feb 7, 2018

Comandeer Feb 7, 2018

Comandeer commented Feb 7, 2018

Comandeer commented Feb 7, 2018

Comandeer commented Feb 7, 2018

mlewand commented Feb 7, 2018

mlewand left a comment

Fix mobile paste #1229

Fix mobile paste #1229

Conversation

Comandeer commented Nov 25, 2017

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames commented Dec 13, 2017

f1ames commented Dec 13, 2017

mlewand commented Feb 7, 2018

mlewand left a comment

Choose a reason for hiding this comment

The good

Missed things

Notices/discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Feb 7, 2018

Comandeer commented Feb 7, 2018

Comandeer commented Feb 7, 2018

mlewand commented Feb 7, 2018

mlewand left a comment

Choose a reason for hiding this comment

mlewand Feb 7, 2018 •

edited

Loading