Remove default #size attribute from core

Yes please, let's remove it from core. We really shouldn't be using this anymore unless there's a good reason, like captchas and phone numbers (but even that's debatable). The themes are responsible for displaying how a form looks (especially in a lot of frameworks). If we "need" input widths on forms in core, we should be adding that CSS to seven or bartik.

Another alternative would be to simply remove it from core add #size back in both the seven and bartik themes by preprocessing the elements. This would fix both the "get it out of core for themes that don't need it" and it would temporarily get around the CSS nightmares you're having. Bartik and seven kind of expect these to be set (I'm pretty sure they don't style input widths... could be wrong though).

diff --git a/core/modules/system/css/system.theme.css b/core/modules/system/css/system.theme.css
@@ -84,6 +84,10 @@ h4.label {
+[type="date"], [type="email"], [type="file"], [type="password"], [type="search"], [type="tel"], [type="text"], [type="url"] {
+  min-width: 55%;
+  max-width: 100%;
+}

This REALLY should be added to the individual seven and bartik themes, not system.theme.css (which is also very assumptive). Many frameworks already provide handling of input elements. Enforcing this through core just causes completely unnecessary overrides.

-1

Without heavily considering the implications, I don't have a strong opinion on removing size from elements. Off the top of my head, I agree 'size' is not useful for presentation purposes; and CSS should be utilized. However, I beleive 'size' has useful functional purposes which affect browser behavior to ensure a max number of characters is not exceeded in a form, and therefore should be the same as the db field storing the submitted data.

That said, the problem #1 defined by the issue, that you cannot override the input size with css, the test code is incorrect. The tests incorrectly use max-width in an attempt to establish width. This is incorrect (and I agree CSS can be nightmarishly confusing in this regard). However, the correct way to override an element 'size' is to set the width on an element. You may ALSO add a max-width to ensure that element is contained within its parent container (such as a DOM parent or browser viewport).

correct test: http://jsfiddle.net/mdrmike/6gWA5/22/

supporting example: http://stackoverflow.com/questions/2125509/how-do-i-set-the-size-of-an-h...

I'd actually be into having a set width for these form elements. I hate when there a jagged form elements. Much prefer a fully justified form myself.

Anyways, adding need for UX review.

#14: size defines the visible width only, the maxlength attribute defines behavior (#maxlength in Drupal code). See e.g. http://www.w3schools.com/tags/att_input_size.asp.

This makes the installer look quite bad, so escalating to major.

@webchick - do you think it's worth just adding the CSS piece of this patch - core/modules/system/css/system.theme.css

[type="date"], [type="email"], [type="file"], [type="password"], [type="search"], [type="tel"], [type="text"], [type="url"] {
  min-width: 55%;
  max-width: 100%;
}

So that the installer doesn't look so ugly? That could be done in a separate issue if this one is going to take some time.

This issue seams to become huge. This requires change in many areas, like:

• Form API (Remove all #size options)
• Documentation
• CSS (fixed size overrides)
• JavaScript (size depended scripts)
• Hard coded HTML

I wanted to test patch from #2193271-12: Remove default #size attribute from core in DrupalDevDays in Szeged, applied it automatically with git but returned errors when I tried to apply it to latest HEAD. I will try to re-roll the patch following git re-roll instructions https://drupal.org/patch/reroll

Rerolled patch

Looks nice, here is screenshot.

I was initially confused by this patch because the issue title mentions removing the default size attribute only, but we're actually removing all #size declarations in core. This is going to end up being a really big patch because we're going to have to go through all the forms in core and write CSS against them to get them looking how they were before.

For example here's a before/after of the views edit page.

From my point of view it seems better to remove core form size attributes in a follow up.

This indeed looks like a massive undertaking, and I'm not sure how realistic it is.

I'd love a quick spin-off at this point to at least just fix the configuration page of the install screen. It continues to look really nasty, and it's literally like the first (ok, third) page people will see in Drupal 8.

We could put just

'#size' => NULL

in install page form elements, that would quick fix the bug.

I'm all for just removing the default attribute here, without modifying core's form structures. That should fix the install page.

@sun Can you update the title or the summary to make it clear what the intention of this issue is? Are we removing the default #size value or removing all #size values in core?

In suns absence I'm going to take the lead from the current issue summary. This patch should only remove the default size attribute. It shouldn't remove the size attributes that are specifically defined.

The patch for this issue should be a lot smaller.

Hm. I believe this would actually constitute an API change (suddenly certain form elements would look wider/thinner than normal without CSS correction in the theme in a minor version update), so should be pushed to 9.x instead.

This doesn't seem blocked on a usability review. Re-tagging.

I don't think this is an API change, even if we decided it is, we could probably add some core CSS to keep things looking the same in Stable.

We need to shorten the scope to what the issue title says and what @LewisNyman said

This patch should only remove the default size attribute. It shouldn't remove the size attributes that are specifically defined.

The patch for this issue should be a lot smaller.

Something to just take a next stab at with a patch.

working on the issue @bohemier

Working on this from DrupalNorth2016. Here's a patch that removes #size from the following form elements:

Search
Datetime
Email
Tel
Textfield
Url
Weight
File
MachineName
Password

Tested the install screen and there are small changes in the ui. See attached screens. Easily fixed in css by applying width: 100%

Rerolling... Modified the tests so that #maxlength is changed instead of #size

This needs a re-roll but mostly applies.

patching file core/lib/Drupal/Core/Render/Element/File.php
Hunk #1 FAILED at 30.

Re-rolled.

I'm looking at the patch right now (Vienna 2017 sprints).

Thanks @Jo, we are working on this at the core mentored sprint. The next steps are:

1. Review that we aren't missing any instances that may have snuck into core. (find all #size in core)
2. Manually test the change https://www.drupal.org/contributor-tasks/manual-testing

Looking for any missed instances of#size :)

Here at DrupalCon Vienna - I'm about to work on this issue.

edit: Part 2: Manually testing

Also tested it, input fields get smaller, but it's doesn't break any functionality.

Thank you for the screenshots, @al0a. Can you embed the images into the issue summary?

In order to test patch #50, I added the following fields to a basic page type:
Timestamp
Email
File
Tel
Text (plain)
Link (for URL)
and Number (integer) (fall-back for weight)

My feedback on the above is the following:
Timestamp: no size attribute
Email: size = 60
File: size = 22
Tel: no size attribute
Text (plain): size = 60
Link (for URL): no size attribute (same for the accompanying link text field)
and Number (integer) (fall-back for weight): no size attribute

It seems like accompanying css might be a necessity to this patch?

I'll follow up with demonstrations of the outstanding changes.

Screenshots in chrome:

As said before (#38), this patch "shouldn't remove the size attributes that are specifically defined". The underlying problem is, as said in #34, that the fields that were relying on the default '#size' value will be wider/thinner.

Should we set those specific fields with a default '#size' value or are we ok with the small visual changes in some cases?

... the fields that were relying on the default '#size' value will be wider/thinner ... Should we set those specific fields with a default '#size' value or are we ok with the small visual changes in some cases?

It probably depends on the fields. If there is a good reason to set a size attribute, we should add it. If there's not, the small visual changes are probably an improvement.

Ok so this seems to work and not break any fields. We can merge this and open a new issue if there are fields that need to be wider or narrower - that should be fixed in CSS / Theming.

There are some broken links to images in the issue summary.

FWIW, I'm ok with this patch introducing minor visual changes. Setting more specific widths where needed in a follow up issue is a good idea (URL field looks potentially a bit small for the external URL use case for example). Maybe we should open it already?

I'm wondering if we are worried about this introducing visual regressions in live sites? The minor visual changes to core themes look fine but maybe custom and contrib won't be so forgiving?

Visual disruption could potentially be allowed in minor releases, but this issue does seem extremely disruptive. On the other hand I also can't think of how we'd provide BC for this change that wasn't more problematic than the change itself, nor could I come up with how I would make this change in the continuous upgrade path.

Cottser's frontend BC notes say:

Markup coming from render arrays/JavaScript/etc. aka grey areas
Essentially treat this markup as if it's in Stable. Should likely only be changed for a bugfix if the disruption is worth it.

So, well, agreed on leaving it for the frontend framework managers to review as tagged in #63. For the bit that's within my purview: I don't think this is really a major unless theme or render system maintainers feel particularly that it should be. It was promoted in #19 for some visual regressions in the installer that should have been addressed (presumably long since) with more direct, less sledgehammer fixes.

I see the screenshots, but unless I'm missing something do we only have 'after' screenshots and not 'before'?

I spun up a core 8.5.x in simplytest.me and added the same fields as @rominronin did in #57 to take before screenshots. Added to issue summary.

I reduced the embedded images width to 50% and wrapped those images in links so we can click on them to view fully.

Thanks everyone for working on this.

Discussed with @Cottser and @effulgentsia. We're not actually convinced this change is necessary anymore, or at least not sufficiently so to justify the disruption, for a few reasons:

• The visual regression in the installer was fixed in a more direct way.
• The issue is very old and there's probably been improvements to browsers over the past 4 years.
• Most cases seem to actually work fine to allow CSS to specify the width. @effulgentsia tried the following test HTML:
  

  <!DOCTYPE html>
  <html>
  <head>
      <style>
          #i1, #i2, #i3 {max-width: 10px}
      </style>
  </head>
  <body>
  <input type="text" id="i1" size="20" />
  <input type="text" id="i2" size="30" />
  <input type="text" id="i3" />
  </body>
  </html>

  Per @effulgentsia, all three input elements were rendered as the same size.

• @sun's example seems to be using a max-width of 100%, which seems like a pretty specific case that doesn't justify the disruption. The other evidence in the summary like one person expressing an opinion on stackoverflow that we (including Cottser, a frontend and theme system maintainer) didn't know of as a best practice.

If we were to make this change, we'd need several things:

1. Review and signoff from a forms subsystem maintainer (as well as a frontend framework manager once RTBC again).
2. A stronger case in the IS for why we should make this change, and more current real-world examples or confirmation from theme developers as to why they need this change to be made.
3. BC for Stable. One way to accomplish this might be providing a form alter in Stable to alter the old weights back in.
4. Side-by-side screenshots taken at the same time, of all the elements. It's currently hard to compare them.
5. A more complete patch. The current patch leaves docs about the #size element in place about all these items (maybe it should since someone could still probably specify it on their individual form, but we should confirm that.)
6. Updated documentation indicating that the #size explaining to the developer why this change was made, so that they could follow the best practice in their own code.
7. A change record.

But before we jump down the rabbit hole of most of that work, let's have that subsystem maintainer signoff and that stronger case for making the change. Otherwise, we might want to mark this issue wontfix instead. Thanks!

Removing Novice tag from the issue as it's currently in limbo waiting on subsystem maintainer review.

Given that CSS can [now] fix the OP, I say that we "won't fix" this issue. Chasing browser bugs has never led us down a decent rabbit hole and we certainly have other bigger fish to fry instead of putting the required effort that will be needed to keep BC for this change.

I would still argue that the default size="60" parameter is bad.

It totally breaks flexbox: https://jsbin.com/sabefubagu/4/edit?html,css,output.

.form-item {
  display: flex;
}
.form-item label { /* Fixed width on labels */
  width: 200px;
  flex-shrink: 0;
}
.form-item input { /* Input takes rest of the available width. */
  width: 100%;
}

If the width of the container is too small for size="60" the input will be pushed outside.

Patch #50 can't be applied on 9.2.x.Needs reroll

Checking patch core/lib/Drupal/Core/Datetime/Element/Datetime.php...
Hunk #1 succeeded at 259 (offset -3 lines).
Hunk #2 succeeded at 291 (offset -3 lines).
Checking patch core/lib/Drupal/Core/Render/Element/Email.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 60,
      '#maxlength' => self::EMAIL_MAX_LENGTH,
      '#autocomplete_route_name' => FALSE,
      '#process' => [

error: patch failed: core/lib/Drupal/Core/Render/Element/Email.php:45
error: core/lib/Drupal/Core/Render/Element/Email.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/File.php...
Checking patch core/lib/Drupal/Core/Render/Element/MachineName.php...
Hunk #1 succeeded at 81 (offset 1 line).
Checking patch core/lib/Drupal/Core/Render/Element/Password.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 60,
      '#maxlength' => 128,
      '#process' => [
        [$class, 'processAjaxForm'],

error: patch failed: core/lib/Drupal/Core/Render/Element/Password.php:34
error: core/lib/Drupal/Core/Render/Element/Password.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/Search.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 60,
      '#maxlength' => 128,
      '#autocomplete_route_name' => FALSE,
      '#process' => [

error: patch failed: core/lib/Drupal/Core/Render/Element/Search.php:28
error: core/lib/Drupal/Core/Render/Element/Search.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/Tel.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 30,
      '#maxlength' => 128,
      '#autocomplete_route_name' => FALSE,
      '#process' => [

error: patch failed: core/lib/Drupal/Core/Render/Element/Tel.php:34
error: core/lib/Drupal/Core/Render/Element/Tel.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/Textfield.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 60,
      '#maxlength' => 128,
      '#autocomplete_route_name' => FALSE,
      '#process' => [

error: patch failed: core/lib/Drupal/Core/Render/Element/Textfield.php:49
error: core/lib/Drupal/Core/Render/Element/Textfield.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/Url.php...
error: while searching for:
    $class = get_class($this);
    return [
      '#input' => TRUE,
      '#size' => 60,
      '#maxlength' => 255,
      '#autocomplete_route_name' => FALSE,
      '#process' => [

error: patch failed: core/lib/Drupal/Core/Render/Element/Url.php:36
error: core/lib/Drupal/Core/Render/Element/Url.php: patch does not apply
Checking patch core/lib/Drupal/Core/Render/Element/Weight.php...
error: while searching for:
    // Otherwise, use a text field.
    else {
      $element['#type'] = 'number';
      // Use a field big enough to fit most weights.
      $element['#size'] = 10;
      $element += $element_info_manager->getInfo('number');
    }


error: patch failed: core/lib/Drupal/Core/Render/Element/Weight.php:65
error: core/lib/Drupal/Core/Render/Element/Weight.php: patch does not apply
Checking patch core/modules/system/tests/themes/test_theme/test_theme.theme...
Hunk #1 succeeded at 16 (offset -7 lines).
Checking patch core/tests/Drupal/KernelTests/Core/Render/ElementInfoIntegrationTest.php.

Added reroll for patch #50 ,Please check

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Attached patch against Drupal 10.1.x

Fixed CC issue

Was previous tagged for issue summary appears that still needs to happen also.