Form required class missing from horizontal field groups in Claro

Had to use field_group to test this but I was able to confirm.

Forgot the pcss file.

The patch was applied and works fine.

Steps followed.
1 - Install field group
2 - Go to any content type manage form display
3 - Click on Add field group and select Tabs item and select horizontal or vertical tabs.
4 - Click again on Add field group and add one tab for each tab you want.
5 - Drag the specific fields to the respective tab created.
6 - Put all tabs inside the tabs container
7 - For each tab item on the widget select choose the tab widget
8 - To test the opposite orientation of the tabs go to manage form display. Click on the cogs icon on the tabs container item, and change the direction to vertical or horizontal value.

Screenshots are attached for evidence.
Note, the last tab doesn't have the required fields and the * does not appear as expected.

It seems like the solution is using CSS classes that don't exist in core, and are used by contrib modules instead. We usually don't write CSS in the core themes to explicitly accommodate contrib modules. The solution should either be generic enough for it to not include CSS classes not used by core, or it should be moved to the contrib module queue.

Vertical tabs appear to be valid in claro but horizontal tabs are not so those would need to be replaced.

Tagging for novice as it should be easy to find the replacement.

Also this could use an issue summary update with more recent screenshots of before/after.

I am looking into it

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because @smustgrave's comment still applies.

Novice issue reserved for the Mentored Contribution during the DrupalCon Portland 2024 contribution day. After the 2024.05.08, this issue returns to being open to all. Thanks

Hello, I am at Mentored Contribution DrupalCon Portland2024, I will be working on it during the next hour or two.

Hi, I am at the mentor contribution at Drupalcon24 Portland. I will be working on this one for the next couple of hours.

Hi I'm novice at DrupalCon Portland2024, I'm part of table looking into this one for next hour.

Working on this at DrupalCon 2024 for the next hour

Hi there, working on this at DrupalCon 2024 for the next hour or so.

Hi I'm novice at DrupalCon Portland2024, I'm part of table looking into this one for next hour.

Hello!
Portland DrupalCon 2024 – working on this for the next 2-ish hours.

Verified * marking tab groups that contain mandatory fields is missing form tab Field Groups when using Claro admin theme at Contribution Workshop at DrupalCon Portland 2024 (with help from Mentor Josh Miller).

Commenting that I helped a mentored contribution table look into this issue. We were able to confirm the issue and we looked at the patches. Unfortunately this was happening later in the day and I think we all ran out of gas at the end. I'll review this in a few days if no one else from the group does and try to get it to RTBC so we might get some core credit for everyone's efforts.

checked with "drupal/field_group": "3.x-dev@dev" with 11.0-dev

before vertical fix

After vertical fix:

After horizontal fix :

Fixed with this patch claro_field_group_required_patch.diff

#34 is not valid fix for cor -- it's adding a new entry to claro.info.yml.

Additionally, as @lauriii alluded to in #13, it's usually contrib's responsibility to work nicely with core themes, rather than the other way around. So, I'm moving this to the Field Group queue.

Thanks everyone!

The group is working on this issue at Drupalcon Barcelona 2024

I tried the patch with D9.5.11. Patch applies but does not work.

