Include CKEditor's AutoGrow plug-in

This sounds like a really good idea. While images is a large usecase, I imagine a even bigger usecase is all the content writers who paste in their long text :) - they will love this feature.

I'd love to see a patch!

This seems to work quite well now, I've tested the text in various ways and it expands as expected. I would RTBC this, but I am sure it could benefit from another pair of eyes on the code.

Tested, such a great improvement. Well done.

Hm. The autogrow pattern is not always a desired/expected behavior in my experience.

Somehow I expected this patch to just expose an option in the editor UI...?

I'm also surprised that we need to change so much (precompiled) code in order to include a "new" default plugin that's supposed to be there by default. Can't we include and make all of the default plugins available in core, and leave the aggregation to the aggregation?

Those seem like good questions to sort out.

Auto growing should be restricted to viewport size, having to scroll the page to see all of your content should only be possible with quick edit.

By scrolling inside the ckeditor area you should be able to see all of you content, without scrolling the page. It's such a pain when that happens because the toolbar doesn't scroll with the page.

As far as sun comment I'm pretty sure we're in a jquery-like situation. We can have custom builds but they're not dynamic.

Agreed with option to enable/diable autogrow.

Interesting. There are indeed usecases where auto-grow makes less sense, such as the mobile viewport.

I agree that we should make this an option, but I do think that we can have it on by default. There are usecases where this makes less sense, but I think for the majority of common content editing and writing usecases this change will be beneficial. Perhaps we can manage the usecases where it makes less sense, such as mobile?

We should also not forget that node edit form may use a different theme and width than the final public node... Not sure how this affects ckeditor here, but just had this idea.

I do not think that #2258313: Add license information to aggregated assets or #2276219: Asset libraries should declare their license is a blocker for JS minification. We also do not care about CSS framework licenses for years and just remove all comments (including the license comments).

I think the idea of adjusting it to window.innerheight makes sense, but I think this needs a patch to truly evaluate this suggestion. Because it might come down to fine tuning that percentage.

We could then formulate a default formula for the maximum autogrow height. E.g.:

0.8 * (window.innerHeight - Drupal.displace.offsets.top - Drupal.displace.offsets.bottom)

(That'd be 80% of the effectively available vertical screen estate.)
Thoughts?

What if the size of the window changes?

In my opinion config.autoGrow_maxHeight should be controlled dynamically, relative to the space around the editor. At the moment it is possible to override editor height with editor#autoGrow event, but it's buggy. I submitted some ideas to CKEditor bug tracker.

Well, we can't use this plugin?

Let's try and move this forward. In this patch I have simply updated the CKEditor 4.5.5 to include the autogrow plugin (haven't changed anything mentioned at #23 and I am having an issue already.

When I focus the editor and start typing it continuously gains height with each key I press. Attaching the patch and a gif showing the strange behavior.

I'm still getting the same behavior on Incognito Chrome 48 without plugins, Safari 9 and Firefox 43. Have you applied #24 and cleared caches? I also have js and css aggregation off.

I'm seeing it in Firefox on simplytest. Even clicking inside the text area to give it focus already starts to expand the box. Even backspaces and pressing only modifier keys make the box expand.

@strong>@w m-leers you've been unable to repro this with applying #24's patch - right? And what is the purpose of this patch? I see minified code and license updates, so I assume it's a version update, but that's just a guess.

@wim leers crossing fingers I didn't mess up the build.sh procedure

@mlewand the updates you see are just 2015 to 2016 copyright updates plus the addition of the autogrow plugin.

Let me check it

Problem seems to be here at refreshCache(); where the markerContainer is set: markerContainer = doc[ CKEDITOR.env.ie ? 'getBody' : 'getDocumentElement' ]();

Using markerContainer = doc.getBody(); seems to help solve the issue.

Well the trouble maker are these styles:

• https://github.com/drupal/drupal/blob/bf28e3d006fda29b0a67f2e6bb7ed1f218ab43be/core/themes/bartik/css/base/elements.css#L10
• https://github.com/drupal/drupal/blob/bf28e3d006fda29b0a67f2e6bb7ed1f218ab43be/core/themes/bartik/css/base/elements.css#L7

autogrow inserts an element after the body to get the height of the content. Doing so by appending block element to the body element seems to be fine, however all-non ie uses documentElement for that (looks like a hack, but don’t know the reason yet). It's a risky change.

@thpoul you're right, it seems to work for this case, however while it fixed this case I was able to break the scrolling by mashing keybaord on Firefox. (you the content has a scroll bar, but end user can't see it since autogrow sets overflow-y to hidden)

