CKEditor 5 should not grow to infinite height

Upstream issue: https://github.com/ckeditor/ckeditor5/issues/9489

Upstream issue: https://github.com/ckeditor/ckeditor5/issues/9489

Just a question.
I noticed the upstream issue description says:

for example, if .ck-content's offsetHeight > 1000, I want to be not able to typing text in editor.

But if I got it right, it's different from @Dom.'s desired behavior. I believe he doesn't want to limit the amount of text, but instead only limit the height of the textfield (maybe by adding a scrollbar to the field if the text is too long). That would allow him to keep writing 'long blog posts' while allowing him to keep seeing the bottom and top of the page without too much scrolling.
Is https://github.com/ckeditor/ckeditor5/issues/9489 proposed solution the right one for this issue?

yes altering the height via css is also the solution the ckeditor devs recommended over in https://github.com/ckeditor/ckeditor5/issues/636 . with max-height you would achieve the goal of this issue and with height the min and max goal ( from this issue https://www.drupal.org/project/drupal/issues/3241295). but i've tried that yesterday by applying the fixed height via css and i have to conclude that at least in safari 13.1.2 the scrolling was sort of clunky and jumpy in regards of the scrollbar. but @wim-leers also argued against fixing the issue downstream with css but instead would still go for an upstream fix: https://www.drupal.org/project/drupal/issues/3241295#comment-14424588 (cuz i asked there as well why not try the css fix downstream - but i understand his point made and it is reasonable)

p.s. the comment refers to comment #6. @dom posted the follow up comment in #7. but will take a look at the patch now :)

we check the computed height of that textarea in the JS and add that as an inlined height to the ckeditor equivalent ?

That would pretty simple to create a PoC patch for— could you do that maybe? 🤞 LOL you already did in #7!

#8: In #3241295-7: CKEditor 5 isn't respecting field widgets row settings I was especially concerned about the expectation described in that issue: matching the <textarea rows="N"> behavior. That is impossible. But something that is approximate, like limiting the max height … that's a different matter.

+++ b/core/modules/ckeditor5/js/ckeditor5.es6.js
@@ -430,6 +433,15 @@
+              'height',

Shouldn't this use max-height instead of height? Otherwise this would be attempting to solve #3241295: CKEditor 5 isn't respecting field widgets row settings, which is unsolvable?

I think it'd be great if you could record a very short video demonstrating both cases — we'll need that anyway to get this committed. And that will allow us to jointly decide whether one of the approaches is acceptable.

I'd also for sure like to see max-height in action because I was trying to limit this patch to the scope of this issue and not also mix in #3241295: CKEditor 5 isn't respecting field widgets row settings.

I think it would make sense to update the wording in the setting form

+1

In the default behaviour, using Claro as admin theme, the text area field initially is indeed smaller then the toolbar, and rather swamped by label, toolbar, the dropdown to change the format. Having the field larger by default, would mean that the user can find the actual field more easily. And we expect the user to enter more then just a few words anyway - if not then the field should have not been text area anyway.

Roughly estimating a field height would be a good enough solution. Sitebuildesr don't necessarily want to create one field that has exactely 5 or 12 rows. They rather want to make smaller and bigger fields as a way of showing whether long or short bits of text are expected.

Happy to wait for a video to compare the options.

i like both use cases shown (height and max-height). thanks for the animated gifs for illustrating the patches! would it make sense to let the site builder choose in the settings on a per field basis if the value should be for min-height, max-height or height (even though this issue is only about the max-height)? it just would have to be decided which of the three options should be the default behaviour for a ckeditor5 field?

I think we need some input from the Claro developers and a UX review, because whether or not text areas have autogrow significantly affects the admin theme.

Two screenshots for comparison about how visible the body field is in a default Article form, using Claro on a narrow screen - as a quick impression: on the left using CKEditor 5 default autogrow, on the right with a fixed minimum height.

