fix: Update flashing issue in popups and rtl placement#3729
fix: Update flashing issue in popups and rtl placement#3729mannycarrera4 wants to merge 10 commits intoWorkday:masterfrom
Conversation
| } | ||
| // No cleanup is needed if element or theme is not set, so return undefined (no effect) | ||
| return undefined; | ||
| }, [localRef, style, theme]); |
There was a problem hiding this comment.
This layout effect was happening too late, causing menu and select to flash on first render on the first focusable element. Moved this further up in the first uselayout effect
| </Popup> | ||
| <div style={{display: 'flex', justifyContent: 'flex-start'}}> | ||
| <Popup model={model}> | ||
| <Popup.Target style={{visibility: 'hidden'}}></Popup.Target> |
There was a problem hiding this comment.
when the target is hidden, it knows how to correctly position itself.
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
mc-fix-popper-issues
|
| Run status |
|
| Run duration | 02m 22s |
| Commit |
|
| Committer | Manuel Carrera |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
86
|
Skipped
|
0
|
Passing
|
862
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.36%
|
|
|---|---|
|
Untested elements
|
1539
|
|
Tested elements
|
367
|
Accessibility
99.33%
|
|
|---|---|
|
Failed rules
|
6 critical
5 serious
0 moderate
2 minor
|
|
Failed elements
|
77
|
…a4/canvas-kit into mc-fix-popper-issues
There was a problem hiding this comment.
Pull request overview
Fixes visual flashing in popup-driven components by adjusting focus/positioning timing, and adds RTL-aware placement handling so popups flip left/right (and start/end) correctly when rendered in RTL contexts.
Changes:
- Add RTL-aware placement/fallback placement flipping in
Popperusingdirfrom the popup stack container. - Adjust
MenuItemfocus class application timing to avoid premature focus styling during initial positioning. - Update RTL visual test story layout/target visibility to better exercise RTL placement behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/react/popup/stories/visual-testing/Popup.stories.tsx | Tweaks RTL visual test layout/target visibility for more reliable RTL placement screenshots. |
| modules/react/popup/lib/Popper.tsx | Adds RTL placement + fallback placement flipping and applies it during Popper create/update. |
| modules/react/menu/lib/MenuItem.tsx | Delays applying the focus class until after deferred focus to reduce initial style flash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| React.useLayoutEffect(() => { | ||
| if (model.state.mode === 'single') { | ||
| if (isCursor(model.state, id)) { | ||
| // delay focus changes to allow PopperJS to position | ||
| requestAnimationFrame(() => { | ||
| localRef.current?.focus(); | ||
| setCanShowFocus(true); | ||
| }); |
There was a problem hiding this comment.
requestAnimationFrame callback calls setCanShowFocus(true) but the effect doesn’t cancel the RAF on cleanup. If the menu closes/unmounts before the frame fires, this can set state on an unmounted component (React warning) and potentially leak work. Store the RAF id and cancel it in the effect cleanup (and/or guard with a mounted ref).
| // 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; | ||
|
|
There was a problem hiding this comment.
RTL placement flipping is new behavior in Popper, but there are existing unit tests for Popper behavior. Please add test coverage to assert that in an RTL container the passed placement and fallbackPlacements are flipped as expected (including *-start/*-end), and that the render-prop placement reflects the RTL-aware placement.
mannycarrera4
added
on hold
2 participants
Summary
Fixes: #3712,
Release Category
Components
Release Note
usePopupStackhad two layout effects. The second one was called too late in the render, causing a flashing of styles in components like Select and Menu. The Popup also didn't account for switching placement whenrtlis set. These fixes to handle forwardingrltand switching placement of the popup.Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)