The quickest way here seems to fix the CSS so that it doesn't style CKEditor's html element.

@mlewand thank you so much for looking into it!

@Wim Leers should we open a child issue for the css change or handle it here in the same issue?

Actually it would just need

html {
  height: auto;
}

If we can override that (for the ckeditor only) it's ok

I think it would need to have a file for drupal where it could "fix" all these things. Like an 'extension' to its theme. It would be a solution to many things I guess like #2090937: Seven theme: style CKEditor-native dialogs to match Drupal-native dialogs

#43 So I guess this is postponed until it's resolved in CKEditor?

We can't blame editor for this, in this case that's just wrong CSS applied to the editor. contentsCss should be used solely for content styling. Using it for html / body element styling just breaks the editor.

Classic (framed) editor can safely assume that it's elements are not styled in such way, that's the reason why it's sandboxed in an iframe.

Rerolled the patch for 8.2.x (now having CKEditor 4.5.7) so it doesn't remain in the backlog just for that. Problem at #24 is still here.

I wish we could move this forward, therefore changing status to NR to continue the discussion.

Rerolled for 8.2.x. Same problem.

Well, all right. We'll provide a hotfix for this issue in our dedicated ticket: #14620 - stay tuned.

This fixes the issue.

This is now blocked from the CKEditor 4.5.9 release which includes the above fix.

CKEditor 4.5.9 version is out. Please confirm if it fixes the problem for you.

Thank you mlewand!! Reflecting that this is blocked from #2724225: Update CKEditor library to 4.5.9

On it.

Here it is :)

Oops sorry for the noise.

Let's try and move this forward. It needs more input as I'm having problem calculating Drupal.displace correctly within Drupal.editors.ckeditor.attach and I'm almost certain that my "on window load" method is wrong.

Thank you Wim for the review. Here is the updated patch.

I tried this out (Ubuntu 16.04 / FF 47.0 ) and noticed a bit of strange behavior. Not sure if this was covered above already, I apologize if that is the case.

The textarea does correctly grow and shrink according to the contained text, however:
During expansion I briefly see a vertical scroll-bar in the right side of the textarea. Also the text jumps up about half a line-height for a brief period of time during the expansion. Afterwards everything is fine.

I attached a small screengrab of the behavior.

Great idea!

The "twitching" of the text itself I can actually reproduce with the demo. The vertical scrollbar, however, does not appear there, so that seems to be Drupal-specific.

Ahh I'm sorry. I now tried the second demo on that page, and there I get the scrollbars as well. So I guess it's related to the max-height.

So it is indeed an upstream issue. That doesn't mean we need to block this on that, but I guess it's good to be aware of and maybe track upstream.

I can confirm this behavior (twitching in general, vertical scrollbar with max-height set) on OSX / Chrome 51. Both in Drupal and with the CKEditor Demo.

I've created an issue in https://dev.ckeditor.com for that problem with the vertical bar:
https://dev.ckeditor.com/ticket/14698

EDIT: A potential solution has been implemented: https://dev.ckeditor.com/ticket/14698#comment:2 The question remaining is whether it will hold up to scrutiny.

Yeah, I personally don't see that as a big enough issue to hold this up on that, but then I'm not a usability expert...

And here are the fixes from the eslint :)

Please note #2765751-2: Update CKEditor library to 4.5.10