The min-height was fixed over in #3241295: CKEditor 5 isn't respecting field widgets row settings. Yay!

In comment #3241295-36: CKEditor 5 isn't respecting field widgets row settings, @mherchel fixed this too, but as unrelated art, that did not make it into the commit.

Here it is:

+.ck-editor__editable {
+  max-height: calc(100vh - var(--drupal-displace-offset-top, 0px) - 20px);
+}

Given that CKEditor can be tweaked so easily, i doubt fixing this is an upstream job, so removing that tag.

Added a patch for Drupal 9.5 adding on #3241295: CKEditor 5 isn't respecting field widgets row settings

I think it'd be great to have @mherchel review this 🤓

#20: I think copying over the min-height is not right.

Much appreciated, I'll try to test this as soon as the other work load allows it.
My use case outlined in https://www.drupal.org/project/drupal/issues/3241295#comment-14812492 should mostly be fulfilled by the max-height option here (read: should be fair enough to have an adequate UX for editors that fill in one-line title fields that need to be HTML).

Still I want to mention a possible relation to upstream's behavior once https://github.com/ckeditor/ckeditor5/issues/9489 gets implemented.
There could be a use case where not only the height of the editor box should be restricted to a given amount but its actual content (that would even be better in my use case, which should, strictly speaking, be the use case for every Drupal site owner as it is the only way of creating accessible node titles if they contain foreign language terms/phrases; made this a core issue at https://www.drupal.org/project/drupal/issues/82751#comment-14852902).

I think we need an additional option in the field settings if both min-height and max-height get set. Maybe designers want to control both aspects separately.
The most intuitive way for me seems to be to offer two fields, for (approx.) min rows and (approx.) max rows.
Using just one number input as it is now and adding a checkbox or a radio button to define whether this should be min or max is not as flexible as being able to define both.
Just setting the min AND max height to the value entered (as I think this patch here together with https://www.drupal.org/project/drupal/issues/3241295) takes away flexibility.
If a change to the field config to allow for two inputs however takes another decade as fixing the max height did, I'd pleasantly go for just one input that fixes it to the given amount and disables growing/shrinking ;-)

I think it'd be great to have @mherchel review this 🤓

I'm totally willing to help out. But do we have consensus on what the behavior we want is? In #14 we have animated GIFs (with a soft G) that show the different behaviors. In #15 @rkoller states that he'd like an option for both.

If want to provide multiple options, it increases the scope, but is doable. Assuming so, we need to decide on verbiage and what we want the admin UI for that to look like. We also have to figure out what we want the default behavior to be. Only after that can we pull those settings into CKEditor.

It might be simpler to make a default change, and then create a followup to change the behavior. Either way, we have some decisions to make.

@geek-merlin as a quick test it worked for our needs. However it does duplicate the CSS. I was not too sure how to build on @mherchel patch for min height.

@mherchel we would love your input here. Just adding in your line of css servered our needs and multiple screen resolutions. For us it makes the CKeditor5 work much like the CKeditor4. So I think this should be the default behaviour. Adding in options increases the scrope and I am not sure this is really necessary IMHO.

For us it makes the CKeditor5 work much like the CKeditor4. So I think this should be the default behaviour. Adding in options increases the scrope and I am not sure this is really necessary IMHO.

+1

Which behavior are we looking to implement:

1. The height of the editor always stays the same
2. The height of the editor grows to match the viewport

Both should be straightforward to solve.

• To solve #1, change https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/cke... to use height instead of min-height
• To solve #2, add the following to the same selector:max-height: calc(100vh - var(--drupal-displace-offset-top, 0px) - 20px);

@mherchel thanks for your clear points. I think 2 is the winner to accomplish most if not all user cases. My patch in #25 implimented this and it worked well. However my patch duplicated instead of adding to

.ck-editor__main > :is(.ck-editor__editable, .ck-source-editing-area)

My attempt was for Drupal 9.5 as well.

Just fixing it to the given height is alright with me and reproducing CKE4 behavior also seems like a good idea.
I just thought that https://www.drupal.org/project/drupal/issues/3241295 explicitly wanted to implement an auto-growing feature. If it's not necessary to keep this, I'm all in.

We came across an issue when using CKeditor "View Source". The textarea does not have overflow: auto set which causes issues. Here is an updated patch based on #20.

+++ b/core/modules/ckeditor5/css/editor.css
@@ -8,3 +8,18 @@
+  max-height: calc(100vh - var(--drupal-displace-offset-top, 0px) - 20px);

Note that these CSS variables don't exist within Drupal 9.x. this means that the editor field might overlap the toolbars.

Playing around with the method that stops the autogrow from growing larger than the viewport height.

The user can still inadvertently (or purposefully) scroll down and have a significant part of the editor out of screen. Not sure if this is a problem.

However with #3331158: Support full-screen editing in CKEditor stalled, I believe that we do need a larger editor.

Attaching a patch. Still needs tests, but if we agree this is the desired behavior (and we can get maintainer signoff), I'll write the tests.

Attached a 0 byte patch. Attached is the correct patch.

@mherchel to avoid the:

The user can still inadvertently (or purposefully) scroll down and have a significant part of the editor out of screen. Not sure if this is a problem.

I introduced a small css tweak which resolved the issue for us in Drupal 9.5.x:

.ck-editor__main > :is(.ck-source-editing-area) > textarea {
  overflow: auto;
}

Verified the patch #33 and tested it on Drupal version 10.1.x. The editor initially has a min height and when content is added, it stops growing after a certain max height. Then an internal scroll will appear within the editor. I have added the before and after screen recordings for reference.

The before vs after videos from @Nayana Ramakrishnan make it crystal clear that this is a huge leap forward in usability! 🤩

And magnificently, this is a CSS-only change! 🥳

This is a no-brainer improvement 👍

I think it's deserving of this 😇

Just updating tag

We need to understand whether #3326193: CKEditor 5 can grow past the viewport when there is a lot of content is a duplicate of this or has other purpose. Not very clear to me.

Still needs tests as per #33

I didn't even realize we could write tests for this; that used to be completely impossible. I guess it indeed is possible here. But given it's viewport-related I have some reservations about how reliable that test could work on headless Chrome DrupalCI testing 🤔

@mherchel & @lauriii: thoughts?

Over at #3326193: CKEditor 5 can grow past the viewport when there is a lot of content, I wrote:

Implement the solution that https://ckeditor.com/docs/ckeditor5/latest/installation/getting-started/... recommends:

Classic editor (CKEditor 5) no longer encapsulates the editing area in an <iframe>, which means that the height (and similar options) of the editing area can be easily controlled with CSS. For example, the minHeight and maxHeight settings can be achieved with the following code:

.ck.ck-content:not(.ck-comment__input *) {
/* Note: You can use min-height and max-height instead here. */
  height: 300px;
  overflow-y: auto;
}

… but combine this with JavaScript that computes the desired height, just like we did in #2986147: Autogrow for non-admin users causes CKEditor grow past the viewport when there is a lot of content in the CKEditor.

And #3241295: CKEditor 5 isn't respecting field widgets row settings already implemented min-height, this issue is adding max-height. We are using a different selector than the official docs recommended, but given that we use the "classical" editor UI, I think .ck-editor__main is fine — plus, the official docs were written before the Source Editing plugin existed, so our addition to the selector just for that IMHO makes sense: .ck-editor__main > :is(.ck-editor__editable, .ck-source-editing-area).

Apologies for the duplicate issue 🙈

Transfering issue credit and fixing title.

Same comment as in #3241295-99: CKEditor 5 isn't respecting field widgets row settings: This is a bug that affects also Drupal 9.5 and 9.5 is still long time supported (until December 2023). Not porting this to 9.5 is affecting the CKEditor 5 adoption. Most of the sites are now on Drupal 9.5 (or even 9.4), preparing for D10. Adopting CKEditor 5 is one of steps they want (true, not mandatory, but highly recommended) to solve before migrating to Drupal 10. But this is the kind of bug that wouldn't make CKEditor 5 module production ready.

Let's backport this, at least, to Drupal 9.5 to ease the CKE5 adoption. It seems that the 9.x version requires #3241295: CKEditor 5 isn't respecting field widgets row settings to be fixed first because they touch the same area of code.

Re #33:

+  /* Set the max-height to not grow beyond the height of the viewport (minus
+   * any toolbars. */
+  max-height: calc(100vh - var(--drupal-displace-offset-top, 0px) - var(--drupal-displace-offset-bottom, 0px) - 20px);

Should the internal.drupal.ckeditor5 have also core/drupal.displace as dependency, in ckeditor5.libraries.yml?

This is a D9.5 tentative. It cumulates:

• #3256768: Drupal.displace() should set CSS Variables indicating displacement values
• #3241295: CKEditor 5 isn't respecting field widgets row settings
• Fix from #33

...so not something that can be merged but it might be used as patch for D95 projects.

I am not a UX expert, so I am not sure if this is really problematic :). I'd like to point out though that the suggested max-height only considers the "ckeditor-editor part". The label, help text and text format selector will not be (fully) visible in the viewport when the CKEditor has reached it's maximum height.

I attached a video to demonstrate this. Demo styling has been applied: ckeditor (light yellow background) grows to fill the full viewport (minus drupal-specific "safe displace areas") but parts of the complete drupal field (light blue background) are pushed out of the full viewport.

Also not a CSS person, but 100vh used (?) to be problematic when used on mobile (_native_ not desktop-responsive) where relying on 100vh for "full viewport" can be tricky (e.g. https://chanind.github.io/javascript/2019/09/28/avoid-100vh-on-mobile-we...).

PS. I'd be happy to give a shot at amending the existing nightwatch test (adding "maximum height") in drupal/core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5EditorHeightTest.js ...

Here a (functional js) test-only patch (failing) and a passing test with CSS attached.

As stated in #52 I see some UX issues, but this can easily be adjusted accordingly.

fix ccf error in #53

Sorry for the noise, coding standard fixes and cspell-related issue resolved. only-test.patch should fail the other one should pass.

Reacquainting myself with core contrib and its linting processes, sorry🤦.

The #56 patch is identical to @mherchel's in #33, just with added test coverage goodness! 🤩

Looks great!

@wimleers - a quick pointer to #52 so it doesn't get lost. I am not sure the UX is quite optimal yet (body field label and text input selector pushed outside the viewport, see video in #52) but that's above my paygrade :)

+++ b/core/modules/ckeditor5/tests/src/Nightwatch/Tests/ckEditor5EditorHeightTest.js
@@ -100,6 +102,30 @@ module.exports = {
+            window.Drupal.CKEditor5Instances.forEach((instance) => {
+              instance.setData('<p>Llamas are cute.</p>'.repeat(100));
+            });

❤️🦙

Committed c3a9cde and pushed to 10.1.x. Thanks!

Since this is a feature, it doesn't get a backport to 10.0.x or 9.5.x.

Just sharing a backported patch for 9.5.x in case anyone else needs it. Thanks to everyone who worked on this issue!

@Cottser Thank you

Added default height

Do we have a patch working for drupal 9.5?

#66 Support 10.1.1

#66 Support 10.1.2

Sorry please ignore this file, it has been fixed and merged into 10.1.2 in https://www.drupal.org/project/drupal/issues/3372922

@sir_squall #66 works in 9.5.x

@solideogloria Thanks!

I have migrated to D10 where this issue is not there anymore, but I appreciate that you have answered ;)