✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

• New styling for remaining dropdowns (select all 5) #13798 👈 (View in Graphite)
• New styling for date and time widgets (select all 4) #13797
• New styling for selectbox and multiselect (select all 3) #13796
• Add select all and select x matches (select all 2) #13795
• Define selection constants (select all 1) #13794
• develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

✅ PR preview is ready!

Summary

This PR unifies dropdown/popover styling across multiple components to achieve a consistent visual design language. The key changes are:

1. Light/dark mode border behavior: In light mode, borders are hidden (or match the background) and a boxShadow popover effect is shown instead. In dark mode, a visible border is used with no shadow.
2. Border radius updates: Several components switch from theme.radii.default or theme.radii.md to theme.radii.md2 for inner elements, creating more consistent rounding.
3. Spacing adjustments: Menu items, dividers, and header rows have updated padding/margins for tighter, more consistent spacing.
4. popoverMargin: Added to Popover, BaseColorPicker, and TopNavSection for consistent gap between trigger and popover.
5. Scrollbar gutter handling: ColumnVisibilityMenu now uses a CSS custom property (--scrollbar-gutter-size) to adjust divider padding when a scrollbar is present.
6. Background color simplification: Dark mode background for Inner elements changed from secondaryBg to bgColor for consistency.

The PR touches 9 source files and updates 54 screenshot snapshots across chromium, firefox, and webkit.

Code Quality

The code is well-structured and follows existing patterns. Comments explain the rationale for each styling choice (e.g., "No border in light mode, visible border in dark mode"). A few observations:

1. Minor inconsistency in Popover.tsx (line 119-126): The borderWidth is now conditionally set to theme.spacing.none (i.e., "0") in light mode, but the padding calculation on lines 123-126 still unconditionally subtracts theme.sizes.borderWidth ("1px"):

              borderWidth: lightBackground
                ? theme.spacing.none
                : theme.sizes.borderWidth,

              paddingRight: `calc(${theme.spacing.twoXL} - ${theme.sizes.borderWidth})`, // 1px to account for border.
              paddingLeft: `calc(${theme.spacing.twoXL} - ${theme.sizes.borderWidth})`,
              paddingBottom: `calc(${theme.spacing.twoXL} - ${theme.sizes.borderWidth})`,
              paddingTop: `calc(${theme.spacing.twoXL} - ${theme.sizes.borderWidth})`,

In dark mode: total = 1px border + (1.5rem - 1px) padding = 1.5rem. Correct.
In light mode: total = 0px border + (1.5rem - 1px) padding = 1.5rem - 1px. Off by 1px.
While 1px is likely not noticeable, the comment "1px to account for border" now only applies in dark mode. The padding should ideally be conditioned on the same lightBackground flag for correctness.

2. Inconsistent border approach across menus: Two different strategies are used:

   • ColumnVisibilityMenu uses borderColor: bgColor with a constant borderWidth to keep the border invisible in light mode without pixel shifts.
   • FormattingMenu, BaseColorPicker, Popover, and TopNavSection use borderWidth: none in light mode.
   • ColumnMenu uses yet a third approach with boxShadow: "none" on Body and all styling on Inner.

   All three approaches work, but the variation could be confusing for future maintainers. The comments do a good job explaining the rationale.

3. Style function vs. style object: The PR converts some style: { ... } to style: () => ({ ... }) (e.g., ColumnVisibilityMenu, FormattingMenu). While this works fine with BaseWeb's API, it means a new style object is created on each render. This has negligible performance impact but is worth noting as a pattern choice.

4. Use of theme.spacing.none for borderWidth: Using theme.spacing.none (which resolves to "0") for a borderWidth property is technically mixing semantic tokens — spacing is meant for layout spacing, not border sizing. While it works, using a literal "0" or 0 might be clearer. However, this appears to be a pre-existing pattern in the codebase, so it's acceptable for consistency.

Test Coverage