Also, as mention above, this is more likely an issue for FG module than for core (unless there is a reason core is blocking this fromworking; but there wouldnt be anything in the fix that would have "field_group" in it.

I was facing this issue and patch #10 solved it.
This issue seems related to another one: https://www.drupal.org/project/drupal/issues/3171835
I'm adding to the Relationships to help users found this solution.

I've identified the issue (or hacked the module into making this work?) I have no good knowledge of CSS but the point is something is missing in the module's CSS files. I compared it with claro's form.css component and copy-pasted the CSS. I guess it needs tests on several other admin themes (gin etc) but I believe we now know what's going on.

Work is done on MR 108

I'm also uploading screenshots of before-after on my project

@dimitriskr I'm looking at this today too! Thanks for the MR! It resolves the issue for me but I have a couple ideas for improvements.

I know the issue title says horizontal tabs, but vertical tabs in Claro are also missing asterisks. If we could solve for both that would be ideal.

I may create another branch in the fork that creates a formatters/tabs.css which contains the fix for both horizontal and vertical tabs in Claro:

/* Fix missing required asterisks in Claro */
.horizontal-tabs .form-required::after,
.vertical-tabs__menu .form-required::after {
  display: inline-block;
  margin-inline: 0.15em;
  content: "*";
  color: var(--color-maximumred, #dc2323);
  font-size: 0.875rem;
}

This is slightly different from your CSS. I don't think the selectors need to be quite so specific because they will be harder for someone to override if necessary and more prone to breaking if field_group changes the markup of tabs slightly. Also we can set #dc2323 as the fallback color to be used in the case where the --color-maximumred variable is unset (which I assume would be all themes except Claro).

Creating a formatters/tabs.css makes sense to me because formatters/tabs.js is what adds the .form-required class to the tabs. So tabs.css could be thought of as the associated styling.

I do wonder whether we risk messing up the styling for asterisks in other themes. Should we be taking this a step further and only loading our Claro fix if the base theme is Claro? This is such a weird situation.

I agree creating a formatters/tabs.css to fix this on vertical tabs too. You can use the branch I'm working on, no need to have multiple branches and MRs

As for interfering with other themes, only testing can prove that. I see in the body tag, data-once="" has a claroDetails, so we could limit it like this. Again, I am not familiar with front-end stuff in Drupal, so I'm just throwing random ideas :D Adding a related issue I saw about this

Oh, good call on html[data-once~="claroDetails"]. Looks like that will work!

Thanks for the go ahead. I'll modify the same branch today.

I don't use Gin so I'm not cut out to fix the issue you linked but maybe it does require a similar solution.

I was also looking at https://www.drupal.org/project/drupal/issues/3171835 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:

I'll make a comment on the core issue after I modify the MR.

I came around to the conclusion that ensuring other types of field groups get the required asterisk is the responsibility of Field Group since Field Group adds the .form-required class in the first place. How should Claro know about a class that Field Group added? So I came up with a fix for missing required asterisks in Details and Details Sidebar field groups.

It wasn't possible to do using only CSS because of the markup like I showed in #45. Additionally I found that when I looked at details.html.twig provided by Claro, the template can optionally insert a span meant for adding the asterisk. It's beyond me to say why the template is not doing this, but my fix inserts it via JS if it wasn't added by the template. When the span exists it looks like this:

Unrelated to Claro I also fixed a Tab field group inside of a Details field group not getting the required asterisk. Tab field group is technically a <details> element. There's no form validation when editing form display to prevent someone from putting a Tab inside a Details, so we should make sure it gets marked as required if necessary.

Also pushed a fix for missing required asterisk on <fieldset> in Claro.

Looks like the MR108 partially solves the issue.

It is still missing the asterisk when there's an address field that is required

I can confirm that the MR108 works as desired on D10.3.14

The "form-required" is been added by the tabs.js#L34 already present in the latest release https://git.drupalcode.org/project/field_group/-/blob/4.0.0/formatters/t...

#carolpettirossi on which drupal core version did you do the test?

What I did notice was that when you have a paragraph (required) field in your details tab and you completely removed it the asteriks marker was still visible. This because the logic in the details.js isn't executed on a jax call by the once implementation.

I provided a new commit on the MR108 that handle this as well.

Patch from the PR 108 with the once change

Confirmed the patch in #53 still provides required asterisks for horizontal tab field groups in Claro as expected. Thanks for the once improvement!

Rebased MR !108 onto current 4.x (4.1.0-alpha4).

Changes from the rebase:

• Dropped the fieldset.js hunk from "Fix missing required asterisk on fieldset in Claro" - 4.x already contains the equivalent fix ($('.fieldset__label', $this).addClass('form-required') landed before alpha4), which is what caused the conflict.
• All other commits applied cleanly; no functional changes beyond the rebase.

Going to test this further