Fix autogrow width issue #4393

sculpt0r · 2020-11-23T07:37:33Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that 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

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4372](https://github.com/ckeditor/ckeditor4/issues/4372): Fixed: [Autogrow](https://ckeditor.com/cke4/addon/autogrow) plugin changes editor's width when used with absolute [`config.width`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-width) value.

What changes did you make?

Give an overview…
Add tests for autogrow plugin with multiple width cases.
Change creation of width value for resizing. Based on this logic (#4372 (comment))

Which issues does your PR resolve?

Closes #4372

jacekbogdanski

Please, add a manual test for the changes.

jacekbogdanski · 2020-11-23T11:55:12Z

tests/plugins/autogrow/autogrow.js

+	bender.createEditor = function( width, paragraphs ) {
+		bender.editorBot.create( {
+			name: 'EditorWidth' + width,
+			config: {
+				width: width
+			}
+		}, function( bot ) {
+			bot.setData( autogrowTools.getTestContent( paragraphs ), function() {
+				var editor = bot.editor;
+				var initialEditorSize = autogrowTools.getEditorSize( editor );
+
+				editor.once( 'afterCommandExec', function() {
+					resume( function name( params ) {
+						var editorSize = autogrowTools.getEditorSize( editor );
+
+						assert.isTrue( editorSize.height > initialEditorSize.height, 'editor height should increase' );
+						assert.areEqual( editorSize.width, initialEditorSize.width, 'editor width should not change' );
+					} );
+				} );
+
+				editor.execCommand( 'autogrow' );
+
+				wait();
+			} );
+		} );
+	};


It's better to just create a separate helper method than extending existing objects we don't own, so we won't accidentally overwrite current or future API.

jacekbogdanski · 2020-11-23T12:08:41Z

plugins/autogrow/plugin.js

+				var boxSizingType = editor.container.getComputedStyle( 'box-sizing' );
+				var isBorderBox = boxSizingType === 'border-box';
+
+				var width = editor.container.getSize( 'width', isBorderBox );


We are declaring variables using a single var statement when possible. Please, join variables.

jacekbogdanski · 2020-11-23T12:11:36Z

tests/plugins/autogrow/autogrow.js

@@ -8,7 +8,58 @@

 	bender.editor = {};

+	bender.createEditor = function( width, paragraphs ) {


This method does much more than creating an editor. It set up the whole testing environment and does assertions. Please, rename it into something more verbose, like testXXX.

jacekbogdanski · 2020-11-23T12:16:32Z

tests/plugins/autogrow/autogrow.js

+		'test autogrow for editor width 200%': function() {
+			bender.createEditor( '200%', 6 );
+		},
+		// (#4372)
+		'test autogrow for editor width 20em': function() {
+			bender.createEditor( '20em', 6 );
+		},
+		// (#4372)
+		'test autogrow for editor width 200px': function() {
+			bender.createEditor( '200px', 6 );
+		},
+		// (#4372)
+		'test autogrow for editor width 200': function() {
+			bender.createEditor( 200, 6 );
+		},
+		// (#4372)
+		'test autogrow for editor width 0': function() {
+			bender.createEditor( 0, 6 );
+		},
+		// (#4372)
+		'test autogrow for editor width auto': function() {
+			bender.createEditor( 'auto', 6 );
+		},


Since we are not changing the number of paragraphs anywhere, maybe let's just hardcode it inside the test method? We could also choose a bit higher value to make sure to not depend on some edge case, like 10.

jacekbogdanski · 2020-11-23T12:21:27Z

tests/plugins/autogrow/autogrow.js

+				var initialEditorSize = autogrowTools.getEditorSize( editor );
+
+				editor.once( 'afterCommandExec', function() {
+					resume( function name( params ) {


params is not used, so could be omitted. We could skipname name also since it's not telling anything and we are using anonymous functions in most cases.

sculpt0r · 2020-11-23T13:44:28Z

Please, add a manual test for the changes.

Added manual test for this bug.
Should I also edit PR to check 'Manual tests'?

jacekbogdanski

Just some minor changes required.

jacekbogdanski · 2020-11-23T13:47:12Z

tests/plugins/autogrow/manual/4372.md

@@ -0,0 +1,14 @@
+@bender-tags: 4.15.1, bug, 7372


We are using some more descriptive names for manual tests, please see other manuals as an example. Here, we could go with autogrowboxsize.

We are at 4.15.1. version already. The upcoming release version for bugs is 4.15.2.

7372 tag is invalid here. It should be the ticket number, so 4372 in our case.

I'm confused about one thing:

We are at 4.15.1. version already. The upcoming release version for bugs is 4.15.2.

So I should tag it with a future version of the editor?

7372 tag is invalid here. It should be the ticket number, so 4372 in our case.

🙊 I mean it, so shame on me for this! :(

Yes, it should reflect the target release, not the current one, as we are not providing backward patches.

jacekbogdanski · 2020-11-23T13:48:51Z

tests/plugins/autogrow/manual/4372.md

+2. Add or remove extra lines to force editor resize.
+3. Observe editor width.
+
+#### Expected result:


Headings should follow the correct level depending on the context. Here, we should rather use the 2nd level heading as the first level is added automatically by the bender under Test steps. Also, let's unify a bit headings naming with Expected/Unexpected titles to follow the same convention as in other tests.

Suggested change

#### Expected result:

## Expected

jacekbogdanski · 2020-11-23T13:53:56Z

tests/plugins/autogrow/manual/4372.html

+
+<script>
+	var editor = CKEDITOR.replace( 'editor', {
+		allowedContent: true,


We are avoiding disabling ACF by setting allowedContent to true to mimic the standard editor configuration when possible. Since we are just using paragraphs for these manual tests, this line is not necessary.

jacekbogdanski · 2020-11-23T13:57:31Z

plugins/autogrow/plugin.js

+				var boxSizingType = editor.container.getComputedStyle( 'box-sizing' ),
+				isBorderBox = boxSizingType === 'border-box',
+				width = editor.container.getSize( 'width', isBorderBox );


Suggested change

var boxSizingType = editor.container.getComputedStyle( 'box-sizing' ),

isBorderBox = boxSizingType === 'border-box',

width = editor.container.getSize( 'width', isBorderBox );

var boxSizingType = editor.container.getComputedStyle( 'box-sizing' ),

isBorderBox = boxSizingType === 'border-box',

width = editor.container.getSize( 'width', isBorderBox );

jacekbogdanski · 2020-11-23T14:05:01Z

tests/plugins/autogrow/_helpers/tools.js

@@ -19,8 +19,36 @@ var autogrowTools = ( function() {
 		return html;
 	}

+	function testEditorSizeWithContent( editorWidth ) {
+		bender.editorBot.create( {
+			name: 'EditorWidth' + editorWidth,


Running this test twice with the same editorWidth will result in an error as we will try to create two editors with the same name. Since this test helper has been extracted as a separate tool method, maybe we could improve it by adding some more random editor name? As an example. you could use new Date().getTime() as it's really unlikely it will match milliseconds:

name: 'editor_' + new Date().getTime()

jacekbogdanski · 2020-11-23T14:08:30Z

tests/plugins/autogrow/manual/4372.md

@@ -0,0 +1,14 @@
+@bender-tags: 4.15.1, bug, 7372
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, autogrow


Let's add some minimal editor plugins, so the editor toolbar loads nicely.

Suggested change

@bender-ckeditor-plugins: wysiwygarea, autogrow

@bender-ckeditor-plugins: wysiwygarea, autogrow, basicstyles, toolbar, resize

jacekbogdanski · 2020-11-23T14:10:20Z

tests/plugins/autogrow/manual/4372.md

+#### Expected result:
+The editor should have constant width. Only height is changing according to vertical amount of content.
+
+#### Unexpected result:


Suggested change

#### Unexpected result:

## Unexpected

jacekbogdanski · 2020-11-23T14:11:51Z

tests/plugins/autogrow/manual/4372.md

+1. Focus inside editable window.
+2. Add or remove extra lines to force editor resize.
+3. Observe editor width.


Suggested change

1. Focus inside editable window.

2. Add or remove extra lines to force editor resize.

3. Observe editor width.

1. Focus the editor.

2. Press `enter` multiple times to force the editor to resize.

jacekbogdanski · 2020-11-23T14:12:34Z

tests/plugins/autogrow/manual/4372.md

+3. Observe editor width.
+
+#### Expected result:
+The editor should have constant width. Only height is changing according to vertical amount of content.


Suggested change

The editor should have constant width. Only height is changing according to vertical amount of content.

Editor width is not changing during auto-resize.

jacekbogdanski · 2020-11-23T14:13:04Z

tests/plugins/autogrow/manual/4372.md

+The editor should have constant width. Only height is changing according to vertical amount of content.
+
+#### Unexpected result:
+With each vertical resize editor is shrinking for about margins + paddings.


Suggested change

With each vertical resize editor is shrinking for about margins + paddings.

With each vertical resize editor is shrinking for about 2 pixels.

That was my first thought. Later it comes to my mind (I may be over-obsessed) if the default margin and padding will change, those 2px will be invalid. Anyway, I apply suggestion :)

jacekbogdanski

Looks good!

I've ignored unit tests on IE8 as they were failing for no good reason despite that the manual tests worked fine.

sculpt0r · 2020-11-24T08:19:45Z

Looks good!

I've ignored unit tests on IE8 as they were failing for no good reason despite that the manual tests worked fine.

That IE thing... I have to remember to run tests on supported browsers. Thank you.

sculpt0r changed the base branch from major to master November 23, 2020 07:37

jacekbogdanski self-assigned this Nov 23, 2020

sculpt0r added 3 commits November 23, 2020 13:03

Add failing test - due width change

8bfcbae

Fix for #4372, resize with proper width

f915c4b

Fix code style

95dec77

jacekbogdanski force-pushed the t/4372 branch from 997aa01 to 95dec77 Compare November 23, 2020 12:04

jacekbogdanski requested changes Nov 23, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 23, 2020

sculpt0r added 3 commits November 23, 2020 14:14

Refactor according to PR comments

1c6ca3f

Join variables declaration

4e1c327

Add manual test for #4372 bug

0ffc87c

sculpt0r requested a review from jacekbogdanski November 23, 2020 13:44

jacekbogdanski requested changes Nov 23, 2020

View reviewed changes

sculpt0r added 3 commits November 23, 2020 23:12

PR: Correct manual tests according to suggestions

9d78cef

PR: Fix editor unique naming

44f8d56

PR: Fix variable declaration indent

e55d1f5

sculpt0r requested a review from jacekbogdanski November 23, 2020 23:05

jacekbogdanski self-assigned this Nov 24, 2020

jacekbogdanski added 3 commits November 24, 2020 09:03

Ignore tests on IE8.

c931958

Small corrections.

42a93bc

Added changelog entry [skip ci].

30f7947

jacekbogdanski approved these changes Nov 24, 2020

View reviewed changes

CKEditorBot unassigned jacekbogdanski Nov 24, 2020

jacekbogdanski merged commit 52649c4 into master Nov 24, 2020

jacekbogdanski deleted the t/4372 branch November 24, 2020 08:11

CKEditorBot mentioned this pull request Nov 24, 2020

Autogrow width issue #4372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix autogrow width issue #4393

Fix autogrow width issue #4393

sculpt0r commented Nov 23, 2020 •

edited by jacekbogdanski

Loading

jacekbogdanski left a comment

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020 •

edited

Loading

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

sculpt0r commented Nov 23, 2020

jacekbogdanski left a comment

jacekbogdanski Nov 23, 2020

sculpt0r Nov 23, 2020

jacekbogdanski Nov 24, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

jacekbogdanski Nov 23, 2020

sculpt0r Nov 23, 2020

jacekbogdanski left a comment

sculpt0r commented Nov 24, 2020

		@@ -8,7 +8,58 @@

		bender.editor = {};

		bender.createEditor = function( width, paragraphs ) {

	@bender-ckeditor-plugins: wysiwygarea, autogrow
	@bender-ckeditor-plugins: wysiwygarea, autogrow, basicstyles, toolbar, resize

	The editor should have constant width. Only height is changing according to vertical amount of content.
	Editor width is not changing during auto-resize.

	With each vertical resize editor is shrinking for about margins + paddings.
	With each vertical resize editor is shrinking for about 2 pixels.

Fix autogrow width issue #4393

Fix autogrow width issue #4393

Conversation

sculpt0r commented Nov 23, 2020 • edited by jacekbogdanski Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sculpt0r commented Nov 23, 2020

jacekbogdanski 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski left a comment

Choose a reason for hiding this comment

sculpt0r commented Nov 24, 2020

sculpt0r commented Nov 23, 2020 •

edited by jacekbogdanski

Loading

jacekbogdanski Nov 23, 2020 •

edited

Loading