Field Groups marked as required are missing red asterisk

After applying patch #53, everything seems to work well, but when I click on a group to expand it, I see the element is focused with a green border as per usual, but I'm also seeing a duplicate red asterisk printing on top of the border. Is this by design or is it a bug?

#58 is not intentional. It's a bug that should be reasonably-easy-yet-satisfying to identify and address so that part can be categorized with the "novice" tag

This also needs tests as mentioned in #57 which wouldn't be novice for unless they'd already had experience writing Drupal tests.

Fixing attribution.

The duplicate red asterisk reported in #58 is not trivial to fix. This patch add styles for ".form-required::after" that conflicts with Claro's styles for ".claro-details__summary:focus::after" which adds a green border around the SUMMARY element. When the DETAILS element is required then Claro adds the "form-required" class to the SUMMARY element, then both styles are trying to style the same ":after" element.

The DETAILS element in Claro adds a "SPAN.required-mark" element inside of the SUMMARY element that adds the red asterisk when the DETAILS element is marked as required.
See "core/themes/claro/templates/details.html.twig":

  {%- if title -%}
    {%
      set summary_classes = [
        'claro-details__summary',
        required ? 'js-form-required',
        required ? 'form-required',
        accordion ? 'claro-details__summary--accordion',
        accordion_item ? 'claro-details__summary--accordion-item',
        element['#module_package_listing']  ? 'claro-details__summary--package-listing',

    ]
    %}
    <summary{{ summary_attributes.addClass(summary_classes) }}>
      {{- title -}}
      {%- if required -%}
        <span class="required-mark"></span>
      {%- endif -%}
    </summary>
  {%- endif -%}

Perhaps the style updates in this patch are not needed?

Not sure we can get away from not including the style updates but could something like this work instead? This would resolve conflict with .claro-details__summary and remove dupe asterisks. I can try and update the patch and test it out.

.form-item__label.form-required::after,
.fieldset__label.form-required::after,
:not(.claro-details__summary).form-required::after {
  display: inline-block;
  margin-right: 0.15em;
  margin-left: 0.15em;
  content: "*";
  color: #dc2323;
  font-size: 0.875rem;
}

@phernand42 thanks! that worked in my tests.

@phernand42 There may still be one caveat with that style. It assumes that the <span class="required-mark"></span> is present, so it will not work when a JavaScript #states condition sets the field as required (where the field is using a DETAILS element). In that case, the red asterisk would not show so not a major issue. This scenario feels like an edge case though, so I think that the suggestion in #65 is a good approach.

The patch is for 9.5 since I'm still on 9.5 (in the process of updating to 10 now though) but will try and cook one up for 10.1 later today.

@phernand42 Thanks! The patch applied cleanly to 10.1.x and 11.x. Attached are the patches for both.

Sweet! Thanks @recrit

Was tagged for tests #57 which are still needed. Also don't see an answer, unless I'm missing it somewhere, to the question @xjm has #57.

Also another question: We are removing a class from RenderElement -- seems like a frontend API BC break -- but then only updating the Claro theme. Has anyone tested in Olivero? Stark? Has anyone tested Seven and Bartik on D9? What about contrib themes?

+          if ($complete_form[$group]['#type'] === 'fieldset') {
+            unset($complete_form[$group]['#attributes']['class'][0]);
+          }

This code is really enigmatic and should probably be removed. What class is being unset? How do we know the first element actually is the class we're trying to unset? If this is needed then we should be looping over the ['class'] array and unsetting the specific item that's causing issues, but it would be better to resolve whatever issues are coming up in some alternative way.

I tracked this to #50:

Confirmed I am seeing the issues in #44.

Appeared the form-required class was being added to both the legend and the span.

Took a shot at trying to remove from the legend.

The issues in #44 refer to duplicated red asterisks which have persisted through relatively recent patches but seem to have been resolved by the latest ones.

I'll also point out that this issue has an MR open. We should be working from the MR, not posting patches.

Well, scratch that -- I'm having a lot of trouble getting the MR up-to-date even when following these instructions → so I'm just gonna post a patch.

If someone could just nuke the current MR and make a new one that would be very helpful -- I don't think we have enough changes going on here to merit trying to salvage an MR that's stuck on 9.2.x.

Anyway, this patch is identical to the above but removes the code to unset a class. I haven't tested this thoroughly but it seems to work when using horizontal tabs.

If there are any issues we should try to resolve them in CSS, or if the issue is the class we were previously trying to remove then we should:

