Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autogrow width issue #4372

Closed
foraliexp opened this issue Nov 13, 2020 · 6 comments · Fixed by #4393
Closed

Autogrow width issue #4372

foraliexp opened this issue Nov 13, 2020 · 6 comments · Fixed by #4393
Assignees
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. regression This issue is a regression. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@foraliexp
Copy link

Hi,
I am trying to use the Autogrow plugin, I set the height "CKEDITOR.config.height = 150;" and width "CKEDITOR.config.width = '600';". Then I fill with the content so that the height matches, at this point the whole editor changes the width to a smaller one. Each new line reduces the width by 2px.

@Dumluregn
Copy link
Contributor

I can confirm this issue. It was introduced in 4.14.1. Steps to reproduce (see demo):

  1. Focus editor content (autogrow will adjust the height to the editor content).
  2. Start adding new lines.

Expected: Editor width doesn't change.
Actual: As @foraliexp mentioned, with each new line (but also then lines are removed), the editor width is decreased by 2px.

@Dumluregn Dumluregn added regression This issue is a regression. status:confirmed An issue confirmed by the development team. type:bug A bug. size:S labels Nov 13, 2020
@f1ames f1ames added the good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. label Nov 16, 2020
@Dumluregn
Copy link
Contributor

In #4286 there is an explanation why the issue occurs (see Debugging details section).

@sculpt0r sculpt0r assigned sculpt0r and unassigned sculpt0r Nov 18, 2020
@sculpt0r
Copy link
Contributor

Based on @Dumluregn comment and tests: it happens only when width is set on a fixed value.
Issue can't be reproduced with flexible width (CKEDITOR.config.width = auto;)

@sculpt0r
Copy link
Contributor

sculpt0r commented Nov 20, 2020

Also a related issue: #4323 and PR: #4376

Find that autogrow test fails when I use a bigger data set. So vertically scrollbar appears. Not sure if messure method should cover that in tests?

image

I think that autogrow test also missed some test cases - in the case of editor width. It could be auto, fix value like 200 or edge number case: 0.

So I think I add those cases at first.

The thing that makes me curious is a way of taking editor size. There is a method for that in autogrow tools:

width: parseInt( editor.getResizable().getComputedStyle( 'width' ), 10 ),

while plugin makes it only like that:
editor.resize( editor.container.getStyle( 'width' ), newHeight, true );

Shouldn't test use the same logic as a tested component?

@sculpt0r
Copy link
Contributor

The autogrow plugin takes width by:

editor.resize( editor.container.getStyle( 'width' ), newHeight, true );

which is independent of box-sizing. Seems to be good.

Next it's going to:

CKEDITOR.editor.prototype.resize = function( width, height, isContentHeight, resizeInner ) {

and call

outer.setSize( 'width', width, true );

which always assume width is border-box (last true argument).

It leads to:

CKEDITOR.dom.element.prototype.setSize = function( type, size, isBorderBox ) {

This method decrease provided width by margin and padding size.

size -= marginAndPaddingSize.call( this, type );

Summary:
The plugin should call editor.resize with width prepared to be decreased by paddings and margins. So I think there is a quite good method getSize(width, isBorderBox) which takes care of border-box.

@CKEditorBot
Copy link
Collaborator

Closed in #4393

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Relatively easy to fix. This is a perfect issue if you are willing to create a Pull Request. regression This issue is a regression. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants