USWDS: Header: Add fix for Safari-only header bug #5443

amyleadem · 2023-08-16T18:24:36Z

Summary

Fixed a Safari-only bug that caused the mobile menu to unexpectedly close at a narrow range of window widths.

Breaking change

This is not a breaking change.

Related issue

Closes #5371

Related pull requests

Changelog PR

Preview link

Preview link: Documentation page

Problem statement

In Safari, the mobile header panel will unexpectedly close when opened in a narrow range of window widths. This issue happens when the mobile menu is opened in windows widths within the 15px preceding the $theme-header-max-width breakpoint. With the default "desktop" value of ` 1024px, this issue happens at window widths of 1009px - 1023px.

The issue is demonstrated in this GIF when testing the range right before the "desktop" breakpoint:

Known Safari bug

This issue happens in Safari because it does not include the width of the vertical scrollbar when it calculates the width of the viewport. (See this open webkit bug report). This means that in Safari, the registered viewport width will change depending on the presence or absence of a vertical scrollbar.

In USWDS, opening the mobile menu removes the vertical scrollbar on <body> because its overflow is set to hidden. When this scrollbar disappears, Safari updates the value of the viewport size to be 15px (the width of the scrollbar) wider than the original value. If this new viewport width registers as a "desktop" width, the CSS will show the desktop view of the header.

For example, if you trigger the mobile menu at an original viewport width of 1020px, Safari will reset value of the viewport width to 1035px once the menu is opened. Because this new width is greater than the default $theme-header-max-width value of 1024px, the CSS turns on desktop menu display.

Solution

To resolve this, this PR forces Safari to keep the the body's vertical scrollbar when the mobile menu is open. This fix is applied only to Safari and only to the affected 15px of screen width (by default, 1009-1024px).

Forcing the scrollbar at these window widths prevents Safari from changing its width definition when the mobile menu is open, which removes confusion about which version of the header navigation to show.

Note
This script checks specifically for "not Chrome" because the Chrome browser reports having both Chrome and Safari userAgent. More details in this MDN article.

Testing and review

Review:

Consider if allowing vertical scroll for these 15px is a reasonable cost for this solution
Confirm that this change only affects Safari and only at the affected screen widths (by default, 1009px-1024px)
Confirm that the is-safari class is added to Safari only
Confirm the code meets standards
Confirm the changelog looks good

Use Storybook to test that the mobile menu opens in Safari:

Open the Documentation page in Safari
Resize the window to somewhere between 1009px-1024px
Click the "Menu" button to open the mobile navigation panel
Confirm that the mobile header successfully opens as expected

Use Storybook to test that this does not affect other browsers:

Open the Documentation page in other browsers
Resize the window to somewhere between 1009px-1024px
Click the "Menu" button to open the mobile navigation panel
Confirm that the main page content is not scrollable

- This forces the browser scrollbar to prevent media query confusion

mahoneycm

This is working well for me! While the scrolling isn't ideal, I think it's a rare case someone would find themselves at that viewport size and try to scroll.

A couple potential recommendations to try as well as a documentation edit then I think we'll be golden 🥇

mahoneycm · 2023-08-24T16:41:35Z

packages/usa-header/src/index.js

+  // Detect Safari and add a body class for a Safari-only CSS bug fix.
+  // More details in https://github.com/uswds/uswds/pull/5443
+  const isSafari = navigator.userAgent.indexOf("Safari") !== -1;
+  const isNotChrome = navigator.userAgent.indexOf("Chrome") === -1;
+  if (isSafari && isNotChrome) {
+    body.classList.add("is-safari");
+  }
+


I was wondering why we check for Chrome but not other browsers. I've found articles that explain that Chromium browsers will provide the Safari user agent as well as the chomium one.

Could we add a note about this and potentially a source so if we forget and question this down the road it's well documented?

Good call, @mahoneycm. I've added a comment in the code and a note in 5a41838.

I also included a note in the Solution section of the PR description pointing to this MDN article that explains userAgent detection with Safari and Chrome.

mahoneycm · 2023-08-24T16:47:03Z

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

+@media (min-width: $safari-header-bug-min-width) {
+  .usa-js-mobile-nav--active.is-safari {
+    overflow: scroll;
+  }
+}


In your JS solution, were you capturing the current scroll position then using that to lock the view to that position?

I was curious if we could set position: fixed with the current top offset to lock vertical scrolling at these widths but it seems like your previous solution was doing something similar.

Interesting idea!
In 625c7f4, I added position: fixed to the body CSS when the overflow: scroll rule is enacted in Safari, and it does prevent scroll while maintaining the scrollbar. (Woohoo!)

A couple of things I noticed: When the menu is opened in this state, the page content does shift down a bit so that the content aligns with the top of the viewport. In the current state, the content also shifts, but not as much and only on page close.

I will work on thinking through if there are any possible downsides to this approach. I will also experiment with iOS tomorrow.

This stack overflow solution uses JavaScript to calculate the position of the viewport after the scroll and set the document body top to match it to prevent the content shift.

I like this idea. Thanks! Implemented in 0600cf0.

Now when the mobile menu is open in the 15px window range in Safari, the body element will have fixed positioning and receive the current JS scrollTop value to set the body'stop position.

Trying to think of any potential unintended side effects and I think we are good to go. Curious if anyone else has thoughts on this.

I noticed that while there isn't a vertical shift when the menu opens, there is a shift to the top when the menu is then closed.

If we can get that sorted I think we'll be in a good standing! I like that this solution prevents scrolling on the affected widths.

@mahoneycm This is pre-existing behavior, so I am inclined to leave it as is in this PR just to keep things scoped. What do you think?

@amyleadem Good point! Since the header isn't sticky and remains at the top, I think that we can leave it 👍

@mahoneycm Great! Ready to smash that "approve" button? Are there any outstanding items?

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

mejiaj

Nice fix. Added some suggestions. I was able to test this in Safari 16.6 without any issues.

Consider moving the Safari check to its own function and calling that in toggleNav().

packages/usa-header/src/index.js

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

packages/usa-header/src/index.js

- Improve readability of code

amyleadem

Thanks for the suggestions @mejiaj. Definitely feels more readable now. You and @mahoneycm are making me better!

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

packages/usa-header/src/index.js

mejiaj

Very minor suggestion to separate browser check from setting padding. Otherwise LGTM.

I tested functionality on:

Chromium v116
Firefox v118
Safari v16.6
Safari iPad mini (2022)

The class only shows in Safari and the scrollbar is gone.

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

mejiaj · 2023-09-22T13:55:31Z

packages/usa-header/src/index.js

+const setSafariScrollPosition = (body) => {
+  const isSafari = navigator.userAgent.includes("Safari");
+  const isNotChrome = !navigator.userAgent.includes("Chrome");
+  const currentScrollPosition = `-${window.scrollY}px`;
+  if (isSafari && isNotChrome) {
+    body.style.setProperty("--scrolltop", currentScrollPosition);
+    body.classList.add("is-safari");
+  }
+};


On initial load, the safari class is missing. It's only added on initial toggle.

It might be better to split this into two functions, one that simply checks for safari (and possibly adds class) and another that handles the scroll.

Example

@mejiaj Good idea. I broke out addSafariClass() from setSafariScrollPosition() in 75ed3a3. Let me know what you think.

Co-authored-by: James Mejia <james.mejia@gsa.gov>

mejiaj

Nice job, I was able to confirm:

Safari class doesn't show on other browsers
Scroll error is gone in Safari
Only padding right is modified when setting/removing styles

thisisdano

Well-scoped solution

Add fix for Safari-only header bug fix

f78c1ae

- This forces the browser scrollbar to prevent media query confusion

This was referenced Aug 16, 2023

USWDS - header: Fix Safari scrollbar bug #5410

Closed

USWDS-Site - Changelog: Safari header bug [#5443] uswds/uswds-site#2251

Merged

Amy Leadem added 3 commits August 16, 2023 14:23

Move Safari browser detection to JS

6b0ed09

Use 15px instead of 1rem; update comments

24b0c4d

Add comments for Safari bug fix

ebb109c

amyleadem marked this pull request as ready for review August 22, 2023 15:27

amyleadem requested review from mahoneycm and mejiaj August 22, 2023 15:41

mahoneycm reviewed Aug 24, 2023

View reviewed changes

Amy Leadem added 6 commits September 5, 2023 10:38

Add comment about chrome userAgent report

5a41838

Add fixed positioning on body for Safari bug

625c7f4

Set body top position with scrollTop

0600cf0

Run prettier

7ce6493

Test: Remove media query to test impact in iOS

4e0a9b6

Revert test

b54c1f6

amyleadem requested a review from mahoneycm September 7, 2023 20:21

amyleadem mentioned this pull request Sep 7, 2023

Release notes: Track 3.6.1 changelogs and release notes #5492

Closed

mahoneycm approved these changes Sep 14, 2023

View reviewed changes

mejiaj suggested changes Sep 20, 2023

View reviewed changes

packages/usa-header/src/index.js Outdated Show resolved Hide resolved

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

packages/usa-header/src/index.js Outdated Show resolved Hide resolved

Amy Leadem added 3 commits September 20, 2023 09:22

Add comment for scrolltop var

cb3ce22

Move Safari check to new setSafariScrollPosition()

9c8f878

- Improve readability of code

Run prettier

de7f838

amyleadem commented Sep 20, 2023

View reviewed changes

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

packages/usa-header/src/index.js Outdated Show resolved Hide resolved

packages/usa-header/src/index.js Outdated Show resolved Hide resolved

amyleadem requested a review from mejiaj September 20, 2023 16:49

mejiaj suggested changes Sep 22, 2023

View reviewed changes

mejiaj mentioned this pull request Sep 22, 2023

USWDS-Site: Documentation updates for release 3.6.1 uswds/uswds-site#2283

Merged

amyleadem and others added 3 commits September 22, 2023 08:24

Update Safari fix comment

39550da

Co-authored-by: James Mejia <james.mejia@gsa.gov>

Separate addSafariClass from setSafariScrollPosition

75ed3a3

Run prettier

e9f3395

amyleadem requested a review from mejiaj September 22, 2023 16:19

mejiaj approved these changes Sep 25, 2023

View reviewed changes

mejiaj requested a review from thisisdano September 25, 2023 14:07

thisisdano approved these changes Sep 28, 2023

View reviewed changes

thisisdano merged commit 47f3117 into develop Sep 28, 2023

thisisdano deleted the al-safari-header-bug branch September 28, 2023 22:03

thisisdano mentioned this pull request Sep 29, 2023

USWDS 3.6.1 #5532

Merged

mahoneycm mentioned this pull request Oct 27, 2023

USWDS - Bug: Setting $theme-header-max-width: "none" causes error when building usa-nav styles #5579

Closed

2 tasks

mahoneycm mentioned this pull request Nov 15, 2023

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

Closed

3 tasks

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

USWDS: Header: Add fix for Safari-only header bug #5443

USWDS: Header: Add fix for Safari-only header bug #5443

Uh oh!

Conversation

amyleadem commented Aug 16, 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

Known Safari bug

Solution

Testing and review

Uh oh!

mahoneycm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amyleadem Sep 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amyleadem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mejiaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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 Aug 16, 2023 •

edited

Loading

amyleadem Sep 12, 2023 •

edited

Loading