fix: Implement TopNavigation breakpoints in CSS #3371

TrevorBurnham · 2025-03-24T18:03:25Z

Description

This PR switches from JavaScript-based breakpoints to CSS container queries for TopNavigation, making its appearance consistent when rendered with SSR. I've added Sass mixins for this purpose that can be reused across other components to replace the useBreakpoints hook.

I've opted to use container queries rather than media queries for backward compatibility's sake, since useBreakpoints checks container size rather than viewport size. However, this introduces a complication: To use container queries, we need to add container-type: inline-size to the TopNavigation element's styles.

I noticed that adding container-type: inline-size causes overflowing content to get clipped in Safari due to a longstanding bug in that browser. [Update 4/5: After updating to Safari 18.4 on my Mac, I no longer see the bug, so it looks like Apple just fixed this.] To address that, I've enabled expandToViewport to render utility dropdowns in a portal. I've also added position: relative and z-index: 1 to the TopNavigation styles to give it a stacking context, which appears to help with the issue. However, there is still a possibility that users who are rendering their own dropdowns in the TopNavigation (e.g. by using an Autosuggest searchbox) without expandToViewport may see issues in older versions of Safari where the dropdown is rendered behind other content with a higher z-index. I would suggest calling this out in the release notes and recommending the use of expandToViewport for dropdowns in TopNavigation.

Once this change is merged, I'd love to start applying this same change to other components that use useBreakpoints. Container queries are supported in all modern browsers.

Fixes #3337

How has this been tested?

I've run this change locally, eyeballed it at different breakpoints, and clicked various controls to ensure that they're visible. I'm open to ideas for testing the breakpoint behavior more thoroughly.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

pan-kot · 2025-03-26T12:04:52Z

Hello @TrevorBurnham,

Thanks for contributing the fix!

Our team is taking a look and running internal screenshot tests and manual tests.

Could you please fix the failing unit tests? P.S. the "Measure bundle size" action is expected to fail, please ignore it.

TrevorBurnham · 2025-03-26T13:28:47Z

@pan-kot Sure thing. I've pushed a fix for the unit tests: 51e8d78

codecov · 2025-03-27T09:11:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (0a681b8) to head (0da8645).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3371      +/-   ##
==========================================
- Coverage   96.46%   96.46%   -0.01%     
==========================================
  Files         805      805              
  Lines       23019    23004      -15     
  Branches     7963     7535     -428     
==========================================
- Hits        22205    22190      -15     
- Misses        760      807      +47     
+ Partials       54        7      -47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pan-kot · 2025-03-27T16:12:59Z

@TrevorBurnham, thanks for addressing the tests! Please also take a look at test coverage.

Also, there is a small regression: the right hand size padding became smaller, see attachments:

Expected:

Actual:

TrevorBurnham · 2025-03-27T16:26:37Z

Thanks! Can you give me instructions for viewing the component before/after? What I've been doing is running npm run build and then running a project against the built components directory, but some of the styles seem to be lost in that process (possibly because the "visual refresh" flag doesn't work).

pan-kot · 2025-03-28T10:55:01Z

Thanks! Can you give me instructions for viewing the component before/after? What I've been doing is running npm run build and then running a project against the built components directory, but some of the styles seem to be lost in that process (possibly because the "visual refresh" flag doesn't work).

The npm run build should be fine: you can build the mainline and then your commit, and compare the differences. The ./pages/lib folder should contain the static assets for the dev pages.

TrevorBurnham · 2025-03-31T11:47:07Z

I've updated the PR branch to fix the visual inconsistencies and improve code coverage.

One call-out: Previously TopNavigation passed a prop called offsetRight to the ButtonTrigger for utility menu dropdowns, then added styles based on the .offset-right-* class. Since the offsetRight value depended on the breakpoint in JavaScript, that approach doesn't work; there needs to be a CSS selector that can be used in the TopNavigation styles to reference the ButtonTrigger element. So I added a prop called internalClass to allow a classname to be passed through. Let me know if there's a better way to do this.

src/top-navigation/styles.scss

src/internal/components/menu-dropdown/index.tsx

pan-kot · 2025-04-02T05:08:38Z

src/test-utils/dom/top-navigation/index.ts