• 54 screenshot snapshots updated across chromium, firefox, and webkit for: popover, navigation top nav, dataframe column menu, visibility menu, formatting menu, and search toolbar. This provides good visual regression coverage.
• No new unit tests added. This is acceptable since the changes are purely visual/styling and are well-covered by the screenshot tests. The existing unit tests for the affected components (Popover.test.tsx, BaseColorPicker.test.tsx, ColumnMenu.test.tsx, ColumnVisibilityMenu.test.tsx, FormattingMenu.test.tsx) should continue to pass as no behavioral changes were made.
• No new E2E tests added. The existing E2E tests with updated snapshots adequately cover the visual changes.

Backwards Compatibility

No breaking changes. All changes are internal styling adjustments:

• No public API changes (no Python or protobuf changes).
• No behavioral changes to any component.
• The visual appearance changes are intentional and part of a design system update.
• Custom themes should work correctly since the changes use theme tokens consistently.

Security & Risk

• No security concerns: Changes are limited to CSS styling.
• Low regression risk: The visual changes are well-covered by snapshot tests across three browser engines.
• Potential minor concern: The ColumnVisibilityMenu now removes maxHeight from the Inner div and moves scrolling to a content div. While the comment explains this is to clip the scrollbar within rounded corners, the 70vh cap on the outer content div ensures the menu doesn't grow unboundedly.

Accessibility

• No interactive elements were added, removed, or altered.
• role="menuitem" attributes remain intact.
• aria-label attributes are preserved.
• Focus styles and keyboard navigation remain unchanged.
• The borderRadius changes are purely cosmetic and don't affect assistive technology.
• Color contrast should be maintained since the changes use existing theme tokens (bgColor, bodyText, borderColor).

Recommendations

1. Consider fixing the Popover.tsx padding calculation (Popover.tsx lines 123-126): The padding subtraction should account for the conditional border width. Something like:

   const borderWidthValue = lightBackground ? theme.spacing.none : theme.sizes.borderWidth
   // ... then use borderWidthValue in both borderWidth and padding calc
   paddingRight: `calc(${theme.spacing.twoXL} - ${borderWidthValue})`,

   This fixes the 1px discrepancy in light mode and makes the code self-consistent.

2. Consider documenting the border strategy pattern: Since there are now three approaches used across menus (border=none, border=bgColor, transparent Body + styled Inner), it might help future contributors to have a brief comment or dev doc explaining when to use which approach. This is optional but would improve maintainability.

3. Minor: In StyledPopoverContent (styled-components.ts line 326), the change from 0 to theme.spacing.none is a good practice for token consistency, but the comment "Match dropdown vertical padding (twoXS top/bottom)" is slightly confusing since the actual vertical padding value is twoXS, not none. The comment is about the vertical padding being twoXS, and the horizontal none is implied. This is fine as-is.

Verdict

APPROVED: This is a well-executed visual styling unification PR. The changes consistently apply a light/dark mode border/shadow pattern across 5+ dropdown components, are backed by comprehensive snapshot tests across 3 browsers, and carry no behavioral or backwards-compatibility risk. The 1px Popover padding discrepancy (recommendation #1) is a minor nit that could be addressed in a follow-up.

This is an automated AI review using opus-4.6-thinking. Please verify the feedback and use your judgment.

Merge activity

• Feb 16, 9:33 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 16, 9:34 PM UTC: Graphite couldn't merge this pull request because a downstack PR Define selection constants (select all 1) #13794 failed to merge.
• Feb 16, 11:04 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 16, 11:23 PM UTC: Graphite couldn't merge this pull request because a downstack PR Add select all and select x matches (select all 2) #13795 failed to merge.
• Feb 16, 11:48 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 17, 12:29 AM UTC: Graphite couldn't merge this PR because it had merge conflicts.

@sfc-gh-bnisco I force pushed to get rid of the incorrect merge. This is supposed to be 17 commits from (st.popover a0c6d22 to Update snapshots b20b7ed). Somehow when Graphite failed in the middle, a couple of the commits from the previous branch are showing up here and not properly rebased out. I tried to restack/rebase it, but it kept saying, "we believe [this] is 93 commits long" and it wasn't working like expected trying to point it to a different starting point.

There are a handleful of merge conflicts that popped up from something new in develop while the stack was merging. I've cherry-picked the 17 commits into a clean branch with the correct resolution from the current state of develop: safekeeping branch

So, I can abandon this and make a clean PR if I've made this too much of a mess. Either way, I have the data on safely on that other branch.