From 05f2f939b3c343e611c816b6570c8fdf3c104ee6 Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Fri, 30 Jan 2026 09:35:01 -0700 Subject: [PATCH 1/6] fix: Update flashing issue in popups and rtl placement --- modules/react/popup/lib/Popper.tsx | 52 ++++++++++++++++--- .../react/popup/lib/hooks/usePopupStack.ts | 49 ++++++++++------- .../react/popup/stories/examples/Basic.tsx | 42 ++++++++------- .../stories/visual-testing/Popup.stories.tsx | 22 ++++---- 4 files changed, 110 insertions(+), 55 deletions(-) diff --git a/modules/react/popup/lib/Popper.tsx b/modules/react/popup/lib/Popper.tsx index 38c25723db..ccca9f834e 100644 --- a/modules/react/popup/lib/Popper.tsx +++ b/modules/react/popup/lib/Popper.tsx @@ -13,9 +13,25 @@ export type PopperOptions = Options; export const defaultFallbackPlacements: Placement[] = ['top', 'right', 'bottom', 'left']; import {usePopupStack} from './hooks'; -import {useLocalRef} from '@workday/canvas-kit-react/common'; +import {isElementRTL, useLocalRef} from '@workday/canvas-kit-react/common'; import {fallbackPlacementsModifier} from './fallbackPlacements'; +/** + * Flips a placement for RTL layouts. Popper.js doesn't automatically flip + * placements based on dir attribute, so we need to do it manually. In RTL: + * - `left` ↔ `right` + * - `-start` ↔ `-end` + */ +const flipPlacementForRTL = (placement: Placement): Placement => { + return placement + .replace(/left/g, '__LEFT__') + .replace(/right/g, 'left') + .replace(/__LEFT__/g, 'right') + .replace(/-start/g, '__START__') + .replace(/-end/g, '-start') + .replace(/__START__/g, '-end') as Placement; +}; + export interface PopperProps { /** * The reference element used to position the Popper. Popper content will try to follow the @@ -172,16 +188,25 @@ const OpenPopper = React.forwardRef( return undefined; } + // Check RTL from the popup container (stackRef) to flip placements. + // Popper.js doesn't automatically flip left/right placements in RTL mode. + // We use stackRef because it has the dir attribute set by usePopupStack. + const isRTL = stackRef.current ? isElementRTL(stackRef.current) : false; + const rtlAwarePlacement = isRTL ? flipPlacementForRTL(popperPlacement) : popperPlacement; + const rtlAwareFallbackPlacements = isRTL + ? fallbackPlacements.map(flipPlacementForRTL) + : fallbackPlacements; + if (stackRef.current) { const instance = createPopper(anchorEl, stackRef.current, { - placement: popperPlacement, + placement: rtlAwarePlacement, ...popperOptions, modifiers: [ placementModifier, { ...fallbackPlacementsModifier, options: { - fallbackPlacements, + fallbackPlacements: rtlAwareFallbackPlacements, }, }, ...(popperOptions.modifiers || []), @@ -204,15 +229,23 @@ const OpenPopper = React.forwardRef( React.useLayoutEffect(() => { // Only update options if this is _not_ the first render if (!firstRender.current) { + // Check RTL from the popup container to flip placements. + const popperElement = localRef.current?.state?.elements?.popper; + const isRTL = popperElement ? isElementRTL(popperElement) : false; + const rtlAwarePlacement = isRTL ? flipPlacementForRTL(popperPlacement) : popperPlacement; + const rtlAwareFallbackPlacements = isRTL + ? fallbackPlacements.map(flipPlacementForRTL) + : fallbackPlacements; + localRef.current?.setOptions({ - placement: popperPlacement, + placement: rtlAwarePlacement, ...popperOptions, modifiers: [ placementModifier, { ...fallbackPlacementsModifier, options: { - fallbackPlacements, + fallbackPlacements: rtlAwareFallbackPlacements, }, }, ...(popperOptions.modifiers || []), @@ -220,7 +253,14 @@ const OpenPopper = React.forwardRef( }); } firstRender.current = false; - }, [popperOptions, popperPlacement, fallbackPlacements, placementModifier, localRef]); + }, [ + popperOptions, + popperPlacement, + fallbackPlacements, + anchorElement, + placementModifier, + localRef, + ]); const contents = <>{isRenderProp(children) ? children({placement}) : children}; diff --git a/modules/react/popup/lib/hooks/usePopupStack.ts b/modules/react/popup/lib/hooks/usePopupStack.ts index b688c4bdc7..8e4e8457a4 100644 --- a/modules/react/popup/lib/hooks/usePopupStack.ts +++ b/modules/react/popup/lib/hooks/usePopupStack.ts @@ -81,11 +81,40 @@ export const usePopupStack = ( : undefined; const element = localRef.current!; + // Set dir attribute so Popper.js reads the correct direction when it initializes. + // When there's no target (e.g. consumer doesn't use Popup.Target), find the nearest + // element with a `dir` attribute: start from the focused element (the trigger) or body, + // then use closest('[dir]'). Prefer reading getAttribute('dir') when present to avoid + // getComputedStyle. + if (typeof document !== 'undefined') { + let elementToCheck: Element | undefined = targetEl ?? undefined; + if (elementToCheck == null) { + const active = document.activeElement; + const start = active && !element.contains(active) ? active : document.body; + elementToCheck = start.closest('[dir]') ?? document.documentElement; + } + // Find closest ancestor with dir attribute, or use document element + elementToCheck = elementToCheck.closest('[dir]') ?? document.documentElement; + const explicitDir = elementToCheck.getAttribute('dir'); + const isRTL = + explicitDir != null ? explicitDir.toLowerCase() === 'rtl' : isElementRTL(elementToCheck); + element.setAttribute('dir', isRTL ? 'rtl' : 'ltr'); + } + + // Set theme CSS variables to prevent flashing of styles + const keys = Object.keys(style); + if (theme) { + for (const key of keys) { + // @ts-ignore + element.style.setProperty(key, style[key]); + } + } + PopupStack.add({element: element, owner: targetEl}); return () => { PopupStack.remove(element); }; - }, [localRef, target, popupRef]); + }, [localRef, target, popupRef, style, theme]); // The direction will properly follow the theme via React context, but portals lose the `dir` // hierarchy, so we'll add it back here. When there's no target (e.g. consumer doesn't use @@ -113,23 +142,5 @@ export const usePopupStack = ( } }, [localRef, target]); - React.useLayoutEffect(() => { - const element = localRef.current; - const keys = Object.keys(style); - if (element && theme) { - for (const key of keys) { - // @ts-ignore - element.style.setProperty(key, style[key]); - } - return () => { - for (const key of keys) { - element.style.removeProperty(key); - } - }; - } - // No cleanup is needed if element or theme is not set, so return undefined (no effect) - return undefined; - }, [localRef, style, theme]); - return localRef; }; diff --git a/modules/react/popup/stories/examples/Basic.tsx b/modules/react/popup/stories/examples/Basic.tsx index 818940bed5..ef1b7258ad 100644 --- a/modules/react/popup/stories/examples/Basic.tsx +++ b/modules/react/popup/stories/examples/Basic.tsx @@ -22,25 +22,27 @@ export const Basic = () => { }; return ( - - Delete Item - - - - Delete Item - - - Are you sure you'd like to delete the item titled 'My Item'? - - - - - Delete - - Cancel - - - - +
+ + Delete Item + + + + Delete Item + + + Are you sure you'd like to delete the item titled 'My Item'? + + + + + Delete + + Cancel + + + + +
); }; diff --git a/modules/react/popup/stories/visual-testing/Popup.stories.tsx b/modules/react/popup/stories/visual-testing/Popup.stories.tsx index cdf930f612..2b5e4018cd 100644 --- a/modules/react/popup/stories/visual-testing/Popup.stories.tsx +++ b/modules/react/popup/stories/visual-testing/Popup.stories.tsx @@ -148,16 +148,18 @@ export const PopupRTL = { }); return ( - - - - - - למחוק פריט - האם ברצונך למחוק פריט זה - - - +
+ + + + + + למחוק פריט + האם ברצונך למחוק פריט זה + + + +
); }, From cf6c6fb1332caa7e38498a413578e371264616c7 Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Mon, 2 Feb 2026 10:02:59 -0700 Subject: [PATCH 2/6] fix: Fix menu item flashing --- modules/react/menu/lib/MenuItem.tsx | 6 +- modules/react/menu/stories/examples/Basic.tsx | 36 +++++------ .../react/popup/lib/hooks/usePopupStack.ts | 61 ++++++++++--------- 3 files changed, 55 insertions(+), 48 deletions(-) diff --git a/modules/react/menu/lib/MenuItem.tsx b/modules/react/menu/lib/MenuItem.tsx index 07d5b8e451..8e97bb3a11 100644 --- a/modules/react/menu/lib/MenuItem.tsx +++ b/modules/react/menu/lib/MenuItem.tsx @@ -184,6 +184,8 @@ export const useMenuItemFocus = createElemPropsHook(useMenuModel)( (model, ref, elemProps: {'data-id': string} = {'data-id': ''}) => { const {localRef, elementRef} = useLocalRef(ref as React.Ref); const id = elemProps['data-id']; + const [canShowFocus, setCanShowFocus] = React.useState(false); + // focus on the item with the cursor React.useLayoutEffect(() => { if (model.state.mode === 'single') { @@ -191,14 +193,16 @@ export const useMenuItemFocus = createElemPropsHook(useMenuModel)( // delay focus changes to allow PopperJS to position requestAnimationFrame(() => { localRef.current?.focus(); + setCanShowFocus(true); }); } } // eslint-disable-next-line react-hooks/exhaustive-deps }, [id, localRef, model.state.cursorId, model.state.mode]); + return { ref: elementRef, - className: isCursor(model.state, elemProps['data-id']) ? 'focus' : undefined, + className: canShowFocus && isCursor(model.state, elemProps['data-id']) ? 'focus' : undefined, }; } ); diff --git a/modules/react/menu/stories/examples/Basic.tsx b/modules/react/menu/stories/examples/Basic.tsx index 91d72f97c1..0718ea3dec 100644 --- a/modules/react/menu/stories/examples/Basic.tsx +++ b/modules/react/menu/stories/examples/Basic.tsx @@ -6,22 +6,24 @@ import {BodyText} from '@workday/canvas-kit-react/text'; export const Basic = () => { const [selected, setSelected] = React.useState(''); return ( - setSelected(data.id)}> - Open Menu - - - - First Item - Second Item - - Third Item (with a really, really, really long label) - Fourth Item - - - - - Selected: {selected} - - +
+ setSelected(data.id)}> + Open Menu + + + + First Item + Second Item + + Third Item (with a really, really, really long label) + Fourth Item + + + + + Selected: {selected} + + +
); }; diff --git a/modules/react/popup/lib/hooks/usePopupStack.ts b/modules/react/popup/lib/hooks/usePopupStack.ts index 8e4e8457a4..c3639035b8 100644 --- a/modules/react/popup/lib/hooks/usePopupStack.ts +++ b/modules/react/popup/lib/hooks/usePopupStack.ts @@ -65,6 +65,18 @@ export const usePopupStack = ( } return localRef.current; }); + + // Set CSS variables during render (before any effects) to prevent flashing + // This is safe because setting style properties is idempotent + if (localRef.current && theme) { + const element = localRef.current; + const keys = Object.keys(style); + for (const key of keys) { + // @ts-ignore + element.style.setProperty(key, style[key]); + } + } + // We useLayoutEffect to ensure proper timing of registration of the element to the popup stack. // Without this, the timing is unpredictable when mixed with other frameworks. Other frameworks // should also register as soon as the element is available @@ -81,40 +93,11 @@ export const usePopupStack = ( : undefined; const element = localRef.current!; - // Set dir attribute so Popper.js reads the correct direction when it initializes. - // When there's no target (e.g. consumer doesn't use Popup.Target), find the nearest - // element with a `dir` attribute: start from the focused element (the trigger) or body, - // then use closest('[dir]'). Prefer reading getAttribute('dir') when present to avoid - // getComputedStyle. - if (typeof document !== 'undefined') { - let elementToCheck: Element | undefined = targetEl ?? undefined; - if (elementToCheck == null) { - const active = document.activeElement; - const start = active && !element.contains(active) ? active : document.body; - elementToCheck = start.closest('[dir]') ?? document.documentElement; - } - // Find closest ancestor with dir attribute, or use document element - elementToCheck = elementToCheck.closest('[dir]') ?? document.documentElement; - const explicitDir = elementToCheck.getAttribute('dir'); - const isRTL = - explicitDir != null ? explicitDir.toLowerCase() === 'rtl' : isElementRTL(elementToCheck); - element.setAttribute('dir', isRTL ? 'rtl' : 'ltr'); - } - - // Set theme CSS variables to prevent flashing of styles - const keys = Object.keys(style); - if (theme) { - for (const key of keys) { - // @ts-ignore - element.style.setProperty(key, style[key]); - } - } - PopupStack.add({element: element, owner: targetEl}); return () => { PopupStack.remove(element); }; - }, [localRef, target, popupRef, style, theme]); + }, [localRef, target, popupRef]); // The direction will properly follow the theme via React context, but portals lose the `dir` // hierarchy, so we'll add it back here. When there's no target (e.g. consumer doesn't use @@ -142,5 +125,23 @@ export const usePopupStack = ( } }, [localRef, target]); + React.useLayoutEffect(() => { + const element = localRef.current; + const keys = Object.keys(style); + if (element && theme) { + for (const key of keys) { + // @ts-ignore + element.style.setProperty(key, style[key]); + } + return () => { + for (const key of keys) { + element.style.removeProperty(key); + } + }; + } + // No cleanup is needed if element or theme is not set, so return undefined (no effect) + return undefined; + }, [localRef, style, theme]); + return localRef; }; From d748bc782b26bc213aac8d8d4259f69d88222848 Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Mon, 2 Feb 2026 10:03:38 -0700 Subject: [PATCH 3/6] fix: Undo storybook change --- modules/react/menu/stories/examples/Basic.tsx | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/modules/react/menu/stories/examples/Basic.tsx b/modules/react/menu/stories/examples/Basic.tsx index 0718ea3dec..91d72f97c1 100644 --- a/modules/react/menu/stories/examples/Basic.tsx +++ b/modules/react/menu/stories/examples/Basic.tsx @@ -6,24 +6,22 @@ import {BodyText} from '@workday/canvas-kit-react/text'; export const Basic = () => { const [selected, setSelected] = React.useState(''); return ( -
- setSelected(data.id)}> - Open Menu - - - - First Item - Second Item - - Third Item (with a really, really, really long label) - Fourth Item - - - - - Selected: {selected} - - -
+ setSelected(data.id)}> + Open Menu + + + + First Item + Second Item + + Third Item (with a really, really, really long label) + Fourth Item + + + + + Selected: {selected} + + ); }; From fcd715f5274ad8f85081c05396e3c60543ac8fc8 Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Mon, 2 Feb 2026 10:07:08 -0700 Subject: [PATCH 4/6] fix: Update popupstack --- modules/react/popup/lib/hooks/usePopupStack.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/modules/react/popup/lib/hooks/usePopupStack.ts b/modules/react/popup/lib/hooks/usePopupStack.ts index c3639035b8..b688c4bdc7 100644 --- a/modules/react/popup/lib/hooks/usePopupStack.ts +++ b/modules/react/popup/lib/hooks/usePopupStack.ts @@ -65,18 +65,6 @@ export const usePopupStack = ( } return localRef.current; }); - - // Set CSS variables during render (before any effects) to prevent flashing - // This is safe because setting style properties is idempotent - if (localRef.current && theme) { - const element = localRef.current; - const keys = Object.keys(style); - for (const key of keys) { - // @ts-ignore - element.style.setProperty(key, style[key]); - } - } - // We useLayoutEffect to ensure proper timing of registration of the element to the popup stack. // Without this, the timing is unpredictable when mixed with other frameworks. Other frameworks // should also register as soon as the element is available From 2bcd58a51ba7d38966307b1f09dbce957e5c936c Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Mon, 2 Feb 2026 10:17:57 -0700 Subject: [PATCH 5/6] fix: Undo story change --- .../react/popup/stories/examples/Basic.tsx | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/modules/react/popup/stories/examples/Basic.tsx b/modules/react/popup/stories/examples/Basic.tsx index ef1b7258ad..818940bed5 100644 --- a/modules/react/popup/stories/examples/Basic.tsx +++ b/modules/react/popup/stories/examples/Basic.tsx @@ -22,27 +22,25 @@ export const Basic = () => { }; return ( -
- - Delete Item - - - - Delete Item - - - Are you sure you'd like to delete the item titled 'My Item'? - - - - - Delete - - Cancel - - - - -
+ + Delete Item + + + + Delete Item + + + Are you sure you'd like to delete the item titled 'My Item'? + + + + + Delete + + Cancel + + + + ); }; From a80be20bcd85d2f7f84c9721e2d223f76f3b1d45 Mon Sep 17 00:00:00 2001 From: "manuel.carrera" Date: Thu, 19 Feb 2026 08:09:44 -0700 Subject: [PATCH 6/6] fix: Update select --- modules/react/menu/lib/MenuItem.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/modules/react/menu/lib/MenuItem.tsx b/modules/react/menu/lib/MenuItem.tsx index 8e97bb3a11..fcfd83b425 100644 --- a/modules/react/menu/lib/MenuItem.tsx +++ b/modules/react/menu/lib/MenuItem.tsx @@ -184,7 +184,6 @@ export const useMenuItemFocus = createElemPropsHook(useMenuModel)( (model, ref, elemProps: {'data-id': string} = {'data-id': ''}) => { const {localRef, elementRef} = useLocalRef(ref as React.Ref); const id = elemProps['data-id']; - const [canShowFocus, setCanShowFocus] = React.useState(false); // focus on the item with the cursor React.useLayoutEffect(() => { @@ -193,7 +192,6 @@ export const useMenuItemFocus = createElemPropsHook(useMenuModel)( // delay focus changes to allow PopperJS to position requestAnimationFrame(() => { localRef.current?.focus(); - setCanShowFocus(true); }); } } @@ -202,7 +200,7 @@ export const useMenuItemFocus = createElemPropsHook(useMenuModel)( return { ref: elementRef, - className: canShowFocus && isCursor(model.state, elemProps['data-id']) ? 'focus' : undefined, + className: isCursor(model.state, elemProps['data-id']) ? 'focus' : undefined, }; } );