Adjusting the row count in form display settings (e.g. in a content type form display) does not change the height of the text editor.
This is particularly bad for mobile users because there is no way to adjust the text editor height in touch - so mobile/touch users are forced to work in a narrow text editor no matter what, even when they have iPads etc with big resolution (my client is using iPad on the road for the next 5 months...).
Comment | File | Size | Author |
---|---|---|---|
#73 | interdiff-2717599-64-73.txt | 746 bytes | shashikant_chauhan |
#73 | 2717599-73.patch | 1.62 KB | shashikant_chauhan |
#64 | interdiff-2717599-56-64.txt | 787 bytes | criz |
#64 | 2717599-64.patch | 1.67 KB | criz |
#56 | interdiff-2717599-51-56.txt | 833 bytes | helenc |
Comments
Comment #2
Wim LeersThat sounds like a bug in CKEditor.
Comment #3
Wim LeersComment #4
mlewand CreditAttribution: mlewand commentedRelated issue in CKEditor's bug tracker can be found in issue #5153.
Btw as for Drupal tracker isn't this bug a dup of https://www.drupal.org/node/507696 or https://www.drupal.org/node/2410565?
Comment #5
Wim LeersReflecting that this is blocked on upstream.
#4: even if this is the same problem: this is Drupal 8 core, not the Drupal 7 Wysiwyg module. But I've added those as related issues. Thanks!
Comment #6
ifrikIs there anyway of solving this for Drupal? Given that the upstream CKEditor issue is about 6 years old?
This is a real UX problem because in the Manage Form Display it's possible to create a visual hierarchy with some fields have more rows then others.
This is displayed well when the text format does not use a text editor (such as the Restricted HTML format in Core) but all fields are displayed with the same 5 rows, when the text format uses CKEditor.
It also leaves sitebuilders wondering why they can set the number of rows, when it has no effect.
It's possible to override that by CSS, but fixing that manually for every single field that should have a number of rows is obviously not a good UX for site builders.
Comment #7
zserno CreditAttribution: zserno as a volunteer commentedA pretty simple workaround could be if we would compute and pass the height of the textarea as a CKEditor config property in each field's editor attach callback method. Attached a draft of this idea.
Comment #8
Wim Leers#7: nice hack :) You can do that in a theme. But it's not acceptable in Drupal core, because you make a lot of assumptions: line height to be 1.5em, a certain vertical margin, etc. That is a bit too simplistic, because depending on the theme (grep the codebase for
ckeditor_stylesheets
to know what I'm referring to), there may be completely different margins and line heights, or even just font sizes.Comment #9
zserno CreditAttribution: zserno as a volunteer commented@Wim Leers You are totally right, I guess that is the reason why the original issue hasn't progressed in 6 years.
I can see one way to come around this: set the editor's height after it is attached to the textarea. That way we can measure line-height and margins. Afterall it won't always be precise (because custom CSS styles can do wild things in the editor) but I think that would already be a huge help for site builders to see that updating the row number is effecting the height of the editor.
Comment #10
Wim LeersYeah, while I wrote #8, I was thinking the exact same thing… Depressing, isn't it? :(
At the same time,
rows
only really makes sense in a monospace/coding world, doesn't it? Because what if some of your rows are<h1>
s, and others are<p>
s? What would then even make sense?Have you seen #2239419: Include CKEditor's AutoGrow plug-in? Wouldn't that already adequately address this issue?
Comment #11
ifrikEven if the rows don't quite correlate with the exact number of rows depending on whether they are
<h1>
or<p>
, at least the number of rows allows site builders to visually structure a form. This won't be the exact number of lines that the user fills in but gives users an indication of what kind of length is expected.The autogrow functionality is a good usability improvement for the user who creates content later, but it does not solve the site builders issue of managing the form display.
Comment #12
zserno CreditAttribution: zserno as a volunteer commented#10 Absolutely agreed, good point! This issue is actually a duplicate of #2729271: Change the default height which has become a duplicate of #2239419: Include CKEditor's AutoGrow plug-in.
So closing this one as a druplicate. :)
Comment #13
Jeff Burnz CreditAttribution: Jeff Burnz commentedAutogrow sounds great, thanks for closing this and pointing to the new issue.
Comment #14
ifrikSorry, but Autogrow provides a different functionality, and it does not solve the issue at hand.
On the Manage form display, the number of rows is a required field. If a format with no text editor is chosen, that configuration is respected. If a format is chosen that uses CKEditor, it's broken.
If this can't get fixed, then the Manage Form Display needs to make clear that the display of a text area depends on which Text format/editor is used.
Autogrow is great for the user that has started adding content, but it is no solution for managing forms.
No Editor:
CKEditor:
Comment #15
Wim Leers+1
This is exactly the problem.
For
\Drupal\Core\Field\Plugin\Field\FieldWidget\StringTextareaWidget
, entering a number of rows is fine.For
\Drupal\text\Plugin\Field\FieldWidget\TextareaWidget
, it is not. Because there, you can specify a text format, and the text format may mean that you may use a text editor (when using theeditor
module or not), which may in turn mean that the number of rows configured there is irrelevant. So, we need to update their descriptions to correctly inform the user.Attached patch fixes that.
Comment #16
Wim LeersComment #17
Wim LeersThese changes should actually live in
editor
module, but:hook_form_alter()
that we can write to do this. That's why it lives intext
module.text
module.Comment #18
Wim LeersSuch noob!
Comment #19
ifrikThanks, this additional text means that site builders don't have to wonder any more what they might have done wrong.
Comment #20
ifrikScreenshot for the review:
Comment #21
webchickLet's get a usability review of the new text. My gut reaction is "That sounds like a problem. What do I do about it?" I think the answer is "nothing, you're screwed." :) But... dunno, feels it's missing something.
Comment #22
webchickLike, would a better approach be to check to see if text editors are enabled, and if so just grey out this option?
Comment #23
Wim LeersWe can't do that. Because a formatted text field (a field with
value
andformat
properties) allows the end user to select a Text Format, and it depends on the Text Format which Text Editor will be used, or even if any will be used. Some Text Editors — like the one in use here on Drupal.org, do still show a<textarea>
.So:
<textarea>
Hence this help text is the best we can do.
Comment #24
Bojhan CreditAttribution: Bojhan as a volunteer and commentedThe text is not very optimal. It starts with a condition (we should try to avoid), and ends with "ignored" - which makes it quite complex to understand.
Can't we just say that "A text editor might choose a different number of rows to display optimally" or something like that? Talking about the effect, not too much the cause or technical implication.
Comment #25
Wim Leers#24: works for me.
Comment #26
ifrik#24 also works for me.
As for
: At least it aknowledges that there is a technical problem, and that it's not the sitebuilder doing anything wrongly - which stops them going round in circles wondering whether they had actually set this rows, or whether they did at some time change something in the text editor configuration, or, or....
Comment #27
ifrikComment #28
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone as suggested in #24
Please review.
Comment #29
yoroy CreditAttribution: yoroy commentedThanks @snehi. I think we can do without the "optimally" word here, lets remove it. All text areas with the CKEditor show with the same height, this is far from optimal. Otherwise, this text looks ok (while still leaving us with an annoying state of affairs).
Comment #30
ifrikRegarding the patch in #7 and the comment in #8:
If the approach would work but can't be done in core because it would need to take theme specific settings into account, could we then have a patch like this for the Seven admin theme?
Then at least then admin theme that is available in Drupal core would work somewhat correctly.
Comment #31
Wim LeersThis issue needs just one tiny thing: removing the word #29.
inComment #32
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedHi @Roy Scholten, @Wim Leers,
I have removed the word "optimally" as suggested in #29,#31 from the latest patch .
Please find the patch attached.
Thanks,
,
Comment #33
Wim LeersThanks! Could you please provide an interdiff next time? See https://www.drupal.org/documentation/git/interdiff.
I compared manually this time, and it looks great.
Comment #34
webchickReviewed this with @xjm and @effulgentsia. We were concerned that:
'A text editor might choose a different number of rows to display.'
...might make it sound like a human editor of text is choosing something, rather than this setting being overridden by code.
So what about something more like (just a suggestion, feel free to iterate to find something better):
'Text editors might display a different number of rows.'
By linking to the Editor module help page, people can get more information on what that means.
Comment #35
Jeff Burnz CreditAttribution: Jeff Burnz commented@zserno #7 thanks for the trick, I used this in my editor skin, cheers!
Comment #36
Wim LeersLet's do #34.
Comment #37
thpoul CreditAttribution: thpoul at Pixual commentedComment #38
thpoul CreditAttribution: thpoul at Pixual commentedI now see what @webchick actually meant. Sorry for the noise. Hope the link is right.
Comment #39
Wim LeersYou can't hardcode that URL. Use :url.
Comment #40
thpoul CreditAttribution: thpoul at Pixual commentedOops!
Edit: Wrong interdiff posted, it's actually 32-40
Comment #41
Wim LeersYep, much better indeed!
Comment #43
Wim LeersRandom fail in
ConfigImportAllTest
.Comment #45
Wim LeersNow random fails in
ManageDisplayTest
,NodeTypeInitialLanguageTest
,DateTimeTest
. With one very interesting exception in all of them:Hm…
Comment #47
Wim LeersOh hah, this is linking to the
help.page
route, even if it doesn't exist…Basically, what @webchick asked for in #34 is impossible. It's only possible conditionally:
… I'm not sure whether that is something we actually want. Pinging @webchick.
Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn ResponsiveImageStyleForm::form(), there's:
But not much other precedent in HEAD for different help text based on whether help.module is enabled. I think #47 does make sense though.
Note that per above example, something more descriptive than
:url
would be nice. Perhaps:text_editor_help
?Yeah, since there's not much clear precedent for #description that varies by whether help.module is installed, probably a good idea to get her feedback, so tagging accordingly. Especially since it will mean 2 very similar strings (differing only by the presence of an
<a>
tag) needing to be translated by translators. If we introduce this type of same text, but linked or not based on help.module installation, I could imagine us wanting to use the same pattern elsewhere.Comment #49
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFurthermore, editor.module might also not be enabled. Here's a patch that implements #47.
In this patch, that text is now different, because I added an additional sentence in the case of help.module not being enabled.
Comment #50
webchickOh dear. Yeah, this is probably not worth all the hassle. @xjm's concern was that "text editors" might sound like "an editor (person) of text" to the non-initiated. Then again, this is a user 1-ish admin page, so not sure we need to be all that concerned about it. I suggested putting "WYSIWYG" in there somewhere but was resoundly shot down. :P But maybe something on that order?
Anyway, this doesn't need to be held up on me, nor does it need product manager review. It's a simple string of text.
Comment #51
Wim LeersOkay, so now what? We just go back to #32 AFAICT. Reuploading #32 and re-RTBC'ing.
(Like #50 says, that should be fine, since it's for user-1-ish users anyway.)
Comment #52
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer #34, it's not just the phrase "text editor" that is semi-ambiguous as to whether it refers to software or to a person, but also the word "choose" that furthers that ambiguity. So here's wording that's closer to #37 that I think is clearer that this isn't referring to a person making a choice.
Additionally, when I Google "text editor", Google returns this definition ahead of the list of website matches: "a system or program that allows a user to edit text." So I think we're using as good a terminology as we can here, even if people who are confused might not think to do that search. Linking to a help page was a good idea, but as #49 shows, too cumbersome.
I'm still not entirely +1 to this patch though. It seems to me there are 3 distinct reasons why a text editor might ignore this setting:
But this patch provides no information about these 3 possibilities, and therefore, how helpful is it really?
Comment #53
xjmBy the way, we do use conditionals for help page links based on whether or not the help page is available elsewhere in core, based on a permission, whether a module is enabled, etc. So that, in itself, is not too cumbersome, if it would help the user understand. And if they are configuring this setting, they likely also have access to that help page, so it is the 80% case.
I think the current string in #52 is definitely better, because it does not personify the text editor as much. However, the word "ignore" still has emotional connotations. How about "override"?
(@effulgentsia's other points are also worth discussing; just wanted to add this.)
Comment #55
helenc CreditAttribution: helenc at Torchbox commentedFirst of all hello - I found this issue via issues tagged as 'novice'. I hope I am not sticking my oar in too much here.
1) I agree that 'text editor' as used in the wording 'A text editor might choose a different number of rows to display' sounds like a person. But if the link doesn't work as webchick suggested, we just need to go back to making clear that 'text editor' refers to a text format you can select, e.g.
"Note that this setting may be ignored if your chosen text format for the field uses a wysiwyg text editor'.
2) Why is the number of rows a required field at all: if it is sometimes ignored, it would make sense to make this optional.
3) Finally, if the number of rows is to remain a required field, I would suggest upping the default value from '3' for the body field - 3 is very short.
Edit: I see 3 is for the 'summary' part of the body - the main body area has 9.
I will attempt to make a patches for some of the above points.
Comment #56
helenc CreditAttribution: helenc at Torchbox commentedPatch for item 1 in comment #55 attached. I removed the words "Note that" to make it slightly less wordy.
Comment #58
Manjit.Singhtriggering test bot.
Comment #60
Wim LeersJava errors, not test failures:
So, retesting.
Comment #61
Wim Leerss/wysiwyg/text/
Because: at
/admin/config/content/formats
, we also say , so we should be consistent.Comment #62
Wim Leers#2788905: "Number of rows" configuration on Page Body field not working reported exactly the same:
Bumping from
to since it seems to be confusing more and more people.Comment #63
valthebaldToo long discussion to fit into 'Novice' category, removing Novice tags as part of Novice triage BoF Dublin 2016
Comment #64
crizOkay, so here is an updated version of the last patch that uses more consistent wording and makes it more clear by naming a text editor as example.
Comment #65
valthebaldThis could use better wording. My bet is 'This setting may be ignored if the text format chosen for the field uses WYSIWYG editor (i.e. CKEditor)' (or suggestion from #61)
Comment #66
criz@valthebald As I understand #61 we should not use the term "WYSIWYG" to be consistent. It is called "Text editor". So adding your suggestion it would be "This setting may be ignored if the text format chosen for the field uses a text editor (e.g. CKEditor)."
Comment #67
Wim Leers#65: -1, we should not use "i.e.", because it's possible to install another text editor.
Comment #68
xjmI think adding the example of CKEditor helps a lot to clarify what this is about! Good idea.
The confusion above about "i.e." versus "e.g." gets at the reasoning behind #2684683: [meta] Replace i.e. and e.g. with English words. So let's avoid it and use the English word "like".
The sentence is now harder to read due to the conditionals and passive voice, though, plus still uses the word "ignore". I'd suggest:
While the information that it's specifically when the text format for the field is configured to use a WYSIWYG is more specific and useful, I don't think it's helpful enough in context to merit a long and more complex sentence with a bit of terminology overload.
Phew, usability is difficult. ;) Thanks everyone for iterating on this. I agree with making this normal priority.
Comment #69
xjmOr maybe:
Comment #70
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI also wonder if "override" is clear in what it does?
Comment #71
Wim Leers@xjm: it's
CKEditor
, notCKeditor
:)Other than that, +1 to #68. I think #68 is better than #69 for the reason given in #70.
Comment #72
ifrikI'm also fine with #68.
On #70: Even if somebody wouldn't know exactly what "override" might mean when they first read it: they will have an indication what's going on when they stare at a form that doesn't look like they expected it....
Comment #73
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedadding patch.
Comment #74
Wim LeersComment #78
xjmReally great collaboration, everyone.
Committed 4318d98 and pushed to 8.3.x. Thanks! NB: I accidentally pushed it to 8.2.x as well, which was an accident since this is a string addition. So that's what the revert message is about.
I love that the list of credited contributors for this issue is longer than the actual text. :)
Comment #79
Wim Leers:D
/me highfives everyone who helped!
Comment #81
John Pitcairn CreditAttribution: John Pitcairn commentedFor anyone else stumbling across this, the hook_editor_js_settings_alter() solution (based on the text format) documented around the web no longer works in Drupal 8.3.x.
A pure css hack is possible. Set the editor height based on the textarea rows attribute for every row height you will use. If you enable the editor maximize button, override those heights when maximized:
Comment #82
geek-merlinReopened #2788905: "Number of rows" configuration on Page Body field not working to discuss about a real fix.
Maybe we can continue the work of #7..#10.
Comment #83
PhilYFor anyone landing here from search engines, see CKEditorHeight module.