This can't go in 8.1.x whereas #2765751: Update CKEditor library to 4.5.10 can, and has. Also this issue is affected by a bug fixed in 4.5.10 so I really think that one had to go first.

Also there was discussion early in the issue about make this optional that I can't see how it was resolved.

Here is the reroll after #2765751: Update CKEditor library to 4.5.10 commit. The 2239419-ckeditorjs.txt is just the diff without the vendor/ckeditor folder, for easier review.

+        // @todo Remove when http://dev.ckeditor.com/ticket/12120 is fixed.

I think we can now say "Remove when CKEditor is upgraded to 4.6." that is clearer IMO.

Hm. When I tried to manually test this on simplytest.me, it did not have the desired effect, or it did, but it was a bit finicky.

1) I uploaded http://assets.worldwildlife.org/photos/1139/images/carousel_small/elepha... on an article add form:

2) However, the text area did not "grow" accordingly (this is in the latest version of Chrome on OS X 51.0.2704.106):

3) If I click the image, it also doesn't grow. However, if I click just to the side of the image and give the actual text area focus, then it works as intended:

Also got some really funky stuff happening in OSX Safari Version 9.1.1 (11601.6.17), like no JS being loaded on the page at all, WYSIWYG editor not showing up, and the drop-button one long image. But that could've just been a simplytest.me glitch.

Additionally, when going back to edit the node, it looks like screenshot 2 again. Is that intended?

So it seems like we need to auto-focus the text field when adding a new image with no text in the box. Also, seems like when editing, it would be better to start at the "autogrow" size?

Tagging for manual testing also, to see if others can replicate the Safari weirdness.

@Wim Leers, I believe @webchick means on node edit when we talk about "starting" with the autogrow size, for a WYSIWYG field that already has a large something in it.

I tested this with text as well. When you save a node with a long something (text or image or whatever), then go back and edit it, the WYSIWYG is tiny on the new page load until it receives focus. So the reasonable (I think) questions are:

1. Should the WYSIWYG autogrow be triggered on a new page load, or is it sufficient to do when the WYSIWYG receives focus?
2. Even if the answer to #2 that first question is no, is there something we can do with the JS we've added to Drupal in order to make the WYSIWYG receive focus when an image is added, so that the autogrow is triggered? The fact that clicking on the image does not cause the WYSIWYG to receive focus may be a CKEditor bug, but the fact that the image editing dialog can result in it not having focus immediately afterward seems like it could be something we can fix in this patch.

The difficulty with UX improvements for dynamic interactions is that if they are (or feel to the end user) inconsistent, the improved UX can be unintentionally frustrating as well. So I think these are valid questions to ask.

Should the WYSIWYG autogrow be triggered on a new page load, or is it sufficient to do when the WYSIWYG receives focus?

Elaborating on this, once you've interacted with the WYSIWYG, it does not "auto-shrink" when it loses focus. Which is fair enough. But that also means that it feels buggy for it to be shrunken on page load but never go back to that state after it receives focus.

#95 @webchick I tried to reproduce the behaviour you described in Chrome 51@OS X in CKEditor, but failed to do so.

Could you tell which specific sandbox on simplytest.me you used to get this effect?

Could you tell which specific sandbox on simplytest.me you used to get this effect?

I'm kind of confused by this question; maybe I've misunderstood. The sandboxes go away after 30 minutes so one can't go back and visit it (but they are all built the same way so it should not matter). @webchick clicked the "simplytest.me" button Dreditor provides next to the patch.

I can also reproduce it in Chrome by :

1. Clicking the simplytest.me button next to the latest patch.
2. Installing that Drupal site normally.
3. Going to "Add content" and selecting "Article".
4. Clicking the image icon in the CKeditor toolbar.
5. Clicking "choose file.." and select a large photo.
6. Clicking on the alt text field and entering an alt text.
7. Clicking the save button.

If I stop there and touch nothing else, the text field does not grow to accommodate the image. It's only when I click on the field, select another browser tab, etc. that the auto-grow is triggered. I also reproduced this with keyboard navigation for the above steps. Interestingly, it does not seem to happen for a local installation in an Incognito window using the same steps after the first.

