Visitar URL original
USWDS - Fieldset: Resolve legend margin error by mahoneycm · Pull Request #5344 · uswds/uswds · GitHub
Skip to content

USWDS - Fieldset: Resolve legend margin error - #5344

Closed
mahoneycm wants to merge 6 commits into
developfrom
cm-fieldset-legend-margin
Closed

mahoneycm wants to merge 6 commits into
developfrom
cm-fieldset-legend-margin

Conversation

@mahoneycm

@mahoneycm mahoneycm commented Jun 21, 2023 •

Copy link
Copy Markdown
Contributor

Summary

Resolved usa-fieldset & usa-legend margin conflict. usa-fieldset now appropriately respects the margin set by child legend elements.

Breaking change

This is potentially a breaking change.

Warning
In Chrome and Safari, anywhere there is usa-legend nested beneath usa-fieldset, a larger margin is rendered than before. This comes from the margin set by usa-legend which was hidden by this bug.

Related issue

Closes #4153

Related pull requests

Form change log item →

Preview link

Checkbox →
Mailing Address →
Create Account Page →

Problem statement

fieldset does not respect margins set by usa-legend or usa-legend--large.

Solution

Add display: contents to usa-fieldset styles.

display: contents simply removes their principal box, and their contents render as normal. 1

Testing and review

  1. Inspect components which use usa-fieldset and usa-legend. Ex:
  2. Confirm the margin set by usa-legend or usa-legend--large is respected and visable.

It may be helpful to duplicate the fieldset element in the preview and make sure there is a visible margin between them

Testing Checklist

  • Confirm margin is respected
  • Removing display: contents from usa-fieldset causes margin to be disregarded
  • Confirm there is no unintended visual regression in components using usa-fieldset

Additional content

Screenshots from issue

Current Behavior:
Screen Shot 2021-04-14 at 1 47 24 PM
Expected Behavior:
Screen Shot 2021-04-14 at 2 24 45 PM


Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm run prettier:sass to format any Sass updates.
  • Run npm test and confirm that all tests pass.

Footnotes

  1. https://drafts.csswg.org/css-display/#unbox:~:text=display%3A%20contents%20simply%20removes%20their%20principal%20box%2C%20and%20their%20contents%20render%20as%20normal. ↩

@mahoneycm
mahoneycm requested review from amyleadem and mejiaj July 18, 2023 17:18
@mahoneycm mahoneycm changed the title Display: contents on fieldset USWDS - Fieldset: Resolve legend margin error Jul 18, 2023
@mahoneycm
mahoneycm marked this pull request as ready for review July 18, 2023 17:52

@amyleadem amyleadem left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahoneycm This is a nice simple fix and it looks like this solution works as expected in Chrome and Safari. I noticed that Firefox is mostly unchanged, but I believe that is because it was already respecting the margin.

I caught some visual changes that made me think that we should consider this a visually breaking change. Any usa-fieldset with a usa-legend element will now have a different top margin in Chrome and Safari. It looks like Firefox was already respecting the margin so the only changes I expect there are if an additional top margin is added via utility classes, etc (see an example in the notes below).

We should think through the ramifications of this change and flag this as a breaking change if needed. If it is, we should move this to the 3.7.0 release and provide a helpful description for users so they can know what to look for and how to fix the changes.

Change requested:

  • In the form story the fieldsets have an added margin-top-2 utility class. Could you remove those? They will not be needed any longer with this PR and it will make for easier testing.

Some browser notes:

  • In Chrome, Safari, and Firefox the elements are now evenly spaced vertically in the form story (Before | After). This is expected. I just wanted to note that the reasons they are evenly spaced are a bit different:

  • In Chrome and Safari, the margins grew:

    image

  • In Firefox, the margins shrank when the margin-top-2 utility class was present:

    image

  • And there was no change in Firefox when the margin utility class was removed:

    image

  • In Chrome and Safari, I noticed that the mailing address example, the fieldset shifts down (Before | After). This also happens in the create account page (Before | After).

  • In Chrome, I also see that the component preview on our site now has slightly larger vertical margins than before.

    image

I’ll keep poking around at individual components and will report back if I see anything else.

@hobbez

hobbez commented Aug 31, 2023 •

Copy link
Copy Markdown

While testing the proposed solution of .usa-fieldset{display:contents;}, I came across an odd behavior – using the fieldset id as a URL anchor hash does not scroll the page to the fieldset. Without the additional CSS rule, the scroll works fine.

@hobbez

hobbez commented Aug 31, 2023

Copy link
Copy Markdown

As workaround to the anchor issue, setting an ID for the fieldset's legend tag and using that as the anchor instead works fine.

@mahoneycm

Copy link
Copy Markdown
Contributor Author

@amyleadem margin-top-2 removed and pushed up!

