Text fields not big enough

Created on 18 July 2019, almost 5 years ago
Updated 7 February 2023, over 1 year ago

Problem/Motivation

In some cases, the texfields do not allow all content you want to enter. The example (and possibly only relevant case) I ran into is the feature security header, that can become a very long list of features.

In addition, it can be very hard to check multiple fields at once for similar entries.

Proposed resolution

One proposal was to change the fields to text areas. Especially for the feature security, it might be good to have separate input fields for all possible features, although that would be quite a UX challenge. Would we also need to cater for entering your own features? Or make that a configuration in itself, the offered features in the form..?

Remaining tasks

* Reach concencus.
* Write patch
* Review

User interface changes

The input fields would be changed somehow, to make them more easily inspectable and in case of feature security, allow any input desired.

API changes

None.

Data model changes

Especially the proposed change for feature security would require a different way of storing the configuration.

Original report by dunx

It's very hard to check multiple fields at once for similar entries. Can the majority of fields be changed to use ?

✨ Feature request
Status

Fixed

Version

1.0

Component

Code

Created by

🇬🇧United Kingdom dunx

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Instead of changing all of these inputs to textareas, why don't we increase the maxlength to something less restrictive?

    AFAICS Drupal's formapi doesn't have a maximum value for maxlength which is likely because there is no maximum in the spec:

    Per that last thread, it looks like browsers implement a very high limit in practice, but seems like we could - e.g. - add a zero to the end of the maxlength without breaking anything.

    I am not a UX expert, but I'd think it best not to add 13 textareas to the already very busy admin form.

    I did have a look at using a textarea but adding some attributes to keep the form element roughly the same size by default, but I think we'd then need to get into messing with CSS (e.g. textareas seem to default to width: 100% and the grippie div below them matches).

    Example:

       // CSP default-src directive.
       $form['seckit_xss']['csp']['default-src'] = array(
    -    '#type' => 'textfield',
    -    '#maxlength' => 1024,
    +    '#type' => 'textarea',
    +    '#attributes' => array('rows' => 1, 'cols' => 68, 'style' => 'width: auto'),
         '#default_value' => $options['seckit_xss']['csp']['default-src'],
    

    That seems to work with default "Seven" admin theme, but the grippie div would need some extra CSS, and it'd almost certainly be better not to add a style attribute to the form element.

    It's not too hard to get the CSS to work for Seven etc.. but we risk making a mess in other admin themes.

    On the other hand, just changing all the fields to textarea makes quite a visual mess IMHO.

    All that said, I'm quite inclined to just add that zero to the #maxlength for each textfield.

    Any objections?

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Had to give the textarea + CSS route a try.

    I'm fairly happy with this in Seven, but haven't looked at any other admin themes.

    There are other textareas in the seckit admin form that we could add the rows=1 attribute to for consistency.

    Increasing the maxlength would be a simpler change than this, but it's not very friendly UX.

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Tweaked to make the same change to most of the other textfields / textareas in the admin form.

    I've not changed the textfields which are "max" values expecting small integers.

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Added a test for a long value being submitted for a CSP directive in the admin form.

    I'll commit this patch if tests pass.

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Per #4 one concern here is that users may enter line breaks into textareas and that's likely to cause problems with most if not all of the fields we're changing here.

    We could add validation to raise an error if any of the textareas contain line breaks?

    That might be better than adding to the submit hook to silently remove line breaks - although that's what D7 does to textfields:

    https://git.drupalcode.org/project/drupal/-/blob/7.94/includes/form.inc#...

  • 🇧🇪Belgium Dozz

    Hi, I believe allowing line breaks would be great advantage especially if this change would also be ported to the D8+ versions.

    It would allow people to group entries on different lines but more importantly, make it possible to have entries on separate lines in the configuration files.

    Especially on larger sites, where multiple features are developed and tested simultaneously, multiple feature-branches often include a different change on the same line (e.g. adding an item to script-src) in the configuration file (seckit.settings.yml) which results in harder to fix merge conflicts.

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Okay, but we'd then have to remove the newlines when processing the directives, unless I'm missing something?

    I can see the advantage for D9+ when checking config into version control, but I think that's out of scope for what we're looking at here.

    I'm trying to get this "small" change done in an attempt to prepare a new D7 release :)

  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Here's some validation for the textareas that shouldn't contain newlines (assuming their values are going straight into http response headers).

    Plus a test of that validation.

    I think I'm done for now; will commit if tests pass.

    @dunx your suggestion of allowing newlines in some fields likely makes sense especially in the D9+ branch. I think that should be a followup issue though. Thanks!

    • mcdruid → committed 2647535b on 7.x-1.x
      Issue #3068681 by mcdruid, callinmullaney, dunx, ron_s: Text fields not...
  • Status changed to Fixed over 1 year ago
  • 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

    Marking this as fixed for the 7.x-1.x branch. Thanks!

    A followup issue should be filed for 2.x (and perhaps that should accept + process newlines in some of the inputs).

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Created ✨ Text fields not big enough Fixed

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024