The bit about the editor not auto-growing upon the edit form being loaded though is reproducible in both cases.

Re #101 I can reproduce this in Opera also. Only when I edit the node does simply clicking on the image fire autogrow, when first adding the image you have to either click off or a button.

I tested this with both core default skin and my own on simplytest.me - same result.

I followed the steps from #101(via Dreditor) in Chrome 52.0.2743/OSX and the field did grow in my case, although trying later to change the size of the editor by clicking on the image/editable area failed and I think I know why:

I noticed that both the autogrow and resize plugins are loaded, which may contribute to the problem since these two don't work together well.

Unfortunately this fact is only mentioned in the sample shipped with the autogrow plugin - I'm sorry for that. I'll see to it that it appears in the documentation.

I believe disabling the resize plugin should help in this case.

Rerolled patch without resize plugin.

I think it would be a huge mistake to remove resize capability, the UX is terrible. If it can't work then I suppose we need to file upstream issues or not have autogrow, or find some other solution, I don't see the removal of resize as a solution (but a major regression).

Providing yet another setting in the UI to choose either is something that we should avoid if possible.

Can you elaborate on that? Seems to me making that configurable in the editor settings wouldn't be too bad of an idea?

If I add a large image I can no longer resize the editor to view my content with the image in context, I have to preview it, the resize autogrow max hight is too small, I have a huge screen resolution, but I still get this smaller than my viewport height box to mess about in. The same goes for long tables, I am now forced to scroll around in a box.

EDIT: don't get me wrong on autogrow - it's certainly awesome and I'm with Bojhan, it's a hard decision, on a balance autogrow may well be better for some users, but it's a harsh price to pay.

Unfortunately #108 sounds worse to me than not having autogrow. It sounds like it would make it totally unusable in some cases. I don't think we can ship like that, and it sounds worse than the bug Angie identified. I also don't think a UI configuration for it is a good solution, if it is choosing between these two options.

Is there not some way we can just work around the bug by triggering the autogrow with some small Drupal JS? All it has to do is put focus on the field after one of these dialog interactions and on node load, or otherwise manually kick the autogrow into action. We could ship with that, and then remove the workaround once it is fixed upstream in CKEditor.

And actually the bug with it not autogrowing is still reproducible on simplytest.me with the current patch. So unfortunately this is not ready; the current patch introduces a significant regression without fixing the bug with autogrow either.

Yes. If CKEditor (or any text editor) has a vertical height that's greater than the viewport

No one said anything about being higher than the viewport.

Otherwise you're stuck, you have to continue to scroll within CKEditor forever. This is a problem you'd have with certain themes, but also on just about every smartphone.

But I am not on a Smartphone Wim, and neither are the majority of actual content authors. In any case, I believe the term we should reach for here is "responsive design".

It prevents unusable cases that a user can manually put themselves in

Is that actually a problem, in the real world, if it is, it would be the first time I ever heard it being an actual "unusable case".

And I did NOT misrepresent anything at all - YOU misinterpreted and misrepresented my comment!

I want to use the whole viewport but autogrow does not allow that. I've added a screenshot to show how on one of my monitors (the smallest - Dell 25" 2560x1440) autogrow does not allow me to use about 420px of that height.

Mobile users will likely benefit from autogrow, but for large screen users this may not always be the case - we loose control and we're the major group of content authors. Autogrow is a good feature, but there are costs and those need proper review.

This needs a browserstack and accessibility review. For example it appears autogrow does not respect browser zoom, so if the browser is zoomed autogrow grows off the screen (below the fold etc). I've only had a change to test this in Safari.

@Jeff Burnz, can you provide some additional screenshots further illustrating the problems you describe? Edit: to clarify I'm still not understanding what the scrolling-around-forever problem is or isn't. E.g., before-and-after the patch.

It's difficult as a release manager to evaluate long, conflicting text arguments about the issue, and the fact that there is the disagreement at all indicates risk with this change to a stable feature.