- Document what CSS class is actually causing the issues
- See if we can resolve the issue further up the pipeline by avoiding adding that class in the first place (if CSS isn't an option)

Is it safe to assume that the problem class was .claro-details which is now wrapped in :not() in the CSS?

Rerolled the patch for 11.x

Doesn't this still need tests? We're no longer removing a CSS class but I believe we still need to check whether affected elements are getting the required class.

After reviewing some more placements, the removal of the class with the following code is still needed for FIELDSET elements.

if (isset($complete_form[$group]['#type']) && $complete_form[$group]['#type'] === 'fieldset') {
  unset($complete_form[$group]['#attributes']['class'][0]);
}

Patches 74 and 75 removed this piece of code.
Patch 69 still has the code to remove the class.
The class that it was removing is "form-required". See markup comparison below.

Without removal of the class:
The LEGEND has the "form-required" class.

<fieldset class="required-fields field-group-fieldset required fieldset js-form-item form-item js-form-wrapper form-wrapper" data-drupal-selector="edit-group-details" id="edit-group-details" required="required" aria-required="true" data-once="field-group-fieldset">
      <legend class="fieldset__legend fieldset__legend--visible form-required">
    <span class="fieldset__label js-form-required form-required">Details</span>
  </legend>
  
  <div class="fieldset__wrapper">
    ...
  </div>
</fieldset>

This results in:

Without removal of the class:
The LEGEND does NOT have the "form-required" class.

<fieldset class="field-group-fieldset required fieldset js-form-item form-item js-form-wrapper form-wrapper" data-drupal-selector="edit-group-details" id="edit-group-details" required="required" aria-required="true" data-once="field-group-fieldset">
      <legend class="fieldset__legend fieldset__legend--visible">
    <span class="fieldset__label js-form-required form-required">Details</span>
  </legend>
  
  <div class="fieldset__wrapper">
    ...
  </div>
</fieldset>

This results in:

The attached patches add the class removal back to the patch, but only removes the "required-fields" class.

So am I reading the code correctly that we are:

- Adding #required to field groups, which in this context is only for the purpose of ensuring it gets .form-required for the red asterisk (rather than validation considerations)
- Checking whether the field group we just made #required is a fieldset, in which case we remove .required-fields, even though it only had that class because we made it #required?
- Ensuring .form-required items get the red asterisk, unless it specifically also has .claro-details__summary

This also seems backwards to me. I think that we should ensure that we are only adding #required on items that actually need it, e.g. tabs, and ensure the PHP code is completely agnostic to the CSS. The current patch kinda seems to be taking two different approaches in PHP and CSS to tackling fundamentally the same problem (duplicate asterisks).

Also, am I the only one who noticed that the only way to test this is to install the Field Group contributed module? Is there no way to confirm the issue and test it just in Drupal core? I skimmed and didn't see a discussion on whether this is something that should be addressed in the contrib module or in core. Is this actually an issue with core that happens to be most visible when using a popular contrib module?

@caesius I agree that the RenderElement processing seems a bit backwards. This re-roll was simply keeping the intent of the original patch which is the only one that worked in this scenario.

- Checking whether the field group we just made #required is a fieldset, in which case we remove .required-fields, even though it only had that class because we made it #required?

This is not TRUE. The "required-fields" is already there, not because the patch sets "#required".

That said, I finally tracked down the "required-fields" class and it is being added by field_group contrib module's Drupal\field_group\FieldGroupFormatterBase when using "Mark group as required if it contains required fields". This class is then used by "field_group/formatters/fieldset/fieldset.js" to add the "form-required" class to the legend.

This part of the patch should be fixed in the "field_group/formatters/fieldset/fieldset.js" and "field_group/formatters/details/details.js". There could be an exclusion if there are any legend or summary child elements that contain a "form-required" class.

Summary of changes:

• KEEP - The CSS updates for ":not(.claro-details__summary).form-required::after". Example: The field_group module adds a "strong.form-required" to the vertical tab. Without this CSS update, there is no red asterisk displayed.
• REMOVE? RenderElement: if (isset($settings['options'])) {- This was added in #51 🐛 Field Groups marked as required are missing red asterisk Needs work . I do not get the error when I remove this change, so this might have been unrelated to the actual issue.
• KEEP - RenderElement: $complete_form[$group]['#required'] = TRUE; - This fixes the processing of the group element (details or fieldset) so that it is also marked as required and gets the correct "form-required" classes in the claro templates. Not sure if this is the best place for this fix though.
• REMOVE - The class removal in the RenderElement.
• PENDING - Create a new patch in the field_group to not added the "form-required" classes to the LEGEND of a FIELDSET or SUMMARY of DETAILS when the legend or summary already has a child with the "form-required" class. See "field_group/formatters/fieldset/fieldset.js" and "field_group/formatters/details/details.js"

Thoughts?

I think what we need more anything else is a way to test these changes in Drupal core without using the Field Group module at all, e.g. render array code that could be placed in a custom module or theme to render all the "types" of field groups that come with Drupal. Anything left over should be addressed in the Field Group module.

Also, using patch #74, I can avoid getting a "rogue" asterisk on a fieldset simply by unchecking "Mark as required" for it, which still allows the "required-ness" of fields it contains to percolate up to parent field groups.

So, for each of your suggesstions:

KEEP - The CSS updates for ":not(.claro-details__summary).form-required::after".

Yes, for now, assuming we don't find some better way of accomplishing this without excluding such a specific CSS class.

REMOVE? RenderElement: if (isset($settings['options'])) {

I agree that this needs to be removed, as unset() doesn't raise an error if a nonexistent array key is unset (I just tried it on PHP 8.1) and wrapping unset() in an apparently unnecessary if(unset()) isn't a code standard in Drupal core.

KEEP - RenderElement: $complete_form[$group]['#required'] = TRUE;

Again, we'll keep this for now if we don't find a better way.

REMOVE - The class removal in the RenderElement.

Definitely.

I faced the same issue in the date range field and the #75c patch works for me.

Closed as duplicate

We're using the Tabs field group and have "Mark group as required if it contains required fields" checked.

On the edit page there are now 2 red asterisks in the tab that has a required field. Not sure if that is expected behavior.

^ That is a known bug with the latest patch as noted in the IS "Remaining Tasks" section.

Actually, I'm removing the novice tag from this issue -- given the recent discussion, this is not currntly in a state where further screenshots or manual testing are helpful; none of the remaining tasks as listed in the issue summary are really novice when we're stuck in a cycle of trying to fix one bug without introducing a different one. Thanks!

I have re-rolled the patch from 79 against 10.3.x.

I'm seeing duplicate red asterisks on tabs after applying the above patch. There's a workaround I'll describe below.

This issue was definitely left in a sort of limbo, probably due to the need to find a way to test this solely in Drupal core and not rely on the Field Group module, but I think #74 was the previous "best fit" patch for this issue, rather than #79.

To recap, in #74 I removed a block of code that I found unnecessary for reasons I described in more detail in #80. #79 added the block of code back in due to a misunderstanding of where a rogue asterisk was coming from (it was Field Group's fault, not Core's), and I described a workaround for it in #82:

Also, using patch #74, I can avoid getting a "rogue" asterisk on a fieldset simply by unchecking "Mark as required" for it, which still allows the "required-ness" of fields it contains to percolate up to parent field groups.

So, attaching a 10.3.x reroll of #74. If you see duplicate red asterisks on tabs then you probably need to apply this workaround regardless of which patch you use. The previous patch does seem to work for specifically preventing "fieldsets" from duplicating asterisks, but I'm adamant that adding .required-fields to all field groups only to selectively remove them right after is the wrong approach.

This still needs tests and a new merge request for 11.x could be created...

luenemann → changed the visibility of the branch 3171835-field-groups-marked to hidden.

Changed version by mistake. Changing it back to 11.x.

I verified the issue with patches of #89 and #90 with 2 scenarios : Tabs vertical and Tabs horizontal. Both the patches fixes the issue for Tabs vertical but for Tabs horizontal, 2 asterisks are displayed. Please see the screenshots below:-

Vertical:

Horizontal:

I noticed that for vertical tab, template is rendered from core module and for horizontal tab template is rendered from contrib module field group that has <span class="required-mark"></span> with ::afterwhich results in 2 asterisks.

Vertical:

Horizontal:

Duplicate asterisks are a known issue, as is the fact that this bug seems to mainly if not exclusively manifest when using the Field Group module and we need a way to replicate the issue in core. These are noted in the issue summary in the Remaining Tasks section.

However, I believe this issue was originally created in and remained a core issue because there is at least some degree to which Claro's code does not make sense and makes it more difficult than necessary for Field Group to do its thing. The fact that this issue is four years old and has not been fixed within the Field Group module with its hundreds of thousands of installs says as much.* As does the involvement of frequent Drupal core contributors in this issue, none of whom seemed to question whether this should be fixed in Field Group -- including xjm in #56, who funnily enough moved the linked issue to the Field Group queue. (also funny how these issues keep getting tagged as "Novice" issues that'll be fixed at various Drupalcons...)

So what someone needs to do is actually look at what Field Group does against what Claro is doing and see which one is behaving incorrectly. If Claro is spitting out code that is needlessly hard for modules to work with, then it should be fixed in core. Otherwise the issue should be tossed over to Field Group and kept there.

*alternatively, everyone just forgot Field Group isn't actually a core module...

I tried to create 2 different patches: one for vertical tab (core module) other for horizontal tab (field group contrib module version 8.x-3.x). This is the solution I mentioned in my previous comment which may not be a correct method. I haven’t tested it with all the cases listed in #44 🐛 Field Groups marked as required are missing red asterisk Needs work but this works for Tabs. Attaching screenshots for reference.

Vertical:

Horizontal:

Patch didn't get attached in previous comment. Attaching it again.

The patch in #90 🐛 Field Groups marked as required are missing red asterisk Needs work works for me on Drupal 10.3. I had to apply the config workaround to remove the duplicate asterix, however there is a scenario that doesn't work.

In 1 of my fieldgroups I have a fieldset which contains a required field. In that case there was no duplicate asterix and I had to keep the `Mark group as required...` setting enabled.

So the result is I have 2 field groups with `` and 1 field group with `` which is visually noticeable.