Margin on form-items in table rows (Classy)

A first guess would be that in case there is a single field-item inside a td it should get its spaces from the table elements (td) and not their content.

What I see is that this comes in drupal since 4.7 so I don't know if it's a good idea to change it just because there is a case of a module where this goes a bit wrong.

I would like though to have an opinion from the Classy maintainers as they sure know more than we do :)

Also a problem in Inline Entity Form. Our fields look terrible because of the low spacing. Would be great not to need custom CSS.

The drag handle is positioned and all other elements just kind of float around it. Almost like it's expected to be in its own td, however inline forms are not rendering it that way.

Linking to solution that bojanz committed to Drupal Commerce 2.x: https://github.com/drupalcommerce/commerce/commit/e52fbfe03e305421905243...

In short:

-        $this->t('Value'),
 +        ['data' => $this->t('Value'), 'colspan' => 2],

+      // The tabledrag element is always added to the first cell in the row,
 +      // so we add an empty cell to guide it there, for better styling.
        $value_form['#attributes']['class'][] = 'draggable';
 +      $value_form['tabledrag'] = [
 +        '#markup' => '',
 +      ];

Needed to manually make tabledrag a td manually with an empty value.

@mglaman
This issue is about the element margins (vertical spacing between two elements), which is unaffected by #6.
The extra column trick is just for avoiding reflow problems around the tabledrag element.

Can someone add a couple screenshots highlighting what the problem is before digging in?

Based on what the issue summary says, we should be able to add some form elements to a table and look at it directly in Classy, right? That should reveal the problem directly.

Hi #jonathanjfshaw can you provide screenshots with this bug? And where I can check this bug? Thanks.

Investigated this a bit and manage to track original CSS addition in file /misc/drupal.css in commit http://cgit.drupalcode.org/drupal/commit/?id=ad5e3ebb75df5c861251ba3ed3d... which happened in 2004, yep long time ago:

+tr.light .form-item, tr.dark .form-item {
+  margin-top: 0em;
+  margin-bottom: 0em;
+  white-space: nowrap;
+}

This code didn't change a lot over the year, got moved a couple of time and now it is in /core/themes/classy/css/components/form.css:

tr.odd .form-item,
tr.even .form-item {
  margin-top: 0;
  margin-bottom: 0;
}

This margin reset rules are useful for tables with single table row form elements, for example, pages like

admin/modules
admin/config/regional/language
...

So a use case for this is that we need to have .form-item in table and table rows needs to have .odd or .even classes. And then we apply this margin reset rules so we save up some vertical space.

The problem is that these reset rules are too general and they will be applied for other cases like for Inline Entity Forms or Paragraphs. In this cases, we don't want this.

We could easily fix this if we assume that this rules should be active only in the case when we have one form-item inside of td, and this was probably the case when these rules were originally added:

tr.odd td > .form-item:only-child,
tr.even td > .form-item:only-child {
  margin-top: 0;
  margin-bottom: 0;
}

Use cases where margin reset should be active should still work with this but the rule will not be active for table cell which has multiple elements inside of table cells.

Here are the screenshots before and after the patch applied:

Browser support for :only-child pseudo selector is good https://caniuse.com/#feat=css-sel3, including IE9.

Patch from #18 works well. Thank you!

I've re-run the tests and they all passed for the 8.5 branch. Setting back to Needs Review.

Agree #18 also works for me and makes the UX for my editors way better :)