Also, please note that the fact that autogrow does not happen until the field receives focus on node load is always reproducible and has never been addressed, even though I have mentioned it in nearly every issue comment I've made. If this were not the case, this screenshot should not be possible:

I am also still seeing the behavior Angie first noticed in her review intermittently when I test #104 on simplytest.me. I don't why @Wim Leers can't also reproduce it, but it's starting to feel a bit Emperor's New Clothes. Perhaps this intermittent bug is not a big deal, a trivial problem in itself and we should add the feature even if it still happens under weird circumstances we haven't figured out, but it's also not cool to dismiss that I can reproduce it. (Like, really, am I hallucinating or making things up?)

Finally, it is legitimate to be concerned about usability and accessibility impacts of this change, and to be concerned that all aspects of the change were not taken into account for the usability review. Let's take a step back and recognize that everyone contributing to this issue is doing so because they care about this feature enough to have tested it, and want to make CKEditor even more awesome to use.

It would be really helpful to not RTBC this issue again until these points have been addressed (and addressed by a third party reviewer). When we disagree, we consult others. This is a valuable feature, but since it's a change to stable code, it is fair to be careful of introducing regressions in the process of adding it. (That, or go down the route of making it an experimental feature, which would require significant changes to the patch also.) And the RTBC wars are really draining. There are 40 minutes between #108 and #111; I really don't think that's enough time to objectively consider the concern and get feedback about it. If this feature doesn't make it into 8.2.x because we had to spend some extra time untangling these questions, that is okay. Drupal 8 minor development is always open.

Thanks for your ongoing investment in this issue.

Also, before-and-after screenshots on mobile, including indicating the difference between autogrow and resize, would also help evaluate this.

I tested #104 on Chrome 52/OSX and here are my results:

• Insterting image: editor grows as expected.
• Saving page and returning to it: editor does not grow, but this is due to configuration: http://docs.ckeditor.com/#!/guide/dev_autogrow-section-height-adjustment...
• Switching Source/WYSIWYG: editor grows as expected.

If this is what happens consistently everywhere then setting autoGrow_onStartup to true in the config should be enough to have the editable area always grow as expected.

This patch sets autoGrow_onStartup to true. Now the editor automatically grows the moment it is created.

#115 Yes I can commit some time to accessibility testing and providing screenshots. I am rather busy this week so if anyone else has time that would be great also.

Side not: It is helpful if others do simple tests such as zooming the page 200% and leave that setting in place while you work through different scenarios. Accessibility needs to be everyones concern.

#117
#118

autoGrow_onStartup true certainly needs some discussion, are we assuming to much, or not enough if false?

I'm sprinting at DrupalCorn Camp and will test the zooming in my browser.

@ma.czizek: I too am sprinting at DrupalCorn Camp and will test the zooming in my browser:-)

It seems that if the page is already zoomed in (200%) and then the content is added, the body field will autogrow but does not extend “below the fold.”

However, if enough content is added to a page which is at 100% zoom so that the body field autogrows and then the page is zoomed in to 200%, the body field will extend “below the fold.”

Tested in Google Chrome version 52.0.2743.116 , Firefox version 47.0 , and IE version 11.0.9600.18376. Results were the same across the browsers; only text content was tested.

#122 that's great news and a good start. I don't think the 2nd scenario is a problem.

#119 imho if autoGrow is the only option (ie. no configuration available to the user) then it should have autoGrow_onStartup = true.

Since this issue didn't manage to get into 8.2 (such a shame), in order to move it forward and get it into 8.3.x, I suggest making it fully configurable for the users and let them decide whether they need it or not.

Configuration options could be:

• Enable autogrow (thus disable resize)
• Disable autogrow (thus enable resize)
• Enable autoGrow_onStartup
• Disable autoGrow_onStartup

Nice progress on this issue!

