From fe7dcc20b9ad12c4a8c896f28a16094f2935de64 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 6 May 2026 09:33:37 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"Replace=20`ActionBar`=20overflow=20ca?= =?UTF-8?q?lculations=20with=20CSS=20wrapping=20approach=20=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f59a1b190027b56f853797c4dc2cf9475cf5e225. --- .changeset/many-suns-promise.md | 5 - e2e/components/drafts/ActionBar.test.ts | 4 +- .../ActionBar/ActionBar.examples.stories.tsx | 8 + .../react/src/ActionBar/ActionBar.module.css | 60 +-- .../react/src/ActionBar/ActionBar.test.tsx | 5 +- packages/react/src/ActionBar/ActionBar.tsx | 488 +++++++++++------- 6 files changed, 323 insertions(+), 247 deletions(-) delete mode 100644 .changeset/many-suns-promise.md diff --git a/.changeset/many-suns-promise.md b/.changeset/many-suns-promise.md deleted file mode 100644 index 6b465792c8e..00000000000 --- a/.changeset/many-suns-promise.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@primer/react": minor ---- - -Replace `ActionBar` overflow calculations with CSS wrapping approach to improve performance and stability diff --git a/e2e/components/drafts/ActionBar.test.ts b/e2e/components/drafts/ActionBar.test.ts index 4754c730059..c39e4d1a9d0 100644 --- a/e2e/components/drafts/ActionBar.test.ts +++ b/e2e/components/drafts/ActionBar.test.ts @@ -47,12 +47,12 @@ test.describe('ActionBar', () => { }, }) const toolbarButtonSelector = `button[data-component="IconButton"]` - await expect(page.locator(toolbarButtonSelector).filter({visible: true})).toHaveCount(10) + await expect(page.locator(toolbarButtonSelector)).toHaveCount(10) await page.setViewportSize({width: viewports['primer.breakpoint.xs'], height: 768}) await page.getByLabel('Task List').waitFor({ state: 'hidden', }) - await expect(page.locator(toolbarButtonSelector).filter({visible: true})).toHaveCount(8) + await expect(page.locator(toolbarButtonSelector)).toHaveCount(8) const moreButtonSelector = page.getByLabel('More Comment box toolbar items') await moreButtonSelector.click() await expect(page.locator('ul[role="menu"] [role="menuitem"]')).toHaveCount(3) diff --git a/packages/react/src/ActionBar/ActionBar.examples.stories.tsx b/packages/react/src/ActionBar/ActionBar.examples.stories.tsx index 39d4c54ef7a..6ef573c6341 100644 --- a/packages/react/src/ActionBar/ActionBar.examples.stories.tsx +++ b/packages/react/src/ActionBar/ActionBar.examples.stories.tsx @@ -56,6 +56,14 @@ export const WithGroups = () => ( ) +export const TextLabels = () => ( + + + + + +) + export const SmallActionBar = () => ( diff --git a/packages/react/src/ActionBar/ActionBar.module.css b/packages/react/src/ActionBar/ActionBar.module.css index cd29d9fdad5..13aab764f53 100644 --- a/packages/react/src/ActionBar/ActionBar.module.css +++ b/packages/react/src/ActionBar/ActionBar.module.css @@ -6,38 +6,10 @@ /* wonder why this is here */ /* stylelint-disable-next-line primer/spacing */ margin-bottom: -1px; + white-space: nowrap; list-style: none; - align-items: flex-start; + align-items: center; gap: var(--actionbar-gap, var(--stack-gap-condensed)); - overflow: hidden; - /* Explicit height is required to clip wrapped items */ - height: var(--actionbar-height, var(--control-small-size)); - - /* Only apply scroll animation before overflow is calculated (for SSR/initial render) */ - &:not([data-has-overflow]) { - /* Scroll-based animations have no effect unless the container is scrollable (has overflow, even with overflow:hidden) - so we can use them to detect overflow. It would be cleaner to use scroll-state container queries for this, but - browser support for scroll-driven animations is slightly better. */ - animation: detect-overflow linear; - animation-timeline: scroll(self block); - } - - /* After initial render, JS is used to control visibility which provides progressive enhancement for unsupported browsers */ - &[data-has-overflow='true'] { - --morebutton-display: flex; - } - - &:where([data-size='small']) { - --actionbar-height: var(--control-small-size); - } - - &:where([data-size='medium']) { - --actionbar-height: var(--control-medium-size); - } - - &:where([data-size='large']) { - --actionbar-height: var(--control-large-size); - } /* Gap scale (mirrors Stack) */ &:where([data-gap='none']) { @@ -51,20 +23,6 @@ &:where([data-gap='condensed']) { --actionbar-gap: var(--stack-gap-condensed); } - - & [data-overflowing] { - /* Hide overflowing items. Even though they are clipped by `overflow: hidden`, setting `visibility: hidden` ensures - they can't accidentally be shown and also hides them from screen readers / keyboard nav. `!important` prevents - consumers from unintentionally overriding this and breaking accessibility. */ - visibility: hidden !important; - } -} - -@keyframes detect-overflow { - 0%, - 100% { - --morebutton-display: flex; - } } .Nav { @@ -86,8 +44,6 @@ content: ''; /* stylelint-disable-next-line primer/colors */ background: var(--borderColor-muted); - /* stylelint-disable-next-line primer/spacing */ - margin-top: calc((var(--actionbar-height) - var(--base-size-20)) / 2); } } @@ -95,15 +51,3 @@ display: flex; gap: inherit; } - -.OverflowContainer { - display: flex; - flex-wrap: wrap; - gap: inherit; - justify-content: flex-end; - overflow: hidden; -} - -.MoreButton { - display: var(--morebutton-display, none); -} diff --git a/packages/react/src/ActionBar/ActionBar.test.tsx b/packages/react/src/ActionBar/ActionBar.test.tsx index 98fd2d4b777..bce7afda03f 100644 --- a/packages/react/src/ActionBar/ActionBar.test.tsx +++ b/packages/react/src/ActionBar/ActionBar.test.tsx @@ -228,14 +228,13 @@ describe('ActionBar Registry System', () => { render(
- +
, ) // Component should still render even with zero width - // Button is unlabeled because the label is hidden, so we select by test id instead - expect(screen.getByTestId('zero-width-button')).toBeInTheDocument() + expect(screen.getByRole('button', {name: 'Zero width button'})).toBeInTheDocument() }) it('should clean up registry on unmount', async () => { diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index c7ec993d031..a2c03125535 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -1,7 +1,10 @@ -import {type RefObject, type MouseEventHandler, useContext} from 'react' -import React, {useState, useCallback, useRef, forwardRef, useMemo, useSyncExternalStore} from 'react' +import type {RefObject, MouseEventHandler} from 'react' +import React, {useState, useCallback, useRef, forwardRef, useMemo} from 'react' import {KebabHorizontalIcon} from '@primer/octicons-react' import {ActionList, type ActionListItemProps} from '../ActionList' +import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' +import type {ResizeObserverEntry} from '../hooks/useResizeObserver' +import {useResizeObserver} from '../hooks/useResizeObserver' import type {IconButtonProps} from '../Button' import {IconButton} from '../Button' @@ -12,6 +15,8 @@ import {clsx} from 'clsx' import {useRefObjectAsForwardedRef} from '../hooks' import {createDescendantRegistry} from '../utils/descendant-registry' +const ACTIONBAR_ITEM_GAP = 8 + type ChildProps = | { type: 'action' @@ -19,21 +24,32 @@ type ChildProps = disabled: boolean icon: ActionBarIconButtonProps['icon'] onClick: MouseEventHandler + width: number + groupId?: string } - | {type: 'divider' | 'group'} + | {type: 'divider' | 'group'; width: number} | { type: 'menu' + width: number label: string icon: ActionBarIconButtonProps['icon'] | 'none' items: ActionBarMenuProps['items'] returnFocusRef?: React.RefObject } +/** + * Registry of descendants to render in the list or menu. To preserve insertion order across updates, children are + * set to `null` when unregistered rather than fully dropped from the map. + */ +type ChildRegistry = ReadonlyMap + const ActionBarContext = React.createContext<{ size: Size + isVisibleChild: (id: string) => boolean groupId?: string }>({ size: 'medium', + isVisibleChild: () => true, groupId: undefined, }) @@ -141,10 +157,30 @@ export type ActionBarMenuProps = { returnFocusRef?: React.RefObject } & IconButtonProps -const ActionBarItemsRegistry = createDescendantRegistry() - -const FOCUSABLE_ITEM_SELECTOR = - ':is(button, a, input, [tabindex]):not(:disabled):not([data-overflowing]):not([data-more-button-inactive])' +const MORE_BTN_WIDTH = 32 + +const ActionBarItemsRegistry = createDescendantRegistry() + +const calculatePossibleItems = ( + registryEntries: Array<[string, ChildProps]>, + navWidth: number, + gap: number, + moreMenuWidth = 0, +) => { + const widthToFit = navWidth - moreMenuWidth + let breakpoint = registryEntries.length // assume all items will fit + let sumsOfChildWidth = 0 + for (const [index, [, child]] of registryEntries.entries()) { + sumsOfChildWidth += index > 0 ? child.width + gap : child.width + if (sumsOfChildWidth > widthToFit) { + breakpoint = index + break + } else { + continue + } + } + return breakpoint +} const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.ReactNode => { if (item.type === 'divider') { @@ -195,152 +231,240 @@ const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.Reac ) } -export const ActionBar: React.FC> = ({ - size = 'medium', - children, - 'aria-label': ariaLabel, - 'aria-labelledby': ariaLabelledBy, - flush = false, - className, - gap = 'condensed', -}) => { - const [childRegistry, setChildRegistry] = ActionBarItemsRegistry.useRegistryState() +const getMenuItems = ( + navWidth: number, + moreMenuWidth: number, + childRegistry: ChildRegistry | undefined, + hasActiveMenu: boolean, + gap: number, +): Set | void => { + const registryEntries = Array.from(childRegistry?.entries() ?? []).filter( + (entry): entry is [string, ChildProps] => + entry[1] !== null && (entry[1].type !== 'action' || entry[1].groupId === undefined), + ) + + if (registryEntries.length === 0) return new Set() + const numberOfItemsPossible = calculatePossibleItems(registryEntries, navWidth, gap) - const overflowItems = useMemo( - () => - childRegistry && - Array.from(childRegistry.entries()).filter((entry): entry is [string, ChildProps] => entry[1] !== null), - [childRegistry], + const numberOfItemsPossibleWithMoreMenu = calculatePossibleItems( + registryEntries, + navWidth, + gap, + moreMenuWidth || MORE_BTN_WIDTH, ) + const menuItems = new Set() + + // First, we check if we can fit all the items with their icons + if (registryEntries.length >= numberOfItemsPossible) { + /* Below is an accessibility requirement. Never show only one item in the overflow menu. + * If there is only one item left to display in the overflow menu according to the calculation, + * we need to pull another item from the list into the overflow menu. + */ + const numberOfItemsInMenu = registryEntries.length - numberOfItemsPossibleWithMoreMenu + const numberOfListItems = + numberOfItemsInMenu === 1 ? numberOfItemsPossibleWithMoreMenu - 1 : numberOfItemsPossibleWithMoreMenu + for (const [index, [id, child]] of registryEntries.entries()) { + if (index < numberOfListItems) { + continue + //if the last item is a divider + } else if (child.type === 'divider') { + if (index === numberOfListItems - 1 || index === numberOfListItems) { + continue + } else { + menuItems.add(id) + } + } else { + menuItems.add(id) + } + } - const {containerRef} = useFocusZone( - { - bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd, - focusOutBehavior: 'wrap', - focusableElementFilter: element => element.matches(FOCUSABLE_ITEM_SELECTOR), + return menuItems + } else if (numberOfItemsPossible > registryEntries.length && hasActiveMenu) { + /* If the items fit in the list and there are items in the overflow menu, we need to move them back to the list */ + return new Set() + } +} + +export const ActionBar: React.FC> = props => { + const { + size = 'medium', + children, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledBy, + flush = false, + className, + gap = 'condensed', + } = props + + // We derive the numeric gap from computed style so layout math stays in sync with CSS + const listRef = useRef(null) + const [computedGap, setComputedGap] = useState(ACTIONBAR_ITEM_GAP) + + const [childRegistry, setChildRegistry] = ActionBarItemsRegistry.useRegistryState() + + const [menuItemIds, setMenuItemIds] = useState>(() => new Set()) + + const navRef = useRef(null) + // measure gap after first render & whenever gap scale changes + useIsomorphicLayoutEffect(() => { + if (!listRef.current) return + const g = window.getComputedStyle(listRef.current).gap + const parsed = parseFloat(g) + if (!Number.isNaN(parsed)) setComputedGap(parsed) + }, [gap]) + const moreMenuRef = useRef(null) + + useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { + const navWidth = resizeObserverEntries[0].contentRect.width + const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 + const hasActiveMenu = menuItemIds.size > 0 + + if (navWidth > 0) { + const newMenuItemIds = getMenuItems(navWidth, moreMenuWidth, childRegistry, hasActiveMenu, computedGap) + if (newMenuItemIds) setMenuItemIds(newMenuItemIds) + } + }, navRef as RefObject) + + const isVisibleChild = useCallback( + (id: string) => { + return !menuItemIds.has(id) }, - [overflowItems], + [menuItemIds], ) + useFocusZone({ + containerRef: listRef, + bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd, + focusOutBehavior: 'wrap', + }) + + const groupedItems = React.useMemo(() => { + const groupedItemsMap = new Map>() + + for (const [key, childProps] of childRegistry ?? []) { + if (childProps.type === 'action' && childProps.groupId) { + const existingGroup = groupedItemsMap.get(childProps.groupId) || [] + existingGroup.push([key, childProps]) + groupedItemsMap.set(childProps.groupId, existingGroup) + } + } + return groupedItemsMap + }, [childRegistry]) + return ( - -
+ +
} + ref={listRef} role="toolbar" className={styles.List} aria-label={ariaLabel} aria-labelledby={ariaLabelledBy} data-gap={gap} - data-size={size} - data-has-overflow={overflowItems ? overflowItems.length > 0 : undefined} > -
- {/* An empty first element allows the real first item to wrap to the next line and get clipped. */} -
- {children} -
- - - - - - - {overflowItems?.map(([id, menuItem]) => { - if (menuItem.type === 'divider') { - return - } - - if (menuItem.type === 'action') { - const {onClick, icon: Icon, label, disabled} = menuItem - return ( - { - typeof onClick === 'function' && onClick(event as React.MouseEvent) - }} - disabled={disabled} - > - - - - {label} - - ) - } - - if (menuItem.type === 'menu') { - const menuItems = menuItem.items - const {icon: Icon, label, returnFocusRef} = menuItem - - return ( - - - - {Icon !== 'none' ? ( - - - - ) : null} - {label} - - - - {menuItems.map((item, index) => renderMenuItem(item, index))} - - - ) - } - })} - - - + {children} + {menuItemIds.size > 0 && ( + + + + + + + {Array.from(menuItemIds).map(id => { + const menuItem = childRegistry?.get(id) + if (!menuItem) return null + + if (menuItem.type === 'divider') { + return + } else if (menuItem.type === 'action') { + const {onClick, icon: Icon, label, disabled} = menuItem + return ( + { + typeof onClick === 'function' && onClick(event as React.MouseEvent) + }} + disabled={disabled} + > + + + + {label} + + ) + } + + if (menuItem.type === 'menu') { + const menuItems = menuItem.items + const {icon: Icon, label, returnFocusRef} = menuItem + + return ( + + + + {Icon !== 'none' ? ( + + + + ) : null} + {label} + + + + {menuItems.map((item, index) => renderMenuItem(item, index))} + + + ) + } + + // Use the memoized map instead of filtering each time + const groupedMenuItems = groupedItems.get(id) || [] + + // If we ever add additional types, this condition will be necessary + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (menuItem.type === 'group') { + return ( + + {groupedMenuItems.map(([key, childProps]) => { + if (childProps.type === 'action') { + const {onClick, icon: Icon, label, disabled} = childProps + return ( + { + typeof onClick === 'function' && onClick(event as React.MouseEvent) + }} + disabled={disabled} + > + + + + {label} + + ) + } + return null + })} + + ) + } + })} + + + + )}
) } -function useActionBarItem(ref: React.RefObject, registryProps: ChildProps) { - const isGroupOverflowing = useContext(ActionBarGroupContext)?.isOverflowing - const isInGroup = isGroupOverflowing !== undefined - - const subscribeIntersectionObserver = useCallback( - (onChange: () => void) => { - // There's no need to register observers on items inside of a group - // since the entire group overflows at once - if (isInGroup) return () => {} +function useWidth(ref: React.RefObject) { + const [width, setWidth] = useState(-1) - const observer = new IntersectionObserver(() => onChange(), {threshold: 1}) + useIsomorphicLayoutEffect(() => setWidth(ref.current?.getBoundingClientRect().width ?? -1), [ref]) - if (ref.current) observer.observe(ref.current) - return () => observer.disconnect() - }, - [ref, isInGroup], - ) - - const isItemOverflowing = useSyncExternalStore( - subscribeIntersectionObserver, - // Note: the IntersectionObserver is just being used as a trigger to re-check - // `offsetTop > 0`; this is fast and simpler than checking visibility from - // the observed entry. When an item wraps, it will move to the next row which - // increases its `offsetTop` - () => (ref.current ? ref.current.offsetTop > 0 : false), - () => false, - ) - - const isOverflowing = isGroupOverflowing || isItemOverflowing - - ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) - - return {isOverflowing, dataOverflowingAttr: isOverflowing ? '' : undefined} + return width } export const ActionBarIconButton = forwardRef( @@ -348,24 +472,28 @@ export const ActionBarIconButton = forwardRef( const ref = useRef(null) useRefObjectAsForwardedRef(forwardedRef, ref) - const {size} = React.useContext(ActionBarContext) + const {size, isVisibleChild} = React.useContext(ActionBarContext) + const {groupId} = React.useContext(ActionBarGroupContext) + + const width = useWidth(ref) const {['aria-label']: ariaLabel, icon} = props - const {dataOverflowingAttr} = useActionBarItem( - ref, - useMemo( - (): ChildProps => ({ - type: 'action', - label: ariaLabel ?? '', - icon, - disabled: !!disabled, - onClick: onClick as MouseEventHandler, - }), - [ariaLabel, icon, disabled, onClick], - ), + const registryProps = useMemo( + (): ChildProps => ({ + type: 'action', + label: ariaLabel ?? '', + icon, + disabled: !!disabled, + onClick: onClick as MouseEventHandler, + width, + groupId: groupId ?? undefined, + }), + [ariaLabel, icon, disabled, onClick, groupId, width], ) + const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + const clickHandler = useCallback( (event: React.MouseEvent) => { if (disabled) return @@ -374,6 +502,8 @@ export const ActionBarIconButton = forwardRef( [disabled, onClick], ) + if (!isVisibleChild(id) || (groupId && !isVisibleChild(groupId))) return null + return ( ) }, ) const ActionBarGroupContext = React.createContext<{ - isOverflowing: boolean -} | null>(null) + groupId: string | null +}>({groupId: null}) export const ActionBarGroup = forwardRef(({children}: React.PropsWithChildren, forwardedRef) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject - const {dataOverflowingAttr, isOverflowing} = useActionBarItem( - ref, - useMemo((): ChildProps => ({type: 'group'}), []), - ) + const width = useWidth(ref) + + const registryProps = useMemo((): ChildProps => ({type: 'group', width}), [width]) + + const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) return ( - -
+ +
{children}
@@ -416,23 +546,28 @@ export const ActionBarMenu = forwardRef( ) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject + const {isVisibleChild} = React.useContext(ActionBarContext) const [menuOpen, setMenuOpen] = useState(false) - const {dataOverflowingAttr} = useActionBarItem( - ref, - useMemo( - (): ChildProps => ({ - type: 'menu', - label: ariaLabel, - icon: overflowIcon ? overflowIcon : icon, - returnFocusRef, - items, - }), - [ariaLabel, overflowIcon, icon, items, returnFocusRef], - ), + const width = useWidth(ref) + + const registryProps = useMemo( + (): ChildProps => ({ + type: 'menu', + width, + label: ariaLabel, + icon: overflowIcon ? overflowIcon : icon, + returnFocusRef, + items, + }), + [ariaLabel, overflowIcon, icon, items, returnFocusRef, width], ) + const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + + if (!isVisibleChild(id)) return null + return ( @@ -441,7 +576,6 @@ export const ActionBarMenu = forwardRef( aria-label={ariaLabel} icon={icon} {...props} - data-overflowing={dataOverflowingAttr} // overriding IconButton's data-component so that the ActionBar's "More Menu" Icon can be targeted specifically data-component="ActionBar.Menu.IconButton" /> @@ -456,18 +590,14 @@ export const ActionBarMenu = forwardRef( export const VerticalDivider = () => { const ref = useRef(null) - const {dataOverflowingAttr} = useActionBarItem( - ref, - useMemo((): ChildProps => ({type: 'divider'}), []), - ) + const {isVisibleChild} = React.useContext(ActionBarContext) - return ( -