Claro's preprocessing of field multiple value form's table header cell removes potential changes by others

Hey I tried that patch in #4 but the issue persists.

A little extra css fixes it for me:

.js .dropbutton-multiple .dropbutton {
  overflow: hidden;
}

.js .dropbutton-multiple.open .dropbutton {
  overflow: visible;
}

.js .dropbutton-multiple .dropbutton .button{
  margin: inherit;
  width: 100%;
  text-align: left;
  padding-left: 0.3em;
  padding-right: 60px;
}

Edit: this is for a different issue.

Yes, you are right @hctom. The issue I am seeing is that dropbutton multiple is visually broken on Claro. I will create a new issue and a patch.

I have applied the patch from #4, but when testing this with the Paragraphs module it showed a new bug: the title was duplicated. This is because Claro expects there to be a field_label, while the paragraphs module actually moves that key to another place. This means that the title is now both in the place that Paragraphs moves it to, and in the re-definition done by Claro.

I think a better approach is moving the repeated logic from Claro to core. This means that the improvements in structure already built in Claro (so it's easier to extend attributes in a table header) are implemented in core, and Claro only has to extend the attributes.

This doesn't involve a new (nested) variable structure, which should limit the adjustments needed by contrib modules (if any).

Patch is attached.

Oops, created the patch on the core subtree split instead of the drupal git. This is the same patch with a different root.

1. Code in #9 looks solid, and works as expected.
2. There are no functional regressions visible when using the Seven theme, which is great.
   

   Before:

   

   After:

   

   

3. However the layout in Claro doesn't look so swell, when someone (eg paragraphs) adds more items to the header.
   

   Before:

   

   After:

   

   

   The reason is that Classy theme has a CSS rule, that affects the layout of the <h4 class="label"> inside the table header:

   // Classy's form.css file
   .form-composite > legend, .label {
     display: inline;
      ...
   }
   

   But Claro uses its own .form-item__label class on the h4 instead of the "label" class. And the analogous CSS snippet for the display is different:

   // Claro's form.css file
   .form-item__label {
     display: table;
     ...
   }
   

   What's more is that Paragraphs module is overriding that class and setting it back to label, however this has no effect because even though Claro's base theme is Classy, it overrides and disables the form.css from classy/base library.

In conclusion, I think that making any additional modifications regarding point 3 is out of scope of this issue. The visual aberrations pointed out in 3 only happen with contrib module. Core by itself is unaffected. Possibly, yes, a follow-up core issue could be filed to restore the expected inline display style for form labels, but an issue like that might not make it anywhere quickly. Instead, I think the responsibility of fixing the table header layout issues lies with the Paragraphs module in a patch on the related contrib issue #3099024: Paragraphs actions button are removed when Claro admin theme is used.

Thanks @hctom for your work identifying the original issue from Paragraphs, for filing the core issue with patch!, and @bartlangelaan for improving the patch.

RTBC.

Seems like unrelated random failure

I discussed with @alexpott about backporting this to 8.8.x. He recommended not backporting this to 8.8.x because this imposes some risk on contrib modules. I did some research in the contrib environment and it seems like there's another module that is making similar alterations with Claro. Committing this wouldn't directly break the module, but it will remain incompatible with Claro until they've made some changes.

I made some minor documentation improvements to the patch.

+++ b/core/themes/claro/claro.theme
@@ -797,26 +797,15 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
-    $header_attributes = ['class' => ['form-item__label', 'form-item__label--multiple-value-form']];
...
+    $variables['table']['#header'][0]['data']['#attributes']['class'][] = 'form-item__label';

Now that we're not overriding the attributes, the label class added by template_preprocess_field_multiple_value_form function is now rendered in Claro. I think we should remove that to make sure this doesn't have any impact to what is rendered in the UI.

I saw this issue initially through use of paragraphs - some of the head actions had been incorrectly removed.

Patch #13 works well to rectify.

Patch #13 works for me, button Edit All / Collapse All is now visible

Addressed #13.

Works for me on 8.9.x

I found two modules with evidence of current usage that have a preprocess_field_multiple_value_form that effectively undoes the change this patch makes to theme.inc

https://www.drupal.org/project/paragraphs_sets | usage: http://grep.xnddx.ru/node/30426268
https://www.drupal.org/project/require_on_publish | usage http://grep.xnddx.ru/node/30435042

It would be good to either

• add issues to those module's queue informing them of the change to core and that it will only be apparent in Claro
• provide a fallback that performs a find/replace on #prefix
• An explanation as to why it's acceptable to not have the class changes in claro_preprocess_field_multiple_value_form apply in instances where those modules are installed.

I believe the patch is good to go other than that.

Re #22:

As we are changing the structure here and contrib modules like paragraphs or the ones stated in #21 definitely have to react on those changes, why don't we group the new render array structure under a dedicated array key as I did in my first path in #4

#8 points out that this results in duplicating the title in his setup. This would also happen with the modules mentioned in #21. It is true these module undo the fix in #19, they would also undo the fix with patch #4 AND the title would be duplicated.

What modules in #21 are doing this is completely unnecessary and inefficient since they duplicate (literally pasted) steps from template_preprocess_field_multiple_value_form(). It's not an ideal approach, but these modules are have enough usage to justify alerting them of this change (particularly paragraphs_sets, which is already D9 compatible).

Just to say that for those on D8.9.x the patch from comment #9 is still working very nicely for me, today 12 Sep 2020.

In fact the patch works great in combo with another paragraph/Claro D8.9.x patch at comment #89 in this issue: https://www.drupal.org/project/drupal/issues/3092181

Together these 2 patches solve all my Claro+paragraphs troubles in D8.9.x !

Also here to confirm the #9 patch still applies to 8.9.11 and fixes the missing paragraphs dropdown for me which is needed for the drag and drop functionality.

Re #24

Of course this will have duplicate titles here and there, but it is an easy fix for the contrib modules and that's what change records are for: To inform about changes that may need work on the contrib/custom side.

I've seen committers reject patches with BC-breaking changes far less severe than what could happen here, it's unlikely the currently solution would be accepted into core. It doesn't really meet the "We will take reasonable efforts to not break code that developers may be relying on." part of Drupal's Backwards Compatibility Policy, particularity since it's a solution for a Claro-specific problem that impacts all sites regardless of the themes in use. I think there are untapped "reasonable efforts" that can provide a backwards compatible version of this solution.

I have some suggested alternate approaches. None of them are ideal, but preferable to a BC break, and 2/3 of them are specific to Claro - the only theme that requires this change. Someone may be able to figure out a way to do one of these more cleanly.

• The ideal approach is to provide a backwards-compatible layer so sites relying on the previous approach are not disrupted, but are presented with a deprecation notice that the old implementation will be removed in Drupal 10. I'm not sure this is possible with a preprocess function.
• +++ b/core/themes/claro/claro.theme
  @@ -797,26 +797,15 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
  + $variables['table']['#header'][0]['data']['#attributes']['class'][] = 'form-item__label';
  + $variables['table']['#header'][0]['data']['#attributes']['class'][] = 'form-item__label--multiple-value-form';
  

  This problem is happening because Claro is adding/removing few presentation-changing classes from the header of a multivalue form. This class addition/removal logic could be removed from claro_preprocess_field_multiple_value_form altogether, and the styles could be applied via different selectors. If it deviates from Claro's conventions a comment could explain why this is preferable to a BC break.

• It may seem overkill, but Symfony's DomCrawler could parse the #prefix similar to JS and add/remove the classes as needed. Similar to the previous option, this would need a comment explaining why this seemingly excessive approach was needed to protect backwards compatibility.

I agree that this should have been implemented differently in the first place, and it's not pleasant to support BC for a non-ideal implementation. However, it's hard to justify BC breaks unless it's clear all options have been exhausted, and I think some options (however odd) still exist.

This is probably not the right solution, but it works in our situation. I've used the patch from #17 and added a few lines of css to make it work with the paragraph module.

#32 patch worked for me as well (with the specific goal of getting the paragraphs actions links to appear)

Hi @timohuisman,

Thanks for the patch. I can confirm #32 works as expected. I was wondering, if a paragraph has a sub-paragraph if that should be expended as well.

wrong issue, removed my comment

Patch at #32 worked for me to solve problems with Paragraphs Table module

Hi, Patch #32 worked for me over core 9.4.7.

Added reroll of patch #32 on Drupal 10.1.x.

Try to fix yarn run errors.

+1 for RTBC on patch #42

Patch #42 works great for me, thanks!

I agree with the comment in #10 that the height of the table header should be shorter. The H4 on the left and the buttons/options on the right do not need to be stacked vertically. One possible solution with the current markup (at least on a Paragraph field) would be .field-multiple-table thead label { display: inline-block; }.

Patch #42 works great for me, also (tested 9.4.x). Thank you very much.

Moving to RTBC due to the positive feedback.

1. +++ b/core/themes/claro/claro.theme
   @@ -908,27 +908,13 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
   +    if (isset($variables['table']['#header'][0]['data']['#attributes']) && $variables['table']['#header'][0]['data']['#attributes'] instanceof Attribute) {
   

   We could technically add support for array type of attributes too if that increases the chances of this working

2. +++ b/core/themes/claro/css/components/form.pcss.css
   @@ -37,9 +37,12 @@ tr .form-item,
   +.field-multiple-table .field-label h4.label {
   

   Can we add comment explaining that this is to try to keep the styles working even when there are modules overriding the markup

3. A small CR that warns about the render array changes would probably be useful since we know that there are some themes that would be impacted by this.

Patch updated per #47. Creating CR shortly.

Note that I also tried to make the CSS a bit more resilient, but in doing so, it negatively impacts Paragraph's CSS. So, IMO the current CSS is correct because of BC.

The Needs Review Queue Bot tested this issue. It 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.

CR created at https://www.drupal.org/node/3344342. Please double check this, I'm not entirely positive that what I wrote is 100% correct.

Could the Issue summary be updated some? The proposed solution mentions there needs to be discussion. If a decision has already been made can it be documented there please.

Thanks!

Agree. Adding the "Needs issue summary update" tag.

Note that since the patch has been created and still needs review, I'm setting this back to "Needs Review". Please see the documentation at https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

"Needs work" is only for:

There is a patch attached to the issue, but the patch needs additional work before it should be reviewed. The author of the patch may indicate that it is incomplete, or the patch has gone through the reviewing process and has received feedback about areas where it needs improvement.

Will leave in review but if the proposed solution isn't clear any reviewer can't review the patch to see if the approach being proposed is being followed.

Since it's been about a week moving back to NW as the issue summary does need to be updated.

Don't want it to rot away in the review queue.

Actually turned into a great improvement for paragraphs

Tested by installing paragraphs and making a dummy bundle with just a simple boolean field

Without the patch. I can't drag and drop the header

With the patch I can!

+++ b/core/themes/claro/claro.theme
@@ -993,27 +993,13 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
+    if (isset($variables['table']['#header'][0]['data']['#attributes']) && $variables['table']['#header'][0]['data']['#attributes']) {

Isn't the isset check here sufficient?

What I was thinking of doing here originally was something like:

// Convert #attributes to \Drupal\Core\Template\Attribute object to allow
// using its utilities for modifying the attributes.
if (!($variables['table']['#header'][0]['data']['#attributes'] instanceof Attribute)) {
  $variables['table']['#header'][0]['data']['#attributes'] = new Attribute($variables['table']['#header'][0]['data']['#attributes']);
}

However, I'm not certain we need to because it looks like we have other instances where we are already assuming that #attributes are instances of \Drupal\Core\Template\Attribute. Because of this, it's probably fine to just remove the second second condition from here.

Removed the second condition per #58. Patch interdiff attached. Thanks!

Attached is the same patch as above but for 9.5.x.

IMO this bug is a major usability issue when paired with the popular Paragraphs module (which is why I'm working on this), and should be ported to 9.5.x.

Get the same results still using #59 so marking it again.

And agree with the comment made in #60 that this makes paragraphs better as it shows a feature that I actually didn't know existed.

We are allowed to change the render arrays in minor releases based on https://www.drupal.org/about/core/policies/core-change-policies/frontend... so we don't have to implement a BC layer for this. However, we cannot backport this to 9.5.x because of the potential BC concerns.

Committed 90897af and pushed to 10.1.x. Thanks!