I don't necessarily think #125 should be off the table. I do agree that giving users too many configuration options is confusing. However, if we did something smart with the form (like replacing the thingy about "number of lines" that has the confusing text about WYSIWYGs) with these options when a WYSIWYG is actually enabled, and gave them sensible labels text that would reduce the burden of giving the user a choice. That could also be a followup, especially now that we are not under the wire.

I also do think it's at least worth discussing whether autoGrow_onStartup should be enabled by default, if that fixes the disconnect I describe in #98.

I added some remaining tasks to the summary. The first two un-crossed-off tasks might be complete already from before.

if we did something smart with the form (like replacing the thingy about "number of lines" that has the confusing text about WYSIWYGs) with these options when a WYSIWYG is actually enabled, and gave them sensible labels text that would reduce the burden of giving the user a choice

This is a pretty nice idea!

Yesterday I brought up this issue at the Drupal Usability weekly hangout and @gábor-hojtsy suggested that all concerns should be summarized at the issue summary in order to be easier for people to help figure out what is the best solution for this.

...especially now that we are not under the wire.

We might not be under the wire, but it would be great if we could manage and have this committed in 8.3.x at DrupalCon.

This is a nice feature, but of all the usability feedback I've had on CKeditor this is not at the top of the list.

I can think of a few off the top of my head that would have greater impact, including ck blocks, better code snippets and highlighting, two-pane source and preview.

Have we looked closely at overall impact?

@tkoleary, sure, but none of that is in scope for this issue, which is specifically about adding this feature.

@tkoleary Also, why did you revert my (extensive) summary changes from #127? That was a committer review; please don't undo those changes without summarizing how the points have been addressed? The points should be left there and the response to each documented so that committers know whether and how these review points have been addressed. If it was a crosspost, please be more careful.

Restoring previous summary.

@thpoul, I tried to make a start at summarizing the outstanding work; see the summary diff in #127 (and again here). Let's start with that?

@xjm

If it was a crosspost, please be more careful.

I'm sure that was the case, sorry.

Feedback from UX meeting with @thpoul, @webchick @bojhan and @yoroy.

Autogrow is the way to go (vs. resize). The patch is basically there, but:
Can we look at immediately autogrowing on load of the editor instead of on focus of the text area?

It would be great if this wouldn't attract that much attention to itself by growing on focus (a user input), ideally it would not grow then but already be grown (on load of the wysiwyg editor) to the maximum available height in the viewport.

Can we look at immediately autogrowing on load of the editor instead of on focus of the text area?

#118 autogrows the CKEditor on startup.

It would be great if this wouldn't attract that much attention to itself by growing on focus (a user input), ideally it would not grow then but already be grown (on load of the wysiwyg editor) to the maximum available height in the viewport.

+1

There was also one more thing that @webchick suggested to look into, regarding the "hiccup" of the initial <textarea> height just before the CKEditor loads and autogrows.

Hi,

I'd like to add another approach: Remember size of resizable textarea on a user and node basis on every save

Problem: When working with longer texts the textarea has to be manually increased. After a save it defaults back to its spezified size and one has to increase it manually again!

Proposed Solution: Remember the size of the expander and save it on a per user+field+node basis in order to reload that value whenever the node gets loaded. Could be stored in DB or in a cookie.

R, Max

Here is the reroll :)

autogrow on startup +1, looking good!

Credit.

Excellent work on this, everyone!

Seems like the earlier concerns about making this optional are addressed since the AutoGrow plugin effectively replaces the old version of doing the same thing.

We tested this LIVE on UX meeting this week and everything works as expected. Great work, everyone!

Committed and pushed to 8.3.x. Thanks!

Your FACE failed to apply a patch.

Also tagging for 8.3.0 release notes.

I don't see any documentation of the mobile testing?

Since the Resize plugin was removed, and Autogrow didn't make it into Drupal 8.2, the current official release now doesn't have neither. I have created an issue about it here: #2819009: Restore CKEditor Editor Resize plugin, until Autogrow is working

The Resize plugin wasn't removed, only the icon was hidden with a bit of CSS. A patch has been posted by @altrugon here: #2820200: Display resize icon for CKEditor on Seven theme.