Skip to content

feat: improve vue node video preview loading and a11y #7558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jtydhr88 merged 1 commit into from
Dec 16, 2025
Merged

feat: improve vue node video preview loading and a11y #7558

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 42 additions & 19 deletions src/renderer/extensions/vueNodes/VideoPreview.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@
<div
v-if="imageUrls.length > 0"
class="video-preview group relative flex size-full min-h-16 min-w-16 flex-col px-2"
tabindex="0"
role="region"
:aria-label="$t('g.videoPreview')"
@mouseenter="handleMouseEnter"
@mouseleave="handleMouseLeave"
@keydown="handleKeyDown"
>
<!-- Video Wrapper -->
<div
ref="videoWrapperEl"
class="relative h-full w-full grow overflow-hidden rounded-[5px] bg-node-component-surface"
tabindex="0"
role="region"
:aria-label="$t('g.videoPreview')"
:aria-busy="showLoader"
@mouseenter="handleMouseEnter"
@mouseleave="handleMouseLeave"
@focusin="handleFocusIn"
@focusout="handleFocusOut"
>
<!-- Error State -->
<div
Expand All @@ -27,18 +31,18 @@

<!-- Loading State -->
<Skeleton
v-if="isLoading && !videoError"
v-if="showLoader && !videoError"
class="absolute inset-0 size-full"
border-radius="5px"
width="16rem"
height="16rem"
width="100%"
height="100%"
Comment on lines +37 to +38
Copy link
Contributor

@DrJKL DrJKL Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR
I still find it weird that we define the sizing in 3 ways

  • width/height
  • absolute + inset-0
  • size-full

/>

<!-- Main Video -->
<video
v-if="!videoError"
:src="currentVideoUrl"
:class="cn('block size-full object-contain', isLoading && 'invisible')"
:class="cn('block size-full object-contain', showLoader && 'invisible')"
controls
loop
playsinline
Expand All @@ -47,10 +51,13 @@
/>

<!-- Floating Action Buttons (appear on hover) -->
<div v-if="isHovered" class="actions absolute top-2 right-2 flex gap-1">
<div
v-if="isHovered || isFocused"
class="actions absolute top-2 right-2 flex gap-2.5"
>
<!-- Download Button -->
<button
class="action-btn cursor-pointer rounded-lg border-0 bg-white p-2 text-black shadow-sm transition-all duration-200 hover:bg-smoke-100"
:class="actionButtonClass"
:title="$t('g.downloadVideo')"
:aria-label="$t('g.downloadVideo')"
@click="handleDownload"
Expand All @@ -60,7 +67,7 @@

<!-- Close Button -->
<button
class="action-btn cursor-pointer rounded-lg border-0 bg-white p-2 text-black shadow-sm transition-all duration-200 hover:bg-smoke-100"
:class="actionButtonClass"
:title="$t('g.removeVideo')"
:aria-label="$t('g.removeVideo')"
@click="handleRemove"
Expand Down Expand Up @@ -94,7 +101,7 @@
<span v-if="videoError" class="text-red-400">
{{ $t('g.errorLoadingVideo') }}
</span>
<span v-else-if="isLoading" class="text-smoke-400">
<span v-else-if="showLoader" class="text-smoke-400">
{{ $t('g.loading') }}...
</span>
<span v-else>
Expand Down Expand Up @@ -126,12 +133,18 @@ const props = defineProps<VideoPreviewProps>()
const { t } = useI18n()
const nodeOutputStore = useNodeOutputStore()

const actionButtonClass =
Copy link
Contributor

@DrJKL DrJKL Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep this in mind for the button variants.

'flex h-8 min-h-8 items-center justify-center gap-2.5 rounded-lg border-0 bg-button-surface px-2 py-2 text-button-surface-contrast shadow-sm transition-colors duration-200 hover:bg-button-hover-surface focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-button-surface-contrast focus-visible:ring-offset-2 focus-visible:ring-offset-transparent cursor-pointer'

// Component state
const currentIndex = ref(0)
const isHovered = ref(false)
const isFocused = ref(false)
const actualDimensions = ref<string | null>(null)
const videoError = ref(false)
const isLoading = ref(false)
const showLoader = ref(false)

const videoWrapperEl = ref<HTMLDivElement>()

// Computed values
const currentVideoUrl = computed(() => props.imageUrls[currentIndex.value])
Expand All @@ -149,24 +162,24 @@ watch(
// Reset loading and error states when URLs change
actualDimensions.value = null
videoError.value = false
isLoading.value = newUrls.length > 0
showLoader.value = newUrls.length > 0
},
{ deep: true }
{ deep: true, immediate: true }
)

// Event handlers
const handleVideoLoad = (event: Event) => {
if (!event.target || !(event.target instanceof HTMLVideoElement)) return
const video = event.target
isLoading.value = false
showLoader.value = false
videoError.value = false
if (video.videoWidth && video.videoHeight) {
actualDimensions.value = `${video.videoWidth} x ${video.videoHeight}`
}
}

const handleVideoError = () => {
isLoading.value = false
showLoader.value = false
videoError.value = true
actualDimensions.value = null
}
Expand Down Expand Up @@ -194,7 +207,7 @@ const setCurrentIndex = (index: number) => {
if (index >= 0 && index < props.imageUrls.length) {
currentIndex.value = index
actualDimensions.value = null
isLoading.value = true
showLoader.value = true
videoError.value = false
}
}
Expand All @@ -207,6 +220,16 @@ const handleMouseLeave = () => {
isHovered.value = false
}

const handleFocusIn = () => {
isFocused.value = true
Copy link
Contributor

@DrJKL DrJKL Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It really feels like we're using JavaScript to do what the DOM does well otherwise (hovered/focused state management)

Copy link
Contributor

@DrJKL DrJKL Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do a group/focus-within to achieve the same goals?

}

const handleFocusOut = (event: FocusEvent) => {
if (!videoWrapperEl.value?.contains(event.relatedTarget as Node)) {
isFocused.value = false
}
}

const getNavigationDotClass = (index: number) => {
return [
'w-2 h-2 rounded-full transition-all duration-200 border-0 cursor-pointer',
Expand Down