Fieldsets style update

Needs final design specification

Updating design specs.

Hi @ckrina,

There are no fieldsets on the page /admin/config/system/site-information, those are details. The linked design seems to be a details design, not a fieldset.

Fieldset is used in the Date time widget, it wraps date and time inputs (and start-date - end-date):

Found a good example on admin/structure/block/manage/bartik_branding, has 4 various fieldset examples. I guess we can start from that.

@huzooka ok, removing the "details" word from here so it's just about fieldsets. We'll create another one for details. Just pointing here that the design is the same :)

Also removing the wrong path from the Test Pages and adding @pivica 's link.

Searched all definitions of fieldsets in Drupal Core, not too many of them actually, not sure is the plan to replace all of them with details form element?

Here is a list, i hope i didn't skipped something:

- /admin/structure/types/manage/article
  - Preview before submitting
  - Default options
  - Available menus
- /admin/structure/types/manage/article/fields/node.article.body/storage
  - Allowed number of values
- Link field configuration form will wrap all form elements when cardinality is 1 and title is disabled
- /admin/structure/types/manage/article/fields/node.article.field_image/storage
  - Upload destination
  - Allowed number of values
- /admin/structure/media/manage/audio (Enable media core module)
  - Media source configuration
  - Field mapping
  - Default options
- Core media library widget has one for widget form element
- /search/node?keys=claro
  - Keywords
  - Types
  - Languages
- Views blocks config forms can have it, defined in ViewsBlockBase class for views_label_fieldset fieldset element.
- admin/structure/views/add
  - View basic information
  - View settings
  - Page settings
  - Page display settings
  - Block settings
  - Block display settings
  - REST export settings

Find one more, actually, it's a different element fieldgroup (https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Element!Fi...). But it seems it is used only one one place in core - site configuration form for Drupal installer. Not sure about this one but i guess we do not need to cover it as a design element?

We figure it out that fieldgroup type is the only one that looks like regular fieldset (with a border, etc) and the fieldset which is not fieldgroup has form-composite which make fieldset look like regular form group (no border, etc).

This would mean that we should first implement a design for details element and then create a custom example which is using fieldgroup and then style it with a new design.

> This would mean that we should first implement a design for details element and then create a custom example which is using fieldgroup and then style it with a new design.

Not entirely correct, there is a fieldset default style which is used in some places and needs to be covered. Basically, we need to do at least fieldset which is always open style and all details variations.

Uploaded the first patch, still, a bit of stuff needs to be done including always open details variation. There are a bunch of @todo items in the code with open questions - need feedback on that stuff.

Here are some screenshots.

Fieldset example:

Details example:

One problem we have is with form fields - they are defining the top and bottom margin which does not plays well with parent wrappers which are defining inner padding. One solution for this is to apply an only bottom margin to form items and reset bottom margin for last form element - Bootstrap is doing similar, they are only defining a bottom margin to avoid similar problems.

If nobody picks this during the week i will probably work on this more second half of the week.

@pivica

The fieldset you show in #12 as example is a non-composite fieldgroup, like link widget, datetime widget, daterange widget etc.

Checkboxes and radios are composite fieldgroups (those fieldsets are grouping radios/checkboxes). These are themed like a standard form item.

Back to the topic: as a Drupal user, I'd like to distinguish non-composite fieldsets from details. Just because we can use details as a wrapper of form items, the're not the same.

