Field Groups marked as required are missing red asterisk

Created on 18 September 2020, over 4 years ago
Updated 16 January 2023, over 2 years ago

Problem/Motivation

When using the field group module, groups that contain required fields are not styled to have the red asterisk to denote that group has required fields. The form-required class is in the mark, but no styles for it.

Steps to reproduce

  1. Added fields to a node that are required
  2. Create fields groups
  3. Ensure the Mark group as required if it contains required fields. option is selected for a group
  4. Observe when creating content that the red asterisk is missing

Proposed resolution

Update the styles to account for this. The core/themes/claro/css/components/form.pcss.css has some styles already for the form-required class, but they are too specific. I would think making this style selector less specific would fix it, but there could create issues if the after pseudo element is intended for other use.

🐛 Bug report
Status

RTBC

Version

10.1

Component
Claro 

Last updated 1 day ago

Created by

🇺🇸United States a3hill

Live updates comments and jobs are added and updated live.
  • CSS

    It involves the content or handling of Cascading Style Sheets.

  • Needs screenshots

    The change alters the user interface, so before and after screenshots should be added to document the UI change. Make sure to capture the relevant region only. Use a tool such as Aviary on Windows or Skitch on Mac OS X.

  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

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.

  • Status changed to Needs work over 2 years ago
  • 🇦🇺Australia jaydee1818 Melbourne

    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?


  • 🇺🇸United States bnjmnm Ann Arbor, MI

    #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.

  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • 🇺🇸United States xjm

    Fixing attribution.

  • 🇺🇸United States recrit

    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?

  • 🇺🇸United States phernand42

    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;
    }
    
  • 🇺🇸United States recrit

    @phernand42 thanks! that worked in my tests.

  • 🇺🇸United States recrit

    @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.

  • 🇺🇸United States phernand42

    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.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.0 & MySQL 5.7
    last update over 1 year ago
    30,374 pass
  • 🇺🇸United States recrit

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

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update over 1 year ago
    Custom Commands Failed
  • last update over 1 year ago
    30,168 pass
  • 🇺🇸United States phernand42

    Sweet! Thanks @recrit

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    29,482 pass
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    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?

  • 🇺🇸United States caesius
    +          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.

  • 🇺🇸United States caesius

    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?

  • last update over 1 year ago
    Patch Failed to Apply
  • 🇮🇳India gauravvvv Delhi, India

    Rerolled the patch for 11.x

  • last update over 1 year ago
    30,360 pass
  • last update over 1 year ago
    29,643 pass
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States caesius

    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.

  • 🇺🇸United States recrit

    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:

  • 🇺🇸United States recrit

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

  • last update over 1 year ago
    30,384 pass
  • last update over 1 year ago
    29,653 pass
  • 🇺🇸United States caesius

    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?

  • 🇺🇸United States recrit

    @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 RTBC . 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?

  • 🇺🇸United States caesius

    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.

  • 🇮🇳India keshavv India

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

  • 🇺🇸United States smustgrave

    Closed as duplicate

  • 🇺🇸United States pmagunia Philadelphia 🇺🇸

    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.

  • 🇺🇸United States caesius

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

  • 🇺🇸United States anilu@ Houston, TX
  • 🇺🇸United States xjm

    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!

  • 🇺🇸United States gcb

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

  • 🇺🇸United States caesius

    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.

  • 🇩🇪Germany luenemann Südbaden, Germany

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

  • 🇩🇪Germany luenemann Südbaden, Germany

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

  • 🇮🇳India nayana_mvr

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

  • 🇮🇳India nayana_mvr

    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:

  • 🇺🇸United States caesius

    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...

  • 🇮🇳India nayana_mvr

    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 RTBC but this works for Tabs. Attaching screenshots for reference.

    Vertical:

    Horizontal:

  • 🇮🇳India nayana_mvr

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

  • 🇬🇧United Kingdom rick bergmann

    The patch in #90 🐛 Field Groups marked as required are missing red asterisk RTBC 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.

  • 🇧🇷Brazil carolpettirossi Campinas - SP

    I've tried applying patch #99 - 3171835-98-horizontal-tab.patch and it did not solve the issue for me.

    I have a required field in a fieldset that is on a horizontal tab:

  • 🇺🇸United States maskedjellybean Portland, OR

    Hey all, we have a fix for missing required asterisks in horizontal and vertical tabs in Claro in the works in field_group 4.x:

    https://www.drupal.org/project/field_group/issues/3160987 🐛 Form Required Class Missing from Claro RTBC

    It is really the same solution as you have in #99.

    Copying and pasting my comment from that issue:

    "I was also looking at https://www.drupal.org/project/drupal/issues/3171835 🐛 Field Groups marked as required are missing red asterisk RTBC which is marked as related. Looks to be trying to solve the same issue but in Claro/core. My thinking is that since Field Group provides horizontal tabs it should be responsible for making sure they work in core themes. Technically vertical tabs are part of core so I suppose we shouldn't really be fixing them, but since we can do it so easily I think we should.

    However I'm seeing that other types of field groups don't have a required asterisk either, like Details and Fieldsets. These are such basic HTML elements that it seems to me that core should be responsible for making sure they have correct styling. Even if we wanted to fix them in field_group, there isn't enough markup to do it. You can see that summary.form-required already has an ::after pseudo element, so we can't add the asterisk without breaking styling:

    "

    That said, I'm now realizing that .form-required is only added to <detail> because of field_group. In that case maybe all of this is the responsibility of field_group. But then again, it's not possible to style it without modifying a template provided by Claro...

  • 🇺🇸United States maskedjellybean Portland, OR

    I fixed missing required asterisks in <details> and <fieldset> field groups in Claro over on the field_group issue. See my comment: https://www.drupal.org/project/field_group/issues/3160987#comment-16119890 🐛 Form Required Class Missing from Claro RTBC

Production build 0.71.5 2024