Comfy-Org / Comfy-Org/ComfyUI_frontend
Media asset card interaction follow-ups from #14765 review
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 704
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 512
Description
Collected from the review of https://github.com/Comfy-Org/ComfyUI_frontend/pull/14765. None of these blocked that PR; they are grouped here so the bugfix could land clean. Roughly ordered by value. Separate PRs are fine — they are independent.
Related, already filed: https://github.com/Comfy-Org/ComfyUI_frontend/issues/15028 (consolidate multi-select modifier detection).
---
### 1. Modifier-clicks on the native video control bar actuate the control _and_ mutate selection
Introduced by https://github.com/Comfy-Org/ComfyUI_frontend/pull/14765, which removed `@click.stop` from the `` element.
`onVideoClick` returns early when the native controls are showing, but an early `return` does not stop propagation, so the click still reaches the card's `handlePreviewClick`:
```ts
// src/platform/assets/components/MediaVideoTop.vue:73-81
async function onVideoClick(event: MouseEvent) {
if (event.shiftKey || event.metaKey || event.ctrlKey || shouldShowControls.value) {
return // <- bubbles on to MediaAssetCard.vue:26 @click.stop="handlePreviewClick"
}
...
```
Repro (`grid` mode, video playing and hovered so native controls are visible):
- Cmd-click the native play button → toggles playback **and** toggles selection
- Shift-click the scrubber → seeks **and** range-selects up to this card
Narrow: needs playing + hovered + the ~30px control strip, and only in `grid` (in `grid-small` the controls never render). Likely fix is a one-liner:
```ts
if (shouldShowControls.value) return event.stopPropagation()
```
No unit test can reach this — happy-dom has no native-controls shadow DOM — so it needs a manual pass.
---
### 2. Audio cards cannot be modifier-selected
Pre-existing, and the same bug class as FE-1512 but for a different media type. The compact `WaveAudioPlayer` root swallows both events before the card sees them:
```vue
@pointerdown.stop @click.stop
```
`variant` defaults to `'compact'` and `MediaAudioTop.vue` renders `` with no `variant`, so this is the path every audio asset card takes. Shift/Cmd/Ctrl-click on the player strip does nothing to the selection. The rest of the card's preview area still selects, which makes it read as flaky rather than unsupported.
---
### 3. Compact-grid video preview has no keyboard-operable play/pause
Pre-existing gap, newly load-bearing after https://github.com/Comfy-Org/ComfyUI_frontend/pull/14765. A `` without `controls` is not focusable, and the card root has no `tabindex`/`role`/key handler:
```ts
// src/platform/assets/components/MediaVideoTop.vue:51-53
const shouldShowControls = computed(
() => showNativeControls && isPlaying.value && isHovered.value
)
```
Before, the element could at least become focusable once playing. Now `grid-small` passes `showNativeControls: false`, so in compact grid it never becomes focusable at any point in its lifecycle — play/pause there is mouse-only.
---
### 4. `isVideoPlaying` and `showVideoControls` are orphaned
https://github.com/Comfy-Org/ComfyUI_frontend/pull/14765 moved the actions overlay to pure CSS, which deleted `showActionsOverlay` — the last reader of both. What remains is a write-only chain with zero consumers:
```
MediaVideoTop.vue:41-44 emits videoPlayingStateChanged + videoControlsChanged
MediaAssetCard.vue:46-47 @video-playing-state-changed / @video-controls-changed
MediaAssetCard.vue:246-247 const isVideoPlaying / showVideoControls = ref(false)
MediaAssetCard.vue:302-303 provided on the MediaAsset context
mediaAssetSchema.ts:48-49 still on the MediaAssetProviderValue contract
```
`grep -rn "isVideoPlaying\|showVideoControls" src/` returns only writes, the schema declaration, and one test provider. Worth deciding deliberately: either drop the refs, the emits, the watch in `MediaVideoTop`, and the two contract fields, or keep them with a comment saying why. Note `MediaVideoTop.test.ts:68,79` asserts the emits, so those go too if the emits go.
---
### 5. List view: a modifier-click opens the lightbox on top of the selection it just made
Pre-existing, and on the list path rather than the card path, so it was out of scope for https://github.com/Comfy-Org/ComfyUI_frontend/pull/14765.
```vue
```
The emit is unconditional. Chain: `AssetsListItem.vue:41` → `AssetsSidebarListView.vue:48` `preview-asset` → `AssetsSidebarTab.vue:117` `handleZoomClick` → `MediaLightbox`. So Shift-click in list view range-selects and then immediately covers the result with the lightbox.
Contributor guide
Research direction
Start with the referenced paths in MediaVideoTop.vue, MediaAssetCard.vue, WaveAudioPlayer.vue, AssetsListItem.vue, and the listed sidebar components; read the related tests, including MediaVideoTop.test.ts, and trace each event path. Treat the five follow-ups as separate investigations. Done means modifier selection, keyboard playback, lightbox behavior, and video state handling match the stated expected behavior, with manual verification for native controls.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- accessibility, frontend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100