[#704] Fix broken widget after using Ctrl+Z in Edge #734

msamsel · 2017-08-07T12:13:54Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

Modify condition which remove unnecessary div in Edge.
Modify condition to work only in Edge up to 14.xxx. (Edge 15 doesn't add div).
Modify tests to respect new condition related to Edge version.

Close #704

f1ames · 2017-08-22T09:56:51Z

plugins/wysiwygarea/plugin.js

@@ -164,7 +164,7 @@
 		// Prevent IE/Edge from leaving a new paragraph/div after deleting all contents in body. (http://dev.ckeditor.com/ticket/6966, http://dev.ckeditor.com/ticket/13142)


Can you adjust the description to make it clear that this case does not happen in Edge > 15?
You could also reformat the comment so tickets reference is consistent with our convention (ticket reference before . not after .).

f1ames · 2017-08-22T10:18:57Z

plugins/wysiwygarea/plugin.js

@@ -297,7 +297,7 @@
 				var elements = doc.getElementsByTag( tagName );
 				if ( lockRetain ) {
 					if ( elements.count() == 1 && !elements.getItem( 0 ).getCustomData( 'retain' ) &&
-						!elements.getItem( 0 ).hasAttribute( 'data-cke-temp' ) ) {
+						CKEDITOR.tools.isEmpty( elements.getItem( 0 ).getAttributes() ) ) {


What is the reason of this change? Previously elements without data-cke-temp attribute (so not temporary elements) were removed inside this if, now it removes only attributeless elements. Is this change necessary?

Entire function removeSuperfluousElement was design to remove additional div which appear in Edge, when all text is removed from editor. This div never had any attribute. Unfortunately author hadn't predict that not only those extra divs might appear, but also widgets might be construct based on div. And in case of undo command entire widget might be redraw so we lost retain customData attached to div.
This change detects more carefully only those extra divs from Edge, so it becomes more specific. Now divs connected to widgets won't be removed.

Makes sense 👍

f1ames · 2017-08-22T10:22:11Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.html

+	var editor = CKEDITOR.replace( 'editor1', {
+		height: '600px'
+	} );
+	setInterval( function() {


You could listen to change event here instead of using timeout, so it should be more reliable.

Unfortunately change event is not working as it should. :( It's execute before keyup listener removes div from wysiwygarea. I've used keyup listener instead.

f1ames · 2017-08-22T10:23:05Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.md

+@bender-ckeditor-plugins: image2, wysiwygarea, undo, sourcearea
+
+----
+1. Put caret in the text on the right and type some character.


type some text.

sounds a little better IMHO.

f1ames · 2017-08-22T10:27:26Z

tests/plugins/widget/integration/wysiwygarea/superflousdoesntremovedivwrapper.js

+				} ) );
+
+				var snap = editor.getSnapshot();
+				editor.loadSnapshot( snap );


This two lines just reload the snapshot, it it needed here?

Yes.
keydown and keyup don't trigger undo command, which triggers re-creating widget (there even is no history to revert it).
So this simulation works as follows:

Simulate keydown to register listeners on div by superflous function.

Reload snapshot, to simulate Ctrl+Z, when previous snapshot is restored.

Reloading snapshot automatically triggers re-creating widget. Here will be destroyed wrapping div for widget, and retain attribute will be lost (as it happens in Edge).

Simulate keyup to execute proper listener from superflous function.

Re-loading snapshot might be replaced with some kind of triggering undo event, but it would require much more work. First to make some history, and then revert it. So I extract only important part from entire undo process, which is reloading snapshot, what cause the issue in case of using shortcut. This allows on proper simulation behaviour on Edge and proper fail of test in case of Edge environment without bug fix.

Makes sense 👍

f1ames · 2017-08-22T10:31:51Z

tests/plugins/wysiwygarea/superfluouselement.js

@@ -44,8 +44,8 @@ bender.test( {
 		wait();
 	},

-	'Test removing superfluous <div> inserted by Edge': function() {
-		if ( !CKEDITOR.env.edge ) {
+	'Test removing superfluous <div> inserted by Edge < v15': function() {


I know it is not your code, but could you change Test removing... to test removing...? It should be lowercased. Please update all test names in this file if necessary.

Also inserted by Edge < v15, I think < 15 is not needed, as it is also in ignore condition right below test name.

f1ames · 2017-08-22T10:33:11Z

tests/plugins/wysiwygarea/superfluouselement.js

@@ -227,5 +227,31 @@ bender.test( {
 			} );
 		} );
 		wait();
+	},
+
+	'Test not-removing <div> with any attributes in Edge < v15': function() {


It should be not removing instead of not-removing.

f1ames · 2017-09-12T12:46:46Z

plugins/wysiwygarea/plugin.js

-		} else if ( CKEDITOR.env.edge && editor.enterMode != CKEDITOR.ENTER_DIV ) {
+		}
+		// From version Edge 15 additional `div` are not added to the editor.
+		else if ( CKEDITOR.env.edge && editor.enterMode != CKEDITOR.ENTER_DIV && CKEDITOR.env.version < 15 ) {


Rearranging this if as:

CKEDITOR.env.edge && CKEDITOR.env.version < 15 && editor.enterMode != CKEDITOR.ENTER_DIV

will make it more readable IMHO.

f1ames · 2017-09-12T12:47:33Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.html

+	var editor = CKEDITOR.replace( 'editor1', {
+		height: '600px'
+	} );
+	var output = document.getElementById( 'output' )


Missing ; on the end of this line.

f1ames · 2017-09-12T12:48:10Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.html

+		editor.editable().$.addEventListener( 'keyup', function() {
+			output.innerText = editor.editable().getFirst().getName();
+		} );
+	});


Should be } ); instead of });.