I apologize, I should have noted that this change would affect anywhere there is usa-legend nested within usa-fieldset! That was why I added the mailing address and create account pages as examples.

Good to note that this is considered a potentially breaking change. I've updated the PR description with both of those notes!

@mahoneycm
mahoneycm requested a review from amyleadem September 5, 2023 16:10
Comment on lines +6 to 8
display: contents;
margin: 0;
padding: 0;

@amyleadem amyleadem Sep 19, 2023 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying display:contents to the fieldset element appears to cause all other fieldset styles to be ignored. This can be ok on a base level because the USWDS styles are basically declaring a reset to 0. However, I get concerned that this will cause confusion with users who apply utility classes or styles to the fieldset element. It also looks like anchor ids aren't recognized.

Wanted to share this explanation for display:contents because I found the explanation helpful:

display: contents causes an element's children to appear as if they were direct children of the element's parent, ignoring the element itself. This can be useful when a wrapper element should be ignored when using CSS grid or similar layout techniques. 1

I do think that prioritizing usa-legend styles over usa-fieldset make sense because they are more substantive. I just wanted to check if this was the only path forward (does it make sense to apply the base style top margin to usa-fieldset instead of usa-legend?).

Regardless, we should find a way to document this very clearly.

Footnotes

  1. https://caniuse.com/css-display-contents ↩

@mahoneycm mahoneycm Sep 20, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with moving the margin to usa-fieldset is that we offer two different legend options; usa-legend and usa-legend--large, which features larger text and a smaller margin.

@use "uswds-core" as *;
.usa-legend {
@include typeset(
$theme-form-font-family,
$theme-body-font-size,
$theme-input-line-height
);
display: block;
font-weight: font-weight("normal");
margin-top: units(3);
max-width: units($theme-input-max-width);
}
.usa-legend--large {
font-size: font-size($theme-form-font-family, "xl");
font-weight: font-weight("bold");
margin-top: units(2);
}

I wonder if we could take a similar approach to prose and and set the margin using something like

.usa-fieldset {
   ...

   & > .usa-legend {
      margin-top: units(3);
   }
    
   & > + .usa-legend--large {
       margin-top: units(2);
   }
}

What do you think of that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh sorry I forgot there are limitations selecting a parent based on children. I'll look into using has() or other newer selectors to see if it's possible to set the margin on fieldset based on it's child element but we may be stuck there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found my old notes on this issue and postcss rendering breaks when we attempt to target classes using :has().

Alternatively, we could consider creating classes for fieldset to handle the different legend sizes, but that would be a breaking change for users as well.

To me, this was the path of least resistance for users to utlize the margins set via usa-legend classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as documenting this change, do you thing we need to add guidance to our forms page or form elements that use fieldset such as checkbox / radio that users should be mindful when adding styles to fieldset/legend?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh sorry I forgot there are limitations selecting a parent based on children. I'll look into using has() or other newer selectors to see if it's possible to set the margin on fieldset based on it's child element but we may be stuck there

This quirk is limited (or should be) to SASS mixins/functions.

@mejiaj mejiaj Oct 30, 2023 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyleadem @mahoneycm caniuse lists a resource 1 for partial support. It has an important accessibility warning:

The display property has no effect on an element’s semantics: these are defined by the document language and are not affected by CSS. Aside from the none value, which also affects the aural/speech output and interactivity of an element and its descendants, the display property only affects visual layout: its purpose is to allow designers freedom to change the layout behavior of an element without affecting the underlying document semantics.

Looking at our sponsor list example, it means that the item is no longer seen as a list, but as something else (test case in CodePen).

It seems mostly resolved, but safari 2 still has issues. Is there a quick alternative or should we bump from 3.7.0?

Footnotes

  1. https://hidde.blog/more-accessible-markup-with-display-contents/#heading-4 ↩

  2. https://adrianroselli.com/2022/07/its-mid-2022-and-browsers-mostly-safari-still-break-accessibility-via-display-properties.html ↩

Comment thread packages/usa-form/src/test/test-pattern/test-usa-form.twig
@mahoneycm

Copy link
Copy Markdown
Contributor Author

Accessibility concerns

After much consideration, we've decided to hold off on this bug fix for the time being.

Caniuse reports the proposed solution as only partially supported by browsers due to accessibility concerns. Adding display: contents appears to have a history of causing browsers to misinterpret the role attribute. Though some of these concerns have been addressed by the web browsers, it appears there are still be bugs surrounding the use of the display value.

Since this is a result of a browser bug, which can be worked around by adding margin to the fieldset, we're closing this PR for further discussion.

Note
In my voiceover testing, I did not personally run into issues with changing the display of fieldset. We should look into further implications of the potentially affected role attribute and the Accessibility Tree.

@mahoneycm mahoneycm closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Vertical spacing doesn't work for some form input components

4 participants