Our team noticed this issue when using fields with unlimited instances, and a single link field, within a Paragraphs container (similar to the screenshot in #10), and the patch in #18 resolves this.

In testing, we created pages, blocks, forms and content using paragraphs, using various types of fields, and do not see any problems anywhere. On forms using Paragraphs this patch equalizes the spacing between fields, as well as anywhere table rows contain elements with the "form-item" class, such as a contact form containing a link field. We noted no regressions.

Before (below)

After (below)

Themes tested:
• tested with Seven (subtheme of Classy)
• tested with Bratik (subtheme of Classy)
• tested with Adminimal (subtheme of Classy)

Browsers tested:
• Chrome 65.0.3325
• IE 11
• MS Edge 14, 15, 16
• FireFox 52.0
• Safari 11.0.3

Drupal versions tested:
8.5
8.6.x-dev

This very selective minimal patch (margin-on-form-items-in-table-rows-2675464.patch) does seem to work as intended. We think it would be a worthwhile improvement to backend UX where the admin theme is Seven, Bartik or Adminimal theme, or any other theme with Classy as the parent theme.

Based on #21, #22, and #23, and our own testing, I'm marking this as RTBC.

I didn't test this personally, but regarding the change itself, it is pretty small and I would classify as a minor bug fix. It shouldn't adversely affect any subthemes, so I'm ok to make the change.

Thanks @pivica for pointing out on #18 what is the use case for having those rules set. Could we document that in the CSS as well so that when this is being looked at next time, it would be faster to get the idea why it is there?

CSS inline comment added as per suggestion by @lauriii, along with interdiff from #18.

Updated comment in CSS for the patch to include the "why" as requested by @jonathanshaw.

Updating patch against latest 8.6.x commits. This includes updated comment in CSS for the patch to include the "why" as requested by @jonathanshaw.

Patch in #33 has my RTBC -- tested in multiple browsers & with nested fields.

I'll defer to the maintainers of Classy for the official issue status change.

You don't need my permission to change it to RTBC. If you reviewed it and it meets the criteria, feel free.

+++ b/core/themes/classy/css/components/form.css
@@ -32,8 +32,13 @@ form .field-add-more-submit {
+ * Limit scope of margin reset rule to only-child to allow form items inside
+ * table elements to have a margin top and bottom.

The question this should answer to is: why are we modifying the .form-item elements when it is the only child directly under an odd even colored table column. For me the rule seems very strict so there must be a specific reason why it exists, and that's exactly what we want to document here.

Updated with more descriptive comment per jonathanshaw's suggestion, which seems correct to me.

Screenshots were provided two years ago in #18, and are still applicable. Removing the "Needs Screenshots" Issue tag.

Thanks @twfahey! In the future you can also post an interdiff so we can easily see the changes you made.

The visual fix itself looks right but as @lauriii said this is an unusually specific selector. If no one knows why we do this (as #39 suggests) then maybe we should not be doing it. :) What would admin/modules and admin/config/regional/language look like without the rule? Should those tables have their own CSS for this styling, rather than the general rule that is causing us problems here? Etc.

Leaving at RTBC, but tagging for @lauriii's review again. Thank you!

It seems like the original CSS rule was introduced in Drupal 4.4.0 🐘I can't find any reason why this should be only applied to tables with odd even classes (rather than all tables). For example, it seems content list styles are broken since it doesn't have odd even classes.

For that reason something like this would probably make sense:

td > .form-item:only-child {
  margin-top: 0;
  margin-bottom: 0;
}

I'm sorry to come up with this feedback this late, but after my research, I started feeling making this change in Classy is too risky since this doesn't only affect core themes but all themes built on top of Classy. I'd like to suggest that we try to first come up with a proper fix in Seven, and once we are confident it works, try to find a way to move it to Classy. Currently, it is difficult to make this type of risky bug fixes to Classy, see #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility.

We have patch and solution here:
https://www.drupal.org/project/drupal/issues/2982097

Since #2982097: Margin on form-items in table rows (Seven) was fixed long time ago I am updating the issue status here.

Rerolled patch.

This will need a reroll for D10.

This was discussed at a bugsmash group triage, with lendude and myself. lendude pointed out that that this breaks for form elements inside a table which isn't common in a front end theme. He looked at his own projects and did not find any custom CSS for this, suggesting that it might be fixed in Claro. Further, Umami only needs to support what it is needs for demonstration so it does not need a fix. That all seems reasonable so we have agreed to move this is contrib Classy.