f1ames · 2017-09-12T12:55:38Z

tests/plugins/widget/integration/wysiwygarea/superflousdoesntremovedivwrapper.js

+				} ) );
+
+				var snap = editor.getSnapshot();
+				editor.loadSnapshot( snap );


Makes sense 👍

f1ames · 2017-09-12T13:03:21Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.md

+1. Put caret in the text on the right and type some text.
+1. Use `Ctrl + Z` or `Cmd + Z` shortcut to make undo few times.
+
+**Expected:** Text returns to previous form. Below editor will be written `div` on green background.


Maybe:

Text returns to previous form. Below the editor, you should see div on green background.

f1ames · 2017-09-12T13:04:10Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.md

+
+**Expected:** Text returns to previous form. Below editor will be written `div` on green background.
+
+**Unexpected:** Text moves below the image. Below editor will be written `figure` instead of `div`.


And:

Text moves below the image. Below editor, it will be written figure instead of div on the green background.

f1ames · 2017-09-12T13:08:10Z

tests/plugins/widget/integration/wysiwygarea/superflousdoesntremovedivwrapper.js

+
+	bender.test( {
+		// #704
+		'test keeping widget wrapper in editor when superflous elemnts are checked': function() {


Typo:

test keeping widget wrapper in editor when superfluous elements are checked

f1ames · 2017-09-12T13:18:14Z

tests/plugins/widget/integration/wysiwygarea/manual/preventofremovingdiv.html

@@ -0,0 +1,22 @@
+<textarea id="editor1" name="editor1">


This test fails on IE8 due to addEventListener, if it should be run only for Edge add a proper bender.ignore or fix attaching listener if it should be run also for IE8.

I added both things. I know it wasn't necessary, but this not harm anything. ;)

…4.xxx Edge version.

- ignore manual tests on non-edge browser and omit usage of addEventListener - typo fixes - syntax fixes

msamsel · 2017-09-13T11:00:27Z

I've made fixes and rebase branch to major. Also switch target branch on github to major.

f1ames

LGTM 👍

f1ames suggested changes Aug 22, 2017

View reviewed changes

msamsel force-pushed the t/704 branch from e37efa0 to 4c40d4f Compare August 25, 2017 09:44

f1ames self-requested a review September 12, 2017 12:33

f1ames suggested changes Sep 12, 2017

View reviewed changes

Mateusz Samsel and others added 8 commits September 13, 2017 12:53

Add unit and manual tests.

d4fbb19

Fix reomoving div from wrapper on Edge.

66e91c2

Update condition for Edge. From v15 divs are not added.

17782dc

Modify tests conditions, because superflous element works now up to 1…

4e9b4c9

…4.xxx Edge version.

Review fixes

cb9f537

Rewording.

5dd38bb

Manual test target version tag updated.

a3b43f1

Review fixes:

5a0456e

- ignore manual tests on non-edge browser and omit usage of addEventListener - typo fixes - syntax fixes

msamsel force-pushed the t/704 branch from 55d68ec to 5a0456e Compare September 13, 2017 10:53

msamsel changed the base branch from master to major September 13, 2017 10:54

msamsel changed the title ~~Fix broken widget after using Ctrl+Z in Edge~~ [#704] Fix broken widget after using Ctrl+Z in Edge Sep 13, 2017

f1ames self-requested a review September 13, 2017 12:03

If condition rephrasing.

e2c30c0

f1ames approved these changes Sep 13, 2017

View reviewed changes

f1ames merged commit 37f6a4b into major Sep 13, 2017

f1ames deleted the t/704 branch September 13, 2017 12:39

f1ames mentioned this pull request Sep 13, 2017

Broken content in article editor after using shortcut CTRL+Z #704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#704] Fix broken widget after using Ctrl+Z in Edge #734

[#704] Fix broken widget after using Ctrl+Z in Edge #734

msamsel commented Aug 7, 2017

f1ames Aug 22, 2017

f1ames Aug 22, 2017

msamsel Aug 25, 2017

f1ames Sep 12, 2017

f1ames Aug 22, 2017

msamsel Aug 25, 2017

f1ames Aug 22, 2017

f1ames Aug 22, 2017

msamsel Aug 25, 2017

f1ames Sep 12, 2017

f1ames Aug 22, 2017

f1ames Aug 22, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

f1ames Sep 12, 2017

msamsel Sep 13, 2017

msamsel commented Sep 13, 2017

f1ames left a comment

		@@ -164,7 +164,7 @@
		// Prevent IE/Edge from leaving a new paragraph/div after deleting all contents in body. (http://dev.ckeditor.com/ticket/6966, http://dev.ckeditor.com/ticket/13142)


		Expected: Text returns to previous form. Below editor will be written `div` on green background.

		Unexpected: Text moves below the image. Below editor will be written `figure` instead of `div`.

[#704] Fix broken widget after using Ctrl+Z in Edge #734

[#704] Fix broken widget after using Ctrl+Z in Edge #734

Conversation

msamsel commented Aug 7, 2017

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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

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

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

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

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

Choose a reason for hiding this comment

msamsel commented Sep 13, 2017

f1ames left a comment

Choose a reason for hiding this comment