1. Details markers aren't customized.
2. opened style does not follow Figma
3. Focus outline does not follow Figma
4. Padding does not follow Figma (at least summary's padding)
5. RTL!
6. CSS lint fails (yarn lint:css).

Test module here: https://github.com/zolhorvath/details

Missed the most important thing: This issue is not about details, so the patch should be moved to #3038784: Details style update

Hi All, great work towards modernizing the visual appearance of fieldsets! I'm looking at this from an accessibility and front-end dev perspective and I do not mean to discourage progress or be mean :)

I think there's a slight difference in what we're saying when we use the term "Fieldset" across design and development. I think it's important to understand these differences because we might have an easier time implementing this the way it looks (which is nice).

I think we all agree on what a Fieldset is, it's that typical style of a border that surrounds a group of form fields and usually has the Legend/ title sitting at the top and in between the border.

In development a Fieldset...

• Is meant to group related form fields together to convey their relationships in a semantic way (to technologies)
• The Legend is required and is meant to give context to the fields inside of the group
• The Legend is announced for when focusing on each field for screen readers
• Can have the position of the Legend decided by the browser and there may be little control to a developer

In this ticket and in reference to the design, a Fieldset...

• Has removed the typical border styles and implemented a cleaner interface
• Has implemented a pattern of showing and hiding the Fieldset
• Has moved the Legend to be the button that shows and hides the Fieldset

To make sure we can implement the Fieldset the way it has been designed and to make this a fun experience for everyone, I would like to suggest that we do the following:

• Not to change the development requirements as outlined underneath my comment "In development a Fieldset..."
• In development, and in JavaScript, implement a wrapping component that handles the showing and hiding of the Fieldset in a predictable way
• In development, visually-hide the Legend and in JavaScript, populate the button, that handles the showing and hiding of the FIeldset, with the title of the Legend

I fully support your ideas and expertise and hope this isn't too discouraging!

Edit: I meant to say that it looks like we might want to design a higher order component for the Hiding and Showing bit (if that's not already designed) - by that I mean, still design the Fieldset to be clean and beautiful but move the Hiding and Showing into another component. Then saying "We're designing the fieldset to be clean and beautiful, and it will be hidden and shown using the X-hide-show-thingy component."

Ready for the first review.

At the beginning I thought this will be pretty straightforward, but it turned out that fieldsets / legends implementation is quite different (for example the same thing that works on desktop Safari does not work on iOS Safari).

Screenshots attached.

No interdiff (the previous patches were for Details elements).

@anevins Thank you for your input! Patch from #12 was implementing details element, not fieldset element. We have a separate issue for styling details element #3038784: Details style update, which has been already committed. This issue is focusing on a simple fieldset element that is supposed to convey the semantics of a fieldset visually.

1. +++ b/css/src/components/form.css
   @@ -132,35 +135,73 @@ td > .form-item:only-child {
   +  /* The weird almost-nothing top padding is a workaround for Safari. */
   

   I read this as if we are adding some padding to the top to workaround a bug, but the padding is set to 0 here. I'm wondering if this comment is old, or we should rephrase this?

2. +++ b/css/src/components/views-ui.css
   --- a/templates/fieldset.html.twig
   +++ b/templates/fieldset.html.twig
   

   There's a todo in this file telling to remove this file after https://www.drupal.org/node/3010558 has been resolved. I guess that is not relevant since this template contains more changes than just a workaround for the bug?

3. I'm wondering if we could just remove the .fieldset--default variation and make it the real default. This would probably add some complexity to the styles because we would have to override the styles for .fieldset--group, but I think this would make the code easier to understand.

4. +++ b/claro.theme
   @@ -482,6 +482,19 @@ function claro_preprocess_fieldset(&$variables) {
   +  if (
   +    !empty($variables['attributes']['class'])
   +  ) {
   

   Nit: This should be on a single line.

5. +++ b/css/src/components/form.css
   @@ -22,13 +22,16 @@
     * When a table row has a single form item, prevent it from adding unnecessary
     * extra spacing. If it has multiple form items, allow spacing between them,
     * overriding Classy.
   + * Same goes for container-inline.
   

   We could modify the sentence to include this info. Something like:

   When a table row or a container-inline has a single form item, prevent it from adding unnecessary extra spacing. If it has multiple form items, allow spacing between them, overriding Classy.
   

6. +++ b/css/src/components/form.css
   @@ -132,35 +135,73 @@ td > .form-item:only-child {
   +  min-width: 0;
   

   Do we have to redefine this here? This element has min-width: 0 set with .fieldset selector.

Addressing #21.

I'll split fieldset styles into a separate css file.

Fieldset with an own CSS component file :)

Screenshots attached.

+++ b/claro.theme
@@ -427,6 +427,17 @@ function claro_preprocess_form_element(&$variables) {
+  // Remove 'container-inline' class from the main attributes and add a flag
+  // instead.

We probably should open an issue to add an explicit API for inline containers rather than using the container-inline class. After opening the issue, we should add a @todo to reference that.

Opened issue for this in core: #3059593: Provide API for inline containers. Other than that, I think this looks good.

Thanks @lauriii, I think this is fine.

Thank you everyone! Committed and pushed!