USWDS - Header: Use $theme-header-min-width for safari fix calc #5624

amyleadem · 2023-11-17T20:52:14Z

Summary

Fixed a bug that prevented $theme-max-header-width from accepting a value of "none".

Breaking change

This is not a breaking change.

Related issue

Closes #5579

Related pull requests

This PR replaces the fix proposed in #5617.

Changelog PR

Preview link

Preview link: Documentation page

Problem statement

A compilation error occurs when setting $theme-max-header-width to "none".

This is a regression which stems from our Safari header navigation bug fix in #5543. The calc() method cannot take a value of none so it breaks the calculation.

Solution

The calculation mistakenly used $theme-header-max-width instead of $theme-header-min-width. The settings have two distinct purposes:

$theme-header-min-width is the setting that determines where the mobile nav should change to the desktop nav.
$theme-header-max-width is the setting that determines the maximum width of the desktop navigation.

For the Safari fix to work as expected, the fix needs to be triggered within 15px of the mobile/desktop breakpoint, which is $theme-header-min-width.

By removing $theme-header-max-width from the calculation, there should no longer be errors when the setting's value is set to "none".

Testing and review

Confirm that there is no compilation error when you set $theme-header-max-width to "none"
Confirm that the Safari bug fix from USWDS - Nav: Fix compilation error when max header width is set to "none" #5617 still works by checking if the mobile menu opens as expected in Safari:
1. Open the Documentation page in Safari
2. Resize the window to somewhere between 1009px-1024px
3. Click the "Menu" button to open the mobile navigation panel
4. Confirm that the mobile header successfully opens as expected
Confirm that the mobile menu opens as expected in other browsers:
1. Open the Documentation page in other browsers
2. Resize the window to somewhere between 1009px-1024px
3. Click the "Menu" button to open the mobile navigation panel
4. Confirm that the main page content is not scrollable
Confirm that the bug fix works with different values for $theme-header-min-width
1. In a local build, change the value of $theme-header-min-width to expected values like "widescreen" or "tablet"
2. Open the documentation page in Safari
3. Resize the window to within 15px of the value set for $theme-header-min-width
4. Confirm that the menu
Confirm that we don't need to account for any special values for $theme-header-min-width that could cause an error (like "none")

mahoneycm

Great catch! Looks like this slipped through since theme-header-max-width and theme-header-min-width both shared the same value.

No visual regression
Fix is logical based on the origin of the bug
Safari bug fix works as expected
Calc error is no longer present

mejiaj

I still see invalid CSS and an error on latest from al-safari-bug-fix-regression.

Steps to reproduce

Set $theme-header-min-width: "none"
Run npx gulp buildSass
Confirm error at the end.

[11:43:19] 'compileSass' errored after 4.29 s
[11:43:19] CssSyntaxError in plugin "gulp-postcss"
Message:
    postcss-csso: /Users/jmejia-a/web/uswds/dist/css/uswds.css:9423:13: Number, dimension, ratio or identifier is expected

  9421 | }
  9422 |
