diff --git a/.changeset/many-suns-promise.md b/.changeset/many-suns-promise.md new file mode 100644 index 00000000000..6b465792c8e --- /dev/null +++ b/.changeset/many-suns-promise.md @@ -0,0 +1,5 @@ +--- +"@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 c39e4d1a9d0..4754c730059 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)).toHaveCount(10) + await expect(page.locator(toolbarButtonSelector).filter({visible: true})).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)).toHaveCount(8) + await expect(page.locator(toolbarButtonSelector).filter({visible: true})).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.module.css b/packages/react/src/ActionBar/ActionBar.module.css index 13aab764f53..507db13efaa 100644 --- a/packages/react/src/ActionBar/ActionBar.module.css +++ b/packages/react/src/ActionBar/ActionBar.module.css @@ -6,10 +6,34 @@ /* wonder why this is here */ /* stylelint-disable-next-line primer/spacing */ margin-bottom: -1px; - white-space: nowrap; list-style: none; - align-items: center; + align-items: flex-start; 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: block; + } + + &: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']) { @@ -23,6 +47,20 @@ &: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: block; + } } .Nav { @@ -44,6 +82,8 @@ 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); } } @@ -51,3 +91,15 @@ 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 f176d5e7add..ffc7b91956a 100644 --- a/packages/react/src/ActionBar/ActionBar.test.tsx +++ b/packages/react/src/ActionBar/ActionBar.test.tsx @@ -228,13 +228,14 @@ describe('ActionBar Registry System', () => { render(
- +
, ) // Component should still render even with zero width - expect(screen.getByRole('button', {name: 'Zero width button'})).toBeInTheDocument() + // Button is unlabeled because the label is hidden, so we select by test id instead + expect(screen.getByTestId('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 83712ce21fd..7bde0744554 100644 --- a/packages/react/src/ActionBar/ActionBar.tsx +++ b/packages/react/src/ActionBar/ActionBar.tsx @@ -1,10 +1,7 @@ -import type {RefObject, MouseEventHandler} from 'react' -import React, {useState, useCallback, useRef, forwardRef, useMemo} from 'react' +import {type RefObject, type MouseEventHandler, useContext} from 'react' +import React, {useState, useCallback, useRef, forwardRef, useMemo, useSyncExternalStore} 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' @@ -15,8 +12,6 @@ import {clsx} from 'clsx' import {useRefObjectAsForwardedRef} from '../hooks' import {createDescendantRegistry} from '../utils/descendant-registry' -const ACTIONBAR_ITEM_GAP = 8 - type ChildProps = | { type: 'action' @@ -24,32 +19,21 @@ type ChildProps = disabled: boolean icon: ActionBarIconButtonProps['icon'] onClick: MouseEventHandler - width: number - groupId?: string } - | {type: 'divider' | 'group'; width: number} + | {type: 'divider' | 'group'} | { 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, }) @@ -157,30 +141,7 @@ export type ActionBarMenuProps = { returnFocusRef?: React.RefObject } & IconButtonProps -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 ActionBarItemsRegistry = createDescendantRegistry() const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.ReactNode => { if (item.type === 'divider') { @@ -231,60 +192,6 @@ const renderMenuItem = (item: ActionBarMenuItemProps, index: number): React.Reac ) } -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 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) - } - } - - 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', @@ -298,39 +205,10 @@ export const ActionBar: React.FC> = prop // 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) - }, - [menuItemIds], - ) useFocusZone({ containerRef: listRef, @@ -338,21 +216,12 @@ export const ActionBar: React.FC> = prop 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]) + const overflowItems = + childRegistry && + Array.from(childRegistry.entries()).filter((entry): entry is [string, ChildProps] => entry[1] !== null) return ( - +
> = prop aria-label={ariaLabel} aria-labelledby={ariaLabelledBy} data-gap={gap} + data-size={size} + data-has-overflow={overflowItems ? overflowItems.length > 0 : undefined} > - {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 - })} - - ) - } - })} - - - - )} +
+ {/* 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))} + + + ) + } + })} + + +
) } -function useWidth(ref: React.RefObject) { - const [width, setWidth] = useState(-1) +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 () => {} - useIsomorphicLayoutEffect(() => setWidth(ref.current?.getBoundingClientRect().width ?? -1), [ref]) + const observer = new IntersectionObserver(() => onChange(), {threshold: 1}) - return width + 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} } export const ActionBarIconButton = forwardRef( @@ -472,28 +344,24 @@ export const ActionBarIconButton = forwardRef( const ref = useRef(null) useRefObjectAsForwardedRef(forwardedRef, ref) - const {size, isVisibleChild} = React.useContext(ActionBarContext) - const {groupId} = React.useContext(ActionBarGroupContext) - - const width = useWidth(ref) + const {size} = React.useContext(ActionBarContext) const {['aria-label']: ariaLabel, icon} = props - 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 {dataOverflowingAttr} = useActionBarItem( + ref, + useMemo( + (): ChildProps => ({ + type: 'action', + label: ariaLabel ?? '', + icon, + disabled: !!disabled, + onClick: onClick as MouseEventHandler, + }), + [ariaLabel, icon, disabled, onClick], + ), ) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) - const clickHandler = useCallback( (event: React.MouseEvent) => { if (disabled) return @@ -502,8 +370,6 @@ export const ActionBarIconButton = forwardRef( [disabled, onClick], ) - if (!isVisibleChild(id) || (groupId && !isVisibleChild(groupId))) return null - return ( ) }, ) const ActionBarGroupContext = React.createContext<{ - groupId: string | null -}>({groupId: null}) + isOverflowing: boolean +} | null>(null) export const ActionBarGroup = forwardRef(({children}: React.PropsWithChildren, forwardedRef) => { const backupRef = useRef(null) const ref = (forwardedRef ?? backupRef) as RefObject - const width = useWidth(ref) - - const registryProps = useMemo((): ChildProps => ({type: 'group', width}), [width]) - - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + const {dataOverflowingAttr, isOverflowing} = useActionBarItem( + ref, + useMemo((): ChildProps => ({type: 'group'}), []), + ) return ( - -
+ +
{children}
@@ -546,32 +412,33 @@ 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 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 {dataOverflowingAttr} = useActionBarItem( + ref, + useMemo( + (): ChildProps => ({ + type: 'menu', + label: ariaLabel, + icon: overflowIcon ? overflowIcon : icon, + returnFocusRef, + items, + }), + [ariaLabel, overflowIcon, icon, items, returnFocusRef], + ), ) - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) - - if (!isVisibleChild(id)) return null - return ( - + {items.map((item, index) => renderMenuItem(item, index))} @@ -583,14 +450,18 @@ export const ActionBarMenu = forwardRef( export const VerticalDivider = () => { const ref = useRef(null) - const {isVisibleChild} = React.useContext(ActionBarContext) - - const width = useWidth(ref) - - const registryProps = useMemo((): ChildProps => ({type: 'divider', width}), [width]) - - const id = ActionBarItemsRegistry.useRegisterDescendant(registryProps) + const {dataOverflowingAttr} = useActionBarItem( + ref, + useMemo((): ChildProps => ({type: 'divider'}), []), + ) - if (!isVisibleChild(id)) return null - return