chore: Replace focus strategy for video conf popups #36999
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds focus management to TimedVideoConfPopup, introduces lazy loading with Suspense and a skeleton fallback for VideoConfPopup, updates UI theme tokens, adjusts popup positioning and header min-height, adds a new VideoConfPopupSkeleton component and Storybook story, and updates ui-video-conf package scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant RV as RoomView
participant FS as FocusScope
participant S as Suspense
participant VCPS as VideoConfPopupSkeleton
participant VCP as VideoConfPopup (lazy)
participant TVP as TimedVideoConfPopup
U->>RV: Open video conf popup
RV->>FS: Render with restoreFocus
FS->>S: Render Suspense(fallback=VCPS)
Note over S,VCPS: While lazy module loads, show skeleton
S->>VCP: Lazy-load ./VideoConfPopup
VCP-->>S: Module resolved
S->>VCP: Render VideoConfPopup
VCP->>TVP: Render TimedVideoConfPopup
TVP->>TVP: useEffect -> focusManager.focusFirst()
Note over TVP: Initial focus moves to first focusable element
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36999 +/- ##
===========================================
- Coverage 66.35% 66.27% -0.08%
===========================================
Files 3391 3392 +1
Lines 115313 115379 +66
Branches 21137 21169 +32
===========================================
- Hits 76513 76466 -47
- Misses 36187 36297 +110
- Partials 2613 2616 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dougfabris
force-pushed
the
chore/videoconfpopup-focus
branch
from
2473b44 to
967ad80
Compare
MartinSchoeler
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.tsx (1)
29-33: Modal dialog should declare aria-modal and have a reliable accessible nameFor a modal dialog, expose modal semantics explicitly. Add aria-modal and rely on existing aria-label/aria-labelledby passthrough from props.
Apply this diff:
- <VideoConfPopupContainer role='dialog' ref={ref} position={position} {...props}> + <VideoConfPopupContainer role='dialog' aria-modal='true' ref={ref} position={position} {...props}>
🧹 Nitpick comments (9)
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.tsx (1)
19-23: Tighten props typing to the concrete elementUse HTMLAttributes to align with the rendered element.
-type VideoConfPopupProps = { +type VideoConfPopupProps = { children: ReactNode; position?: number; -} & HTMLAttributes<HTMLElement>; +} & HTMLAttributes<HTMLDivElement>;packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupHeader.tsx (1)
5-5: LGTM — minHeight improves layout stabilityOptional: add alignItems='center' to vertically center header controls if designs expect that.
- <Box display='flex' minHeight='x28' justifyContent='space-between'> + <Box display='flex' minHeight='x28' alignItems='center' justifyContent='space-between'>packages/ui-video-conf/src/VideoConfMessage/VideoConfMessageIcon.tsx (1)
27-36: Mark decorative icon as hidden from assistive tech (unless meant as status text)If the surrounding UI already conveys state textually, hide this icon from SRs.
- <Icon size='x20' name={styles[variant].icon} color={styles[variant].color} /> + <Icon size='x20' name={styles[variant].icon} color={styles[variant].color} aria-hidden='true' />If the icon is the only state indicator, instead provide an aria-label derived from variant.
packages/ui-video-conf/package.json (1)
11-14: Make build cleanup cross‑platformrm -rf breaks on Windows. Prefer rimraf (or shx) for portability.
- "build": "rm -rf dist && tsc -p tsconfig.build.json", + "build": "rimraf dist && tsc -p tsconfig.build.json",Add devDependency (or reuse a workspace tool if available):
"devDependencies": { + "rimraf": "^5.0.0",Please confirm CI runners aren’t Windows-only; otherwise the current script will fail there.
apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopup/TimedVideoConfPopup.tsx (1)
45-48: Stabilize initial focus timingA small defer can reduce races with late-mounted children (e.g., icons/images). Optional but tends to reduce flakiness.
- useEffect(() => { - focusManager?.focusFirst(); - }, [focusManager]); + useEffect(() => { + const raf = requestAnimationFrame(() => focusManager?.focusFirst()); + return () => cancelAnimationFrame(raf); + }, [focusManager]);apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopups.tsx (1)
51-56: Fix position calculation; default value on index is ignoredmap’s index starts at 0; the default index = 1 doesn’t apply. Use (index + 1) * 10 to offset the first popup too, and drop the default value on index.
- {(children ? [children, ...popups] : popups).map(({ id, rid, isReceiving }, index = 1) => ( + {(children ? [children, ...popups] : popups).map(({ id, rid, isReceiving }, index) => ( <VideoConfPopupBackdrop key={id}> - <Suspense fallback={<VideoConfPopupSkeleton />}> - <FocusScope restoreFocus> - <VideoConfPopup id={id} rid={rid} isReceiving={isReceiving} isCalling={isCalling} position={index * 10} /> - </FocusScope> - </Suspense> + {/* see focus trap change above */} + <VideoConfPopup id={id} rid={rid} isReceiving={isReceiving} isCalling={isCalling} position={(index + 1) * 10} /> </VideoConfPopupBackdrop> ))}packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupSkeleton.tsx (3)
12-12: A11y: Keep dialog name stable; prefer aria-busy over temporary aria-label.Using aria-label="Loading" changes the dialog’s accessible name while the fallback is shown. Better: keep the same name and mark the container busy.
Apply this diff:
- <VideoConfPopup aria-label='Loading' {...props}> + <VideoConfPopup {...props} aria-busy={true}>
19-19: Use design tokens for spacing (mis).Use Fuselage tokens for consistency with the rest of the sizes.
- <Skeleton mis={8} variant='rect' height='x24' width='x120' /> + <Skeleton mis='x8' variant='rect' height='x24' width='x120' />
9-10: Export the props type for consumers.Handy for typing wrappers/tests and keeps public API consistent with other components.
-type VideoConfPopupSkeletonProps = Omit<ComponentProps<typeof VideoConfPopup>, 'children'>; +export type VideoConfPopupSkeletonProps = Omit<ComponentProps<typeof VideoConfPopup>, 'children'>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
packages/ui-video-conf/src/VideoConfPopup/__snapshots__/VideoConfPopup.spec.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopup/TimedVideoConfPopup.tsx(2 hunks)apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopups.tsx(2 hunks)packages/ui-video-conf/package.json(1 hunks)packages/ui-video-conf/src/VideoConfMessage/VideoConfMessageIcon.tsx(2 hunks)packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.stories.tsx(2 hunks)packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.tsx(1 hunks)packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupHeader.tsx(1 hunks)packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupSkeleton.tsx(1 hunks)packages/ui-video-conf/src/VideoConfPopup/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.stories.tsx (1)
packages/ui-video-conf/src/VideoConfPopup/index.ts (2)
VideoConfPopup(14-14)VideoConfPopupSkeleton(24-24)
apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopups.tsx (1)
packages/ui-video-conf/src/VideoConfPopup/index.ts (2)
VideoConfPopup(14-14)VideoConfPopupSkeleton(24-24)
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupSkeleton.tsx (2)
packages/ui-video-conf/src/VideoConfPopup/index.ts (5)
VideoConfPopup(14-14)VideoConfPopupSkeleton(24-24)VideoConfPopupHeader(15-15)VideoConfPopupContent(18-18)VideoConfPopupFooter(22-22)packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.stories.tsx (1)
Skeleton(57-57)
🔇 Additional comments (6)
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.tsx (1)
6-17: Relative positioning change: verify stacking/offset behavior within the backdropSwitching to position: relative while still using top/left offsets can change layout flow and hit‑testing. Please validate across portals/overlays that:
- the popup remains above the backdrop content,
- offsets still produce the intended diagonal stacking,
- no unexpected layout shift occurs.
Add a z-index here if any layering issues appear.
packages/ui-video-conf/src/VideoConfMessage/VideoConfMessageIcon.tsx (1)
11-23: Theme token migration looks correctColor tokens map cleanly to status variants. No functional concerns.
packages/ui-video-conf/src/VideoConfPopup/index.ts (1)
10-10: LGTM — public export of the skeletonExporting VideoConfPopupSkeleton aligns the library with the new Suspense fallback use.
Also applies to: 24-24
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopup.stories.tsx (1)
57-58: LGTM — Skeleton storyGood to have parity with the runtime fallback.
apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopups.tsx (1)
11-12: Lazy + Suspense integration: looks goodThe lazy import and Suspense usage are correct. Once focus containment is restored, this should help with the flakiness goal.
To be safe, rerun the flaky test (FLAKY-1424) and a manual keyboard-only flow after the focus trap fix.
Also applies to: 16-16
packages/ui-video-conf/src/VideoConfPopup/VideoConfPopupSkeleton.tsx (1)
11-26: Verify focus behavior with the Suspense fallback.Ensure that while the skeleton is visible:
- Focus lands on the popup container (or remains on the trigger) without bouncing.
- When the real content mounts, focus reliably moves to the first focusable element via useFocusManager.
If helpful, I can draft a lightweight Playwright check that opens the popup, asserts activeElement on the container during fallback, then waits for content and asserts focus moved to the intended control.
apps/meteor/client/views/room/contextualBar/VideoConference/VideoConfPopups/VideoConfPopups.tsx
Show resolved
Hide resolved
Proposed changes (including videos or screenshots)
As soon the videoconf popup opens, the focus should be moved to the first focusable element on the popup. We use the
FocusScopefromreact-aria, which has theautoFocusto handle this automatically, but for some reason its not working consistently for this case. We do have an e2e test to ensure this behavior, but the test reveled its not working properly.After tons of attempts to find out why
FocusScopecan't add the focus properly, I decided to change the approach and dispatch the focus using theuseFocusManagerwhich showed itself more consistent, potentially removing the flakinessIssue(s)
Steps to test or reproduce
Further comments
FLAKY-1424
Summary by CodeRabbit