> 9423 | @media (min-width: calc(none - 0.94rem)){
       |             ^
  9424 |   .usa-js-mobile-nav--active.is-safari{
  9425 |     overflow-y:scroll;

[11:43:19] 'buildSass' errored after 9.76 s

packages/usa-nav/src/styles/_usa-nav.scss

thisisdano

Now that I'm digging in, I still see the error that James mentions

amyleadem · 2023-12-06T18:59:40Z

Ok, I'll fix it up and will tag you both for re-review when it's ready.

thisisdano · 2023-12-06T19:02:15Z

Sorry I jumped into this too soon!

mahoneycm

This is looking good to me! One comment about special characters but expected values look great!

Confirm that there is no compilation error when you set $theme-header-max-width to "none"
Confirm that the Safari bug fix from USWDS - Nav: Fix compilation error when max header width is set to "none" #5617 still works by checking if the mobile menu opens as expected in Safari:
Confirm that the mobile menu opens as expected in other browsers:
Confirm that the bug fix works with different values for $theme-header-min-width
Confirm that we don't need to account for any special values for $theme-header-min-width that could cause an error (like "none")

mahoneycm · 2023-12-06T22:09:23Z

packages/usa-nav/src/styles/_usa-nav.scss

-    position: fixed;
-    // --scrolltop set with JS with zero as fallback.
-    top: var(--scrolltop, 0);
+@if $theme-header-min-width != "none" {


Special Tokens

I'm getting a build error when setting $theme-header-min-width: "auto"

Module build failed (from ./node_modules/sass-loader/dist/cjs.js): Undefined operation "calc(auto - 0.94rem) > 0". ╷ 487 │ @if $safari-header-bug-min-width > 0 { │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ╵ packages/usa-nav/src/styles/_usa-nav.scss 487:7 @forward packages/usa-nav/src/styles/_index.scss 4:1 @forward packages/uswds/_index.scss 38:1 root stylesheet

This is a regression from develop.

But it should be noted that setting auto breaks the layout of the header.

Would the error be preferred? If so, should we update the acceptable tokens to exclude values that break the layout? (negative values, 0,)

Potentially out of scope

Important
I began experiencing inconsistent builds and build errors during my testing. There is a chance my broken layouts for certain special tokens might have been a local issue.

@mahoneycm I updated the Safari fix to only run if the value of $theme-header-min-width fits a realistic value for the setting.

The header visually breaks and causes warnings when the value of $theme-header-min-width is outside of the defined breakpoints in our system (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen). This is because the setting is used primary with our at-media mixin, which only accepts the breakpoints defined in $system-properties.

For this reason, I decided to bypass the Safari fix unless the setting was defined with the same values that are acceptable to at-media.

Warning
There is some risk with this approach. If we ever update the accepted values for the at-media mixin to be something other than the system breakpoints, the Safari fix might not be applied in all the right places. However, I feel like the breakpoints in $system-properties will be a stable and reliable place to look for realistic header breakpoints. In my mind, this reduces the risk of future issues.

More on the thought process

First I tried bypassing only the "special" values of “auto” and “none”. This worked in most cases but would cause an error with 1px/2px values because of the unit mismatch.

This caused me to think about how it would be better to only apply the safari fix to *realistic* values, rather than *possible* values. The header visually breaks and causes warnings when the value of `$theme-header-min-width` is outside of the defined breakpoints in our system (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen). For this reason, I decided to bypass the Safari fix unless the setting was defined with one of those values.

Alternatively, we could create a custom map that includes all possible reasonable values, which could mean including the "medium" spacing token set. I ultimately moved away from this because it seemed very rare that a mobile nav would be viewed in safari <120px, especially if the header is broken in other ways by this value.

Can you confirm the following?

Setting $theme-header-min-width to any breakpoint value (card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen)

DOES generate an is-safari class in the CSS

does NOT cause a compile error

Setting $theme-header-min-width to any non-breakpoint value (Ex: "none", "auto", 15, "hamburger")

does NOT generate an is-safari class in the CSS

does NOT cause a compile error. However, this warning is still expected for non-breakpoint values:

Warning: `none` is not a valid USWDS project breakpoint. Valid values: card, card-lg, mobile, mobile-lg, tablet, tablet-lg, desktop, desktop-lg, widescreen

~~I'm still seeing is-safari. The logic looks good and the build warning are appearing, but the safari class is added. I've tried rebuilding the project multiple times.~~

~~The confusing thing is when I run a debug, it returns false as expected and should not be running the block of scss 🤔~~

@debug map-has-key($our-breakpoints, $theme-header-min-width); // Output: false

Update: I misunderstood and was looking for the markup change... CSS looks good 😎

Standard breakpoint generates is-safari class

No compilation errors

Non-breakpoint values bypass safari fix in compiled CSS

Non-breakpoint values cause compilation warning, but no error

Note
Updated the changelog PR to reflect changes to this PR. Feel free to adjust further!

summary: Resolved a compilation error when $theme-max-header-width is set to non-breakpoint values.

amyleadem · 2023-12-07T21:26:45Z

@mejiaj This is ready for review. You can find details about the most recent update in this comment as part of a previous discussion. Curious to hear what you think!

mejiaj

Nice fix. I tested:

$theme-header-min-width: "none";
$theme-header-min-width: "tablet-lg";
$theme-header-min-width: "desktop"

No errors or invalid CSS compiled.

thisisdano

🎉

Use $theme-header-min-width for safari fix calc

f3bce77

amyleadem marked this pull request as ready for review November 17, 2023 21:36

amyleadem mentioned this pull request Nov 17, 2023

USWDS - Nav: Fix compilation error when max header width is set to "none" #5617

Closed

3 tasks

mahoneycm approved these changes Nov 17, 2023

View reviewed changes

amyleadem requested a review from mejiaj November 17, 2023 21:53

amyleadem mentioned this pull request Dec 4, 2023

USWDS-Site: Documentation updates for release 3.7.1 uswds/uswds-site#2385

Merged

mejiaj suggested changes Dec 6, 2023

View reviewed changes

Check for "none" value before safari fix

21c3469

thisisdano approved these changes Dec 6, 2023

View reviewed changes

amyleadem commented Dec 6, 2023

View reviewed changes

packages/usa-nav/src/styles/_usa-nav.scss Outdated Show resolved Hide resolved

thisisdano suggested changes Dec 6, 2023

View reviewed changes

amyleadem added 2 commits December 6, 2023 12:39

Scope safari fix to only work with expected values

e7939f7

Safari fix checks for "none" and negative values

e99b5b0

amyleadem requested a review from mejiaj December 6, 2023 20:08

amyleadem mentioned this pull request Dec 6, 2023

Draft release notes for 3.7.1 #5662

Closed

mahoneycm reviewed Dec 6, 2023

View reviewed changes

amyleadem added 3 commits December 7, 2023 08:23

Check for "auto" before safari fix

85aae5e

Clean up safari fix token check

8a212d1

Only apply Safari fix if given a breakpoint token

4ffcfa0

mejiaj approved these changes Dec 7, 2023

View reviewed changes

amyleadem requested a review from thisisdano December 7, 2023 22:05

thisisdano approved these changes Dec 8, 2023

View reviewed changes

thisisdano merged commit 4d6e2c2 into develop Dec 8, 2023

thisisdano deleted the al-safari-bug-fix-regression branch December 8, 2023 20:46

thisisdano mentioned this pull request Dec 8, 2023

USWDS 3.7.1 #5670

Merged

SherfeyInv mentioned this pull request Jun 7, 2024

[Snyk] Upgrade @uswds/uswds from 3.7.1 to 3.8.0 SherfeyInv/digitalgov.gov#4

Closed

sennhann29 mentioned this pull request Jul 26, 2024

[Snyk] Upgrade @uswds/uswds from 3.4.1 to 3.8.1 sennhann29/digitalgov.gov#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

USWDS - Header: Use $theme-header-min-width for safari fix calc #5624

USWDS - Header: Use $theme-header-min-width for safari fix calc #5624

Uh oh!

amyleadem commented Nov 17, 2023 •

edited

Loading

Uh oh!

mahoneycm left a comment

Uh oh!

mejiaj left a comment

Uh oh!

Uh oh!

thisisdano left a comment

Uh oh!

amyleadem commented Dec 6, 2023

Uh oh!

thisisdano commented Dec 6, 2023

Uh oh!

mahoneycm left a comment

Uh oh!

mahoneycm Dec 6, 2023

Uh oh!

amyleadem Dec 7, 2023 •

edited

Loading

Uh oh!

mahoneycm Dec 7, 2023 •

edited

Loading

Uh oh!

mahoneycm Dec 7, 2023

Uh oh!

amyleadem commented Dec 7, 2023

Uh oh!

mejiaj left a comment

Uh oh!

thisisdano left a comment

Uh oh!

Uh oh!

USWDS - Header: Use $theme-header-min-width for safari fix calc #5624

USWDS - Header: Use $theme-header-min-width for safari fix calc #5624

Uh oh!

Conversation

amyleadem commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking change

Related issue

Related pull requests

Preview link

Problem statement

Solution

Testing and review

Uh oh!

mahoneycm left a comment

Choose a reason for hiding this comment

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thisisdano left a comment

Choose a reason for hiding this comment

Uh oh!

amyleadem commented Dec 6, 2023

Uh oh!

thisisdano commented Dec 6, 2023

Uh oh!

mahoneycm left a comment

Choose a reason for hiding this comment

Uh oh!

mahoneycm Dec 6, 2023

Choose a reason for hiding this comment

Special Tokens

Uh oh!

amyleadem Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahoneycm Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

amyleadem commented Dec 7, 2023

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

thisisdano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amyleadem commented Nov 17, 2023 •

edited

Loading

amyleadem Dec 7, 2023 •

edited

Loading

mahoneycm Dec 7, 2023 •

edited

Loading