Fixed focusing a widget after blurring editor for edge #1466

jacekbogdanski · 2018-01-17T10:48:35Z

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?

Added condition to focus editor for CKEDITOR.plugins.copyformatting.state#applyFormatting event only when client browser is IE .
Closes #1458

f1ames

In the PR description you marked it contains unit test, but I don't see any. It would be good to add some.

f1ames · 2018-01-18T15:39:10Z

plugins/copyformatting/plugin.js

@@ -414,7 +414,7 @@
 			},

 			applyFormatting: {
-				editorFocus: false,
+				editorFocus: CKEDITOR.env.ie && !CKEDITOR.env.edge ? false : true,


With such condition (CKEDITOR.env.ie && !CKEDITOR.env.edge) you will get false for all IEs and true for Edge and all other browsers different than IEs.
From what I understand it should be true only for Edge and false for others, so in this case it should be CKEDITOR.env.ie && CKEDITOR.env.edge ? true : false.

No, it should be false for an IE. It is old fix where IE with editor focus will scroll up if an image is at the bottom of a scrollable container bdbd071.

Ok, I see, after F2F talk, the reason was to rollback the editorFocus: false change as initially it should be applied only for IEs, but was applied for all browsers (see https://dev.ckeditor.com/ticket/16845). So now editorFocus: false should be only used for IEs.
That was also a reason for modifying manual test (which is answer for #1466 (comment)).

Sorry for misleading information about unit tests. As we talked F2F there is no easy way to test this issue directly in the code, so I will skip this part and use manual tests.

f1ames · 2018-01-18T15:40:55Z

tests/plugins/copyformatting/manual/edgefocus.md

@@ -0,0 +1,18 @@
+@bender-tags: bug, 4.8.1, copyformatting, 1458


Target version should be 4.9.0.

f1ames · 2018-01-18T15:41:02Z

tests/plugins/copyformatting/manual/edgefocus.html

+	}
+
+	CKEDITOR.replace( 'editor1', {
+		width: '30em',


Not sure if such detailed width and height is needed, was there any particular reason for this?

f1ames · 2018-01-18T15:44:13Z

tests/plugins/copyformatting/manual/focus.md

@@ -1,4 +1,4 @@
-@bender-tags: bug, 4.7.0, trac16845, copyformatting
+@bender-tags: bug, 4.7.0, trac16845, copyformatting, 1458


If this test is important for this fix it should also contain 4.9.0 version tag. If not, you shouldn't have added 1458 ticket number.

f1ames · 2018-01-19T10:03:41Z

tests/plugins/copyformatting/manual/focus.html

+</textarea>
+
+<script>
+	CKEDITOR.replace( 'editor1' );


Again, from what I understand this was the Edge only fix, what was the reason of removing

if ( !CKEDITOR.env.ie ) { bender.ignore(); }

ignore from this test? Does the changes somehow affect other browsers?

The problem was with IE, but older fix corrupted Edge, so I thought it's good idea to test all other browsers.

f1ames

Looks good 👍

Just some details in tests needs fixing.

f1ames · 2018-02-21T10:56:09Z

tests/plugins/copyformatting/manual/edgefocus.md

@@ -0,0 +1,18 @@
+@bender-tags: bug, 4.9.0, copyformatting, 1458


Please update tag to 4.9.1.

f1ames · 2018-02-21T10:56:35Z

tests/plugins/copyformatting/manual/edgefocus.md

@@ -0,0 +1,18 @@
+@bender-tags: bug, 4.9.0, copyformatting, 1458
+@bender-ui: collapsed
+@bender-ckeditor-plugins: copyformatting,toolbar,wysiwygarea,floatingspace,elementspath,image2,widget,justify,div


You could easily run this test with only copyformatting,toolbar,wysiwygarea,image2. No need for additional plugins.

f1ames · 2018-02-21T10:56:56Z

tests/plugins/copyformatting/manual/edgefocus.md

+## Test Scenario
+
+1. Make sure the editor is not focused.
+2. Select the image on the left by clicking it.


by clicking on it.

f1ames · 2018-02-21T10:57:27Z

tests/plugins/copyformatting/manual/edgefocus.md

+
+1. Make sure the editor is not focused.
+2. Select the image on the left by clicking it.
+3. Remove selection from the image by clicking outside of the editor.


outside of the editor.

f1ames · 2018-02-21T10:57:59Z

tests/plugins/copyformatting/manual/focus.html

@@ -0,0 +1,41 @@
+


Is this blank line really necessary?

f1ames · 2018-02-21T11:01:15Z

tests/plugins/copyformatting/manual/focus.md

@@ -1,4 +1,4 @@
-@bender-tags: bug, 4.7.0, trac16845, copyformatting
+@bender-tags: bug, 4.7.0, 4.9.0, trac16845, copyformatting, 1458
 @bender-ui: collapsed
 @bender-ckeditor-plugins: copyformatting,toolbar,wysiwygarea,floatingspace,elementspath,image2,widget,justify,div


Same here, you only need copyformatting,toolbar,wysiwygarea,image2,justify,div (I know this tests was not added by this PR, but it is a good opportunity to fix it:D).

f1ames

LGTM 👍

f1ames · 2018-02-21T16:59:00Z

Should be merged to master after 4.9.0 release.

mlewand · 2018-03-06T10:32:08Z

Retargetting into a next branch (4.10.0 release) as current major is frozen for the 4.9.0 release.

f1ames suggested changes Jan 19, 2018

View reviewed changes

jacekbogdanski requested a review from f1ames January 26, 2018 08:41

f1ames force-pushed the t/1458 branch from 47a1444 to 4ee3f7d Compare February 20, 2018 14:09

f1ames changed the base branch from master to major February 20, 2018 14:32

f1ames suggested changes Feb 21, 2018

View reviewed changes

jacekbogdanski requested a review from f1ames February 21, 2018 11:38

f1ames approved these changes Feb 21, 2018

View reviewed changes

jacekbogdanski and others added 6 commits March 6, 2018 11:29

Fixed copyformatting focus.

3a9f058

Changeset entry.

c31f605

Changed changeset entry target version.

26fbc02

Small fixes.

5c7c489

Changelog entry fix. [skip ci]

8d712d9

Minor test fixes.

435b2a3

mlewand force-pushed the t/1458 branch from bca333d to 435b2a3 Compare March 6, 2018 10:31

mlewand changed the base branch from major to next March 6, 2018 10:32

Updated issue tags.

cad7ae8

mlewand merged commit 13bcc76 into next Mar 6, 2018

mlewand deleted the t/1458 branch March 6, 2018 10:36

mlewand mentioned this pull request Mar 6, 2018

After blurring editor it takes 2 clicks to focus a widget #1458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed focusing a widget after blurring editor for edge #1466

Fixed focusing a widget after blurring editor for edge #1466

jacekbogdanski commented Jan 17, 2018 •

edited

Loading

f1ames left a comment

f1ames Jan 18, 2018

jacekbogdanski Jan 19, 2018

f1ames Jan 19, 2018

jacekbogdanski Jan 25, 2018

f1ames Jan 18, 2018

f1ames Jan 18, 2018

f1ames Jan 18, 2018

f1ames Jan 19, 2018

jacekbogdanski Jan 19, 2018

f1ames left a comment

f1ames Feb 21, 2018

f1ames Feb 21, 2018 •

edited

Loading

f1ames Feb 21, 2018

f1ames Feb 21, 2018

f1ames Feb 21, 2018

f1ames Feb 21, 2018

f1ames left a comment

f1ames commented Feb 21, 2018

mlewand commented Mar 6, 2018

		@@ -0,0 +1,18 @@
		@bender-tags: bug, 4.8.1, copyformatting, 1458

		@@ -1,4 +1,4 @@
		@bender-tags: bug, 4.7.0, trac16845, copyformatting
		@bender-tags: bug, 4.7.0, trac16845, copyformatting, 1458

		@@ -0,0 +1,18 @@
		@bender-tags: bug, 4.9.0, copyformatting, 1458

Fixed focusing a widget after blurring editor for edge #1466

Fixed focusing a widget after blurring editor for edge #1466

Conversation

jacekbogdanski commented Jan 17, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

f1ames 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

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames Feb 21, 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

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

f1ames commented Feb 21, 2018

mlewand commented Mar 6, 2018

jacekbogdanski commented Jan 17, 2018 •

edited

Loading

f1ames Feb 21, 2018 •

edited

Loading