[#17028] Tabbing through inline editor causes JavaScript error. #451

msamsel · 2017-06-01T14:18:23Z

What is the purpose of this pull request?

Bug fix
It's PR related to #424

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

Add checks in 2 places that values are defined.

close #424

Comandeer

The fix is not working correctly. After tabbing into editor, there is no caret inside and it's impossible to type anything. This is probably caused by the fact that in Chrome selection is removed from window when blurring inline editor → http://dev.ckeditor.com/ticket/13446

The real fix for this issue would be ensure that elements path is always set when focusing the editor. In other words: ensure that there is a caret inside newly focused editor.

Comandeer · 2017-06-02T12:48:47Z

tests/plugins/indentlist/manual/taberrorininlineeditor.html

+<head>
+	<script>
+		if ( bender.tools.env.mobile ) {
+			bender.ignore;


bender.ignore is a function.

msamsel · 2017-06-07T11:07:44Z

It seems that problem start to occur after introducing change in 3f07b6d.
Problem describe there seems to be fixed in Chrome v57. I revert this change and make slightly modification in Unit test. As I see chrome completely remove selection so reading type throw an error. Now test check if native selection exists.

Comandeer · 2017-06-09T09:13:35Z

tests/plugins/indentlist/manual/taberrorininlineeditor.md

@@ -0,0 +1,12 @@
+@bender-tags: 4.7.1, tc, gh424


Please update tags to the new convention.

Comandeer · 2017-06-09T09:14:58Z

tests/plugins/tab/manual/taberrorininlineeditor.md

@@ -0,0 +1,12 @@
+@bender-tags: 4.7.1, tc, gh424


Please update tags to the new convention.

Comandeer · 2017-06-09T09:16:25Z

plugins/indentlist/plugin.js

 					if ( editor.mode != 'wysiwyg' )
 						return;

 					if ( evt.data.keyCode == this.indentKey ) {
-						var list = this.getContext( editor.elementPath() );
+						// Prevent of getting context of empty path. (#17028)


Please update ticket number.

Comandeer · 2017-06-09T09:16:43Z

core/dom/elementpath.js

@@ -57,6 +57,11 @@
 		// Backward compact.
 		root = root || startNode.getDocument().getBody();

+		// Assign root value if startNode is null (#17028).


Please update ticket number.

Comandeer · 2017-06-09T09:17:51Z

tests/core/focusmanager/editor.js

@@ -109,7 +109,12 @@ bender.test( {
 			resume( function() {
 				editor.on( 'blur', function() {
 					resume( function() {
-						assert.areSame( 'None', editor.getSelection().getNative().type );


Are these tests still needed? It seems that now they're checking the native Chrome's implementation.

There is also manual test for it (core/focusmanager/manual/typinginunfocusedinlineeditor), which became unneeded.

Comandeer

LGTM, but there are some cleanup tasks left. Let's do them and finish this PR!

Comandeer · 2017-06-12T08:27:38Z

core/dom/elementpath.js

@@ -57,6 +57,11 @@
 		// Backward compact.
 		root = root || startNode.getDocument().getBody();

+		// Assign root value if startNode is null (https://dev.ckeditor.com/ticket/17028).


Update tickets numbers to the new convention.

@Comandeer I'm a little bit confused, according to #470, there should be permanent link to issue in track. And it is there, or do I missed something?
Or by new convention you understand reference to github issue?

Yes, I mean adding reference to GH issue alongside the reference to trac.

Comandeer · 2017-06-12T08:27:46Z

plugins/indentlist/plugin.js

 					if ( editor.mode != 'wysiwyg' )
 						return;

 					if ( evt.data.keyCode == this.indentKey ) {
-						var list = this.getContext( editor.elementPath() );
+						// Prevent of getting context of empty path (https://dev.ckeditor.com/ticket/17028).


Update tickets numbers to the new convention.

Comandeer · 2017-06-12T08:28:59Z

tests/plugins/indentlist/manual/taberrorininlineeditor.md

@@ -0,0 +1,12 @@
+@bender-tags: 4.7.1, tc, 424


Update tags to the new bug/feature convention.

Additionally this test does not make much sense in browsers other than Chrome (as this thing was always working there) – let's run it only in Chrome.

Comandeer · 2017-06-12T08:29:22Z

tests/plugins/indentlist/taberror.js

+	};
+
+	bender.test({
+		'test tab in inline editor': function() {


Maybe add ticket reference here, just for clarity?

Comandeer · 2017-06-12T08:29:35Z

tests/plugins/tab/errorinlineeditor.js

+	};
+
+	bender.test({
+		'test tab in inline editor': function() {


Maybe add ticket reference here, just for clarity?

Comandeer · 2017-06-12T08:29:47Z

tests/plugins/tab/manual/taberrorininlineeditor.md

@@ -0,0 +1,12 @@
+@bender-tags: 4.7.1, tc, 424


Update tags to the new bug/feature convention.

Additionally this test does not make much sense in browsers other than Chrome (as this thing was always working there) – let's run it only in Chrome.

Comandeer · 2017-06-12T13:03:48Z

tests/plugins/indentlist/manual/taberrorininlineeditor.html

@@ -0,0 +1,40 @@
+<head>
+	<script>
+		if ( !CKEDITOR.env.chrome ) {


Please move it to the script in body as it apparently does not work in head.

Interesting.
when I fired the test from link:
http://tests.ckeditor.dev:1030/#tests/path:/tests/plugins/indentlist/manual with a play button it seemd to be ignored, but there is no notification when test is opened directly.

I moved condition to body to work properly.

Comandeer · 2017-06-12T13:03:58Z

tests/plugins/tab/manual/taberrorininlineeditor.html

@@ -0,0 +1,40 @@
+<head>
+	<script>
+		if ( !CKEDITOR.env.chrome ) {


Please move it to the script in body as it apparently does not work in head.

…on Chrome.

…problems.

Comandeer self-requested a review June 2, 2017 11:35

Comandeer requested changes Jun 2, 2017

View reviewed changes

msamsel changed the title ~~[#170028] Tabbing through inline editor causes JavaScript error.~~ [#17028] Tabbing through inline editor causes JavaScript error. Jun 6, 2017

Comandeer self-requested a review June 9, 2017 08:32

Comandeer requested changes Jun 9, 2017

View reviewed changes

Comandeer self-requested a review June 12, 2017 07:06

Comandeer requested changes Jun 12, 2017

View reviewed changes

Comandeer self-requested a review June 13, 2017 09:28

Comandeer approved these changes Jun 13, 2017

View reviewed changes

Mateusz Samsel and others added 11 commits June 13, 2017 11:48

Creating manual and unit tests.

dab07d5

Fixing throwed error in console.

baf4cec

Creating test for next error thrown after first one.

70130b3

Fixed throwned error.

7eb8fda

Revert fix from #13446 [3f07b6d]. Chrome fix this bug in v57.

8cc72d1

Fixin my manual tests. Update core focusmanager test to work correct …

bf79536

…on Chrome.

Remove unnecessary tests, replace issue references. Fix some linting …

89d3e65

…problems.

Update issue references.

36e3990

Move bender.ignore condition to body.

969901f

Remove redundant tc tag from manual tests.

de378b3

Update changelog.

c8609a4

Comandeer merged commit 2354a28 into ckeditor:master Jun 13, 2017

Comandeer deleted the t/17028 branch June 13, 2017 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#17028] Tabbing through inline editor causes JavaScript error. #451

[#17028] Tabbing through inline editor causes JavaScript error. #451

msamsel commented Jun 1, 2017 •

edited

Loading

Comandeer left a comment

Comandeer Jun 2, 2017

msamsel Jun 7, 2017

msamsel commented Jun 7, 2017

Comandeer Jun 9, 2017

Comandeer Jun 9, 2017

Comandeer Jun 9, 2017

Comandeer Jun 9, 2017

Comandeer Jun 9, 2017

Comandeer left a comment

Comandeer Jun 12, 2017

msamsel Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

Comandeer Jun 12, 2017

msamsel Jun 12, 2017

Comandeer Jun 12, 2017

[#17028] Tabbing through inline editor causes JavaScript error. #451

[#17028] Tabbing through inline editor causes JavaScript error. #451

Conversation

msamsel commented Jun 1, 2017 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Jun 7, 2017

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

Comandeer 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

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 Jun 1, 2017 •

edited

Loading