@@ -101,10 +101,10 @@ export class TopNavigationMenuDropdownWrapper extends ButtonDropdownWrapper {
  }

  findTitle(): ElementWrapper | null {
-    return this.findByClassName(buttonDropdownStyles.title);
+    return createWrapper().findComponent(`.${buttonDropdownStyles.title}`, ElementWrapper);


What is the reason of this change?

Without this change, the test

TopNavigation Utility part › Menu dropdown › has a title in the dropdown

fails to find the title element, due to the menu dropdown having expandToViewport.

pan-kot · 2025-04-02T05:15:25Z

src/test-utils/dom/button-dropdown/index.ts

@@ -40,20 +40,30 @@ export default class ButtonDropdownWrapper extends ComponentWrapper {
   * Finds an item in the open dropdown by item id. Returns null if there is no open dropdown.
   *
   * This utility does not open the dropdown. To find dropdown items, call `openDropdown()` first.
+   *


There are many button dropdown tests that use expandToViewport (see example: https://github.com/cloudscape-design/components/blob/main/src/button-dropdown/__tests__/button-dropdown-highlighted.test.tsx#L24), but the expandToViewport option wasn't needed. I think that is because the findOpenDropdown() method searches from the root, not component root - assuming there is at most one open dropdown. Why do we need the expandToViewport option now?

Good call-out. This change wasn't needed.

pan-kot · 2025-04-02T05:16:20Z

src/internal/styles/foundation/breakpoints.scss

@@ -39,3 +51,27 @@ $_largest_breakpoint: $breakpoint-x-large;
    @content;
  }
 }
+
+// Container query for widths greater than the given breakpoint.


This commit switches from JavaScript-based breakpoints to CSS container queries for TopNavigation, making its appearance consistent when rendered with SSR. To support container queries, we need `container-type: inline-size;` on the TopNavigation root element. This causes overflowing content to get clipped (at least in Safari), so I've enabled `expandToViewport` to render utility dropdowns in a portal. Note that OverflowMenu doesn't need this treatment because it expands the height of TopNavigation rather than overflowing. Fixes cloudscape-design#3337

TrevorBurnham · 2025-04-05T22:17:32Z

I've rebased this branch and made the following changes:

Added position: relative and z-index: 1 to the TopNavigation to work around a Safari bug where overflow content from containers with container-type: inline-size can be incorrectly rendered behind other elements, causing clipping. I've updated the PR description with more details about this bug, which I think is the most serious potential issue with this change.
Renamed internalClassName to just className and added an explanatory comment re: feedback.
Reduced unnecessary test changes and comments re: feedback.

TrevorBurnham requested a review from a team as a code owner March 24, 2025 18:03

TrevorBurnham requested review from pan-kot and removed request for a team March 24, 2025 18:03

TrevorBurnham mentioned this pull request Mar 24, 2025

[Bug]: TopNavigation size changes after SSR hydration #3337

Closed

2 tasks

TrevorBurnham changed the title ~~Implement TopNavigation breakpoints in CSS~~ fix: Implement TopNavigation breakpoints in CSS Mar 24, 2025

TrevorBurnham force-pushed the topnavigation-breakpoints-3337 branch from 9c18810 to 51e8d78 Compare March 26, 2025 13:28

TrevorBurnham had a problem deploying to dev-pages March 27, 2025 09:11 — with GitHub Actions Failure

TrevorBurnham force-pushed the topnavigation-breakpoints-3337 branch from 51e8d78 to ef604a8 Compare March 31, 2025 03:20

TrevorBurnham had a problem deploying to dev-pages April 2, 2025 04:49 — with GitHub Actions Failure

pan-kot reviewed Apr 2, 2025

View reviewed changes

TrevorBurnham force-pushed the topnavigation-breakpoints-3337 branch from ef604a8 to 0da8645 Compare April 5, 2025 22:06

TrevorBurnham had a problem deploying to dev-pages April 7, 2025 09:48 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Implement TopNavigation breakpoints in CSS #3371

fix: Implement TopNavigation breakpoints in CSS #3371

TrevorBurnham commented Mar 24, 2025 •

edited

Loading

pan-kot commented Mar 26, 2025

TrevorBurnham commented Mar 26, 2025

codecov bot commented Mar 27, 2025 •

edited

Loading

pan-kot commented Mar 27, 2025

TrevorBurnham commented Mar 27, 2025

pan-kot commented Mar 28, 2025

TrevorBurnham commented Mar 31, 2025

pan-kot Apr 2, 2025

TrevorBurnham Apr 5, 2025

pan-kot Apr 2, 2025

TrevorBurnham Apr 5, 2025

pan-kot Apr 2, 2025

TrevorBurnham commented Apr 5, 2025

fix: Implement TopNavigation breakpoints in CSS #3371

Are you sure you want to change the base?

fix: Implement TopNavigation breakpoints in CSS #3371

Conversation

TrevorBurnham commented Mar 24, 2025 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

pan-kot commented Mar 26, 2025

TrevorBurnham commented Mar 26, 2025

codecov bot commented Mar 27, 2025 • edited Loading

Codecov Report

pan-kot commented Mar 27, 2025

TrevorBurnham commented Mar 27, 2025

pan-kot commented Mar 28, 2025

TrevorBurnham commented Mar 31, 2025

pan-kot Apr 2, 2025

Choose a reason for hiding this comment

TrevorBurnham Apr 5, 2025

Choose a reason for hiding this comment

pan-kot Apr 2, 2025

Choose a reason for hiding this comment

TrevorBurnham Apr 5, 2025

Choose a reason for hiding this comment

pan-kot Apr 2, 2025

Choose a reason for hiding this comment

TrevorBurnham commented Apr 5, 2025

TrevorBurnham commented Mar 24, 2025 •

edited

Loading

codecov bot commented Mar 27, 2025 •

edited

Loading