Polish media controls, panel headers, and menus - #496
Conversation
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
|
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
One behavioral fix requested inline: allow opening the volume slider without changing mute state. The nested-animation note is optional.
Star Lord automated source review via Wes’s account; head f437cc347e6d385af7ae626dab27604641a7de76, base 0124f3fdfc9ece433d1ecb71aa43f2a378980f19. No code, tests, or app executed. Hosted CI snapshot passed; Windows native validation was skipped. Four supplied screenshots were inspected; the message-menu and refined volume-capsule attachments could not be inspected (malformed link/404). Native titlebar dragging and packaged desktop behavior remain unverified.
| onClick={(event) => { | ||
| const video = videoRef.current; | ||
| if (!video) return; | ||
| setVolumeMotion(event.detail > 0); | ||
| video.muted = !(muted || volume === 0); | ||
| if (!video.muted && video.volume === 0) video.volume = 1; |
There was a problem hiding this comment.
P2 — Keep volume disclosure independent of mute. With an unmuted video at 75%, keyboard activation of this trigger both opens the slider and sets muted = true; the slider therefore starts at zero instead of 75%. ArrowDown cannot lower the current level, and Escape leaves playback muted. Hover users can open it without changing audio, but keyboard/touch users have no equivalent path now that the independent volume Tab stop is gone. Preserve a non-mutating keyboard/touch way to open volume controls (or separate the mute and disclosure actions). Extend the keyboard-opening test at tests/browser/media-controls.spec.mjs:301–306 to assert the starting level/mute state, one downward adjustment, and Escape preservation.
There was a problem hiding this comment.
🤖 Addressed in ceda768, with follow-up overlay regression coverage in a9023a7.
Opening “Video volume” with keyboard, touch, or pointer now preserves the current volume/mute state. Mute/Unmute is a separate action inside the popup. Regression coverage checks opening at 75%, ArrowDown adjustment, Escape preservation/focus, and popup containment at constrained sizes.
The three affected browser files passed 44 Chromium/WebKit executions with no retries. The human tested and accepted the keyboard fix. Required CI and DCO pass at a9023a7.
| {...(onMarker ? { onMarker } : {})} | ||
| /> | ||
| {children} | ||
| <div className={styles.reviewControls} data-review-chrome=""> |
There was a problem hiding this comment.
Optional — Avoid animating nested review chrome twice. This new wrapper and its VideoControls/VideoReviewReactions children all carry data-review-chrome. use-review-entrance.ts:245–256 animates every match, so the children inherit the wrapper’s fade/6px translation in addition to their own (and opacity compounds again on close). Consider keeping a single animation owner for the capsule, e.g. the wrapper. This is cosmetic, not a correctness blocker.
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
Signed-off-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
No merge-blocking findings at a9023a72249c4aeb7e12c08e584b0a79b748e94e against e5a70460a9a3423464e7ee97dede8ccecfd69f10.
- The previous volume-disclosure defect is repaired: opening preserves audio; mute is an explicit separate action. Passing Chromium/WebKit regressions cover keyboard/touch disclosure, first-step volume adjustment, dismissal/focus restoration, and constrained popup containment. The existing nested-chrome animation suggestion remains optional.
- Inspected hosted CI logs and downloaded reports: 6,321 JavaScript tests and 1,096 Chromium/WebKit journey executions passed, with no browser retries, skips, or flaky results. These jobs tested synthetic merge
3a1e6dfb043ef53a1a32b320e2130b48f5867112of the reviewed head into the stated base, not the raw head alone. - Source, test, and screenshot review found no additional blocker in media/modal integration, image zoom/pan, panel close/focus, tabs, or shared headers. Independent playback and shared-UI reviews also found no blockers; the playback reviewer additionally reported successful targeted Chromium/WebKit checks of volume dragging outside the popup and speed-menu Escape handling. I ran no fresh local suite or packaged-app check. Native titlebar dragging and packaged desktop behavior remain unverified; Windows native validation was skipped. The PR records human acceptance of the UI and volume repair, not native validation.
No code correction is required by this review. Approval/merge remains a human decision, subject to required branch approvals and acceptance of the native-validation limits; this is a comment, not an approval.
Brings in #496 (polish for media controls, panel headers and menus): 60 files. None are under src/features/relay, and in the two sidebar components it touches (ChannelSidebar.tsx, SidebarSection.tsx) it adds menu icons only. No hand resolution. Six files change on both sides and git merged each one cleanly: ChannelSidebar.tsx and MessageManagement.tsx (main adds menu icons), and the channel-tabs, message-actions-floating, sidenav-polish and thread-video browser specs. In each of the six, this branch's change and main's change are line for line what they were before the merge. #496 also edits message-overlays.spec.mjs and VideoPlayer.tsx, where message-overlays.spec.mjs:385 failed on hosted WebKit at 4b814f5. This branch changes neither file, so main's versions come in as they are. Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
…page-icon * origin/main: Count unread replies only in conversations you are part of (block#471) Animate the terminal welcome with a compact hex wordmark (block#508) Use top tabs in the new-tab picker (block#505) Polish media controls, panel headers, and menus (block#496) harden pinned browser CI setup and native fixture provenance (block#494) perf(relay): confirm membership hints with exact channel reads (block#486) test(browser): wait for menu and wheel completion (block#492) fix(links): render one hash on completed channel links (block#506) ci: publish Windows and Linux alongside macOS previews (block#491) fix(channels): keep conversations open through archive and restore (block#452) feat(channels): align create and edit forms with draft protection (block#482) Test provider connections before model selection (block#500) Signed-off-by: Matthew Boston <mboston@squareup.com>
Summary
Make media controls, panel headers, tabs, and menus consistent with the shared design system.
Validation
Current head:
a9023a72249c4aeb7e12c08e584b0a79b748e94e.media-controls.spec.mjsandmessages.spec.mjs: 30/30 Chromium/WebKit executions passed on macOS, zero retries; 90s wall time and 145.552s summed test execution.message-overlays.spec.mjs: 14/14 Chromium/WebKit executions passed in 17.3s, zero retries, retaining complete horizontal containment and center hit-testing for the disclosure and popup mute action at both 1024px and 800px widths with enlarged text.ChannelMembersDialog, 28.72s; slowest test: read-state durable-owner behavior, 8.54s. Native job: 419s. No timeout or workflow changes were needed.Browser regression coverage
Overall PR: 9 scenarios added, 0 removed. They cover rendered geometry/focus, native keyboard and touch input, hover popovers, animation/reduced motion, seek positioning, and crowded-tab layout. State-only playback/zoom behavior remains covered by component tests. Existing journeys and both engines remain in place.
The latest repair adds one touch scenario and extends existing keyboard coverage. Real browser touch activation, focus/dismissal, and popup geometry require the browser layer. No retries, timeout increases, or relaxed assertions were introduced.
Causal evidence:
The new touch case took 1.5s/1.7s. The slowest affected cases were the existing shared-thread journey at 19.9s in WebKit and 17.9s in Chromium. These are local measurements, not hosted CI timings.
Remaining validation limits
Native titlebar dragging and packaged desktop behavior were not exercised; terminal browser coverage uses the fixture native bridge. No full
just scanwas run. Existing screenshots are in the PR discussion and local capture set; no screenshots or scratch artifacts are included in the source diff.buzz-review-completed