Comfy-Org / Comfy-Org/ComfyUI_frontend

Media asset card interaction follow-ups from #14765 review

Open
#15,220 2 comments 1 reaction 0 assignees View on GitHub
area:ui Potential Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.