fix prop bindings on Vue nodes' gallery widgets (#5542)

* fix gallery widget navigators binding

* [refactor] improve test structure and type organization - addresses @DrJKL's review feedback

- Move types comment to reference component file (types already exist there)
- Extract helper functions outside describe blocks as pure functions
- Convert to function declarations for better clarity
- Fix 0-indexed vs 1-indexed consistency in test data generation
- Add placeholder for future test constants organization

* [test] Add WidgetTextarea component test with improved structure - addresses @DrJKL's review feedback

- Move helper functions outside describe blocks as pure function declarations
- Fix options type in createMockWidget to use SimplifiedWidget['options'] instead of Partial<TextareaProps>
- Replace emitted\![0] with safer emitted?.[0] optional chaining pattern
- Add comprehensive test coverage for textarea value binding, events, readonly mode, and edge cases

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Update src/renderer/extensions/vueNodes/widgets/components/WidgetGalleria.vue

Co-authored-by: Alexander Brown <drjkl@comfy.org>

* [refactor] export types from component and use 0-indexed numbering - addresses @DrJKL's review feedback

- Export GalleryImage and GalleryValue types from WidgetGalleria.vue component
- Import types in test file instead of redefining them locally
- Change image alt text from 1-indexed to 0-indexed (Image 0, Image 1, etc.)
- Move helper functions outside describe blocks using function declarations
- Add readonly test data constants for better test isolation
- Fix type compatibility issues with readonly arrays

This addresses DrJKL's review comments about:
1. Types being defined in test suite instead of component
2. Helper functions placement and clarity
3. 1-indexed numbering preference
4. Better test organization with readonly constants

* [refactor] implement remaining review feedback - addresses accessibility and factory pattern suggestions

- Fix accessibility: Add descriptive alt text with position context for screen readers
  instead of repetitive "Gallery image" (e.g., "Gallery image 2 of 5")
- Implement factory pattern: Add createGalleriaWrapper() function that takes images,
  creates widget internally, and returns wrapper for cleaner test code
- Update tests to use factory pattern for readonly constant test cases
- Add proper i18n translations for galleryImage and galleryThumbnail
- Use more descriptive alt text following WebAIM accessibility guidelines

Addresses review comments about screen reader accessibility and test factory pattern

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
This commit is contained in:
Christian Byrne
2025-09-14 16:31:56 -07:00
committed by GitHub
parent 11cb525545
commit 96a663704f
4 changed files with 488 additions and 31 deletions

View File

@@ -11,6 +11,8 @@
"removeImage": "Remove image",
"viewImageOfTotal": "View image {index} of {total}",
"imagePreview": "Image preview - Use arrow keys to navigate between images",
"galleryImage": "Gallery image",
"galleryThumbnail": "Gallery thumbnail",
"errorLoadingImage": "Error loading image",
"failedToDownloadImage": "Failed to download image",
"calculatingDimensions": "Calculating dimensions",

View File

@@ -0,0 +1,443 @@
import { mount } from '@vue/test-utils'
import PrimeVue from 'primevue/config'
import Galleria from 'primevue/galleria'
import type { GalleriaProps } from 'primevue/galleria'
import { describe, expect, it } from 'vitest'
import { createI18n } from 'vue-i18n'
import type { SimplifiedWidget } from '@/types/simplifiedWidget'
import WidgetGalleria, {
type GalleryImage,
type GalleryValue
} from './WidgetGalleria.vue'
const i18n = createI18n({
legacy: false,
locale: 'en',
messages: {
en: {
'Gallery image': 'Gallery image'
}
}
})
// Test data constants for better test isolation
const TEST_IMAGES_SMALL: readonly string[] = Object.freeze([
'https://example.com/image0.jpg',
'https://example.com/image1.jpg',
'https://example.com/image2.jpg'
])
const TEST_IMAGES_SINGLE: readonly string[] = Object.freeze([
'https://example.com/single.jpg'
])
const TEST_IMAGE_OBJECTS: readonly GalleryImage[] = Object.freeze([
{
itemImageSrc: 'https://example.com/image0.jpg',
thumbnailImageSrc: 'https://example.com/thumb0.jpg',
alt: 'Test image 0'
},
{
itemImageSrc: 'https://example.com/image1.jpg',
thumbnailImageSrc: 'https://example.com/thumb1.jpg',
alt: 'Test image 1'
}
])
// Helper functions outside describe blocks for better clarity
function createMockWidget(
value: GalleryValue = [],
options: Partial<GalleriaProps> = {}
): SimplifiedWidget<GalleryValue> {
return {
name: 'test_galleria',
type: 'array',
value,
options
}
}
function mountComponent(
widget: SimplifiedWidget<GalleryValue>,
modelValue: GalleryValue,
readonly = false
) {
return mount(WidgetGalleria, {
global: {
plugins: [PrimeVue, i18n],
components: { Galleria }
},
props: {
widget,
readonly,
modelValue
}
})
}
function createImageStrings(count: number): string[] {
return Array.from(
{ length: count },
(_, i) => `https://example.com/image${i}.jpg`
)
}
// Factory function that takes images, creates widget internally, returns wrapper
function createGalleriaWrapper(
images: GalleryValue,
options: Partial<GalleriaProps> = {},
readonly = false
) {
const widget = createMockWidget(images, options)
return mountComponent(widget, images, readonly)
}
describe('WidgetGalleria Image Display', () => {
// Group tests using the readonly constants where appropriate
describe('Component Rendering', () => {
it('renders galleria component', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SMALL])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.exists()).toBe(true)
})
it('displays empty gallery when no images provided', () => {
const widget = createMockWidget([])
const wrapper = mountComponent(widget, [])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('value')).toEqual([])
})
it('handles null or undefined value gracefully', () => {
const widget = createMockWidget([])
const wrapper = mountComponent(widget, [])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('value')).toEqual([])
})
})
describe('String Array Input', () => {
it('converts string array to image objects', () => {
const widget = createMockWidget([...TEST_IMAGES_SMALL])
const wrapper = mountComponent(widget, [...TEST_IMAGES_SMALL])
const galleria = wrapper.findComponent({ name: 'Galleria' })
const value = galleria.props('value')
expect(value).toHaveLength(3)
expect(value[0]).toEqual({
itemImageSrc: 'https://example.com/image0.jpg',
thumbnailImageSrc: 'https://example.com/image0.jpg',
alt: 'Image 0'
})
})
it('handles single string image', () => {
const widget = createMockWidget([...TEST_IMAGES_SINGLE])
const wrapper = mountComponent(widget, [...TEST_IMAGES_SINGLE])
const galleria = wrapper.findComponent({ name: 'Galleria' })
const value = galleria.props('value')
expect(value).toHaveLength(1)
expect(value[0]).toEqual({
itemImageSrc: 'https://example.com/single.jpg',
thumbnailImageSrc: 'https://example.com/single.jpg',
alt: 'Image 0'
})
})
})
describe('Object Array Input', () => {
it('preserves image objects as-is', () => {
const widget = createMockWidget([...TEST_IMAGE_OBJECTS])
const wrapper = mountComponent(widget, [...TEST_IMAGE_OBJECTS])
const galleria = wrapper.findComponent({ name: 'Galleria' })
const value = galleria.props('value')
expect(value).toEqual([...TEST_IMAGE_OBJECTS])
})
it('handles mixed object properties', () => {
const images: GalleryImage[] = [
{ src: 'https://example.com/image1.jpg', alt: 'First' },
{ itemImageSrc: 'https://example.com/image2.jpg' },
{ thumbnailImageSrc: 'https://example.com/thumb3.jpg' }
]
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
const value = galleria.props('value')
expect(value).toEqual(images)
})
})
describe('Thumbnail Display', () => {
it('shows thumbnails when multiple images present', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SMALL])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showThumbnails')).toBe(true)
})
it('hides thumbnails for single image', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SINGLE])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showThumbnails')).toBe(false)
})
it('respects widget option to hide thumbnails', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SMALL], {
showThumbnails: false
})
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showThumbnails')).toBe(false)
})
it('shows thumbnails when explicitly enabled for multiple images', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SMALL], {
showThumbnails: true
})
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showThumbnails')).toBe(true)
})
})
describe('Navigation Buttons', () => {
it('shows navigation buttons when multiple images present', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SMALL])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showItemNavigators')).toBe(true)
})
it('hides navigation buttons for single image', () => {
const wrapper = createGalleriaWrapper([...TEST_IMAGES_SINGLE])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showItemNavigators')).toBe(false)
})
it('respects widget option to hide navigation buttons', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images, { showItemNavigators: false })
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showItemNavigators')).toBe(false)
})
it('shows navigation buttons when explicitly enabled for multiple images', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images, { showItemNavigators: true })
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('showItemNavigators')).toBe(true)
})
})
describe('Readonly Mode', () => {
it('passes readonly state to galleria when readonly', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images, true)
// Galleria component should receive readonly state (though it may not support disabled)
expect(wrapper.props('readonly')).toBe(true)
})
it('passes readonly state to galleria when not readonly', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images, false)
expect(wrapper.props('readonly')).toBe(false)
})
})
describe('Widget Options Handling', () => {
it('passes through valid widget options', () => {
const images = createImageStrings(2)
const widget = createMockWidget(images, {
circular: true,
autoPlay: true,
transitionInterval: 3000
})
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('circular')).toBe(true)
expect(galleria.props('autoPlay')).toBe(true)
expect(galleria.props('transitionInterval')).toBe(3000)
})
it('applies custom styling props', () => {
const images = createImageStrings(2)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
// Check that galleria has styling attributes rather than specific classes
expect(galleria.attributes('class')).toBeDefined()
})
})
describe('Active Index Management', () => {
it('initializes with zero active index', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('activeIndex')).toBe(0)
})
it('can update active index', async () => {
const images = createImageStrings(3)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
await galleria.vm.$emit('update:activeIndex', 2)
// Check that the internal activeIndex ref was updated
const vm = wrapper.vm as any
expect(vm.activeIndex).toBe(2)
})
})
describe('Image Template Rendering', () => {
it('renders item template with correct image source priorities', () => {
const images: GalleryImage[] = [
{
itemImageSrc: 'https://example.com/item.jpg',
src: 'https://example.com/fallback.jpg'
},
{ src: 'https://example.com/only-src.jpg' }
]
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
// The template logic should prioritize itemImageSrc > src > fallback to the item itself
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.exists()).toBe(true)
})
it('renders thumbnail template with correct image source priorities', () => {
const images: GalleryImage[] = [
{
thumbnailImageSrc: 'https://example.com/thumb.jpg',
src: 'https://example.com/fallback.jpg'
},
{ src: 'https://example.com/only-src.jpg' }
]
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
// The template logic should prioritize thumbnailImageSrc > src > fallback to the item itself
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.exists()).toBe(true)
})
})
describe('Edge Cases', () => {
it('handles empty array gracefully', () => {
const widget = createMockWidget([])
const wrapper = mountComponent(widget, [])
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('value')).toEqual([])
expect(galleria.props('showThumbnails')).toBe(false)
expect(galleria.props('showItemNavigators')).toBe(false)
})
it('handles malformed image objects', () => {
const malformedImages = [
{}, // Empty object
{ randomProp: 'value' }, // Object without expected image properties
null, // Null value
undefined // Undefined value
]
const widget = createMockWidget(malformedImages as string[])
const wrapper = mountComponent(widget, malformedImages as string[])
const galleria = wrapper.findComponent({ name: 'Galleria' })
// Null/undefined should be filtered out, leaving only the objects
const expectedValue = [{}, { randomProp: 'value' }]
expect(galleria.props('value')).toEqual(expectedValue)
})
it('handles very large image arrays', () => {
const largeImageArray = createImageStrings(100)
const widget = createMockWidget(largeImageArray)
const wrapper = mountComponent(widget, largeImageArray)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('value')).toHaveLength(100)
expect(galleria.props('showThumbnails')).toBe(true)
expect(galleria.props('showItemNavigators')).toBe(true)
})
it('handles mixed string and object arrays gracefully', () => {
// This is technically invalid input, but the component should handle it
const mixedArray = [
'https://example.com/string.jpg',
{ itemImageSrc: 'https://example.com/object.jpg' },
'https://example.com/another-string.jpg'
]
const widget = createMockWidget(mixedArray as string[])
// The component expects consistent typing, but let's test it handles mixed input
expect(() => mountComponent(widget, mixedArray as string[])).not.toThrow()
})
it('handles invalid URL strings', () => {
const invalidUrls = ['not-a-url', '', ' ', 'http://', 'ftp://invalid']
const widget = createMockWidget(invalidUrls)
const wrapper = mountComponent(widget, invalidUrls)
const galleria = wrapper.findComponent({ name: 'Galleria' })
expect(galleria.props('value')).toHaveLength(5)
})
})
describe('Styling and Layout', () => {
it('applies max-width constraint', () => {
const images = createImageStrings(2)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
// Check that component has styling applied rather than specific classes
expect(galleria.attributes('class')).toBeDefined()
})
it('applies passthrough props for thumbnails', () => {
const images = createImageStrings(3)
const widget = createMockWidget(images)
const wrapper = mountComponent(widget, images)
const galleria = wrapper.findComponent({ name: 'Galleria' })
const pt = galleria.props('pt')
expect(pt).toBeDefined()
expect(pt.thumbnails).toBeDefined()
expect(pt.thumbnailContent).toBeDefined()
expect(pt.thumbnailPrevButton).toBeDefined()
expect(pt.thumbnailNextButton).toBeDefined()
})
})
})

View File

@@ -4,9 +4,8 @@
v-model:activeIndex="activeIndex"
:value="galleryImages"
v-bind="filteredProps"
:disabled="readonly"
:show-thumbnails="showThumbnails"
:show-nav-buttons="showNavButtons"
:show-item-navigators="showNavButtons"
class="max-w-full"
:pt="{
thumbnails: {
@@ -25,16 +24,22 @@
>
<template #item="{ item }">
<img
:src="item.itemImageSrc || item.src || item"
:alt="item.alt || 'Gallery image'"
:src="item?.itemImageSrc || item?.src || ''"
:alt="
item?.alt ||
`${t('g.galleryImage')} ${activeIndex + 1} of ${galleryImages.length}`
"
class="w-full h-auto max-h-64 object-contain"
/>
</template>
<template #thumbnail="{ item }">
<div class="p-1 w-full h-full">
<img
:src="item.thumbnailImageSrc || item.src || item"
:alt="item.alt || 'Gallery thumbnail'"
:src="item?.thumbnailImageSrc || item?.src || ''"
:alt="
item?.alt ||
`${t('g.galleryThumbnail')} ${galleryImages.findIndex((img) => img === item) + 1} of ${galleryImages.length}`
"
class="w-full h-full object-cover rounded-lg"
/>
</div>
@@ -46,6 +51,7 @@
<script setup lang="ts">
import Galleria from 'primevue/galleria'
import { computed, ref } from 'vue'
import { useI18n } from 'vue-i18n'
import type { SimplifiedWidget } from '@/types/simplifiedWidget'
import {
@@ -53,14 +59,14 @@ import {
filterWidgetProps
} from '@/utils/widgetPropFilter'
interface GalleryImage {
export interface GalleryImage {
itemImageSrc?: string
thumbnailImageSrc?: string
src?: string
alt?: string
}
type GalleryValue = string[] | GalleryImage[]
export type GalleryValue = string[] | GalleryImage[]
const value = defineModel<GalleryValue>({ required: true })
@@ -71,6 +77,8 @@ const props = defineProps<{
const activeIndex = ref(0)
const { t } = useI18n()
const filteredProps = computed(() =>
filterWidgetProps(props.widget.options, GALLERIA_EXCLUDED_PROPS)
)
@@ -78,16 +86,18 @@ const filteredProps = computed(() =>
const galleryImages = computed(() => {
if (!value.value || !Array.isArray(value.value)) return []
return value.value.map((item, index) => {
if (typeof item === 'string') {
return {
itemImageSrc: item,
thumbnailImageSrc: item,
alt: `Image ${index + 1}`
return value.value
.filter((item) => item !== null && item !== undefined) // Filter out null/undefined
.map((item, index) => {
if (typeof item === 'string') {
return {
itemImageSrc: item,
thumbnailImageSrc: item,
alt: `Image ${index}`
}
}
}
return item
})
return item ?? {} // Ensure we have at least an empty object
})
})
const showThumbnails = computed(() => {
@@ -99,7 +109,7 @@ const showThumbnails = computed(() => {
const showNavButtons = computed(() => {
return (
props.widget.options?.showNavButtons !== false &&
props.widget.options?.showItemNavigators !== false &&
galleryImages.value.length > 1
)
})

View File

@@ -7,24 +7,26 @@ import type { SimplifiedWidget } from '@/types/simplifiedWidget'
import WidgetTextarea from './WidgetTextarea.vue'
const createMockWidget = (
function createMockWidget(
value: string = 'default text',
options: Record<string, any> = {},
options: SimplifiedWidget['options'] = {},
callback?: (value: string) => void
): SimplifiedWidget<string> => ({
name: 'test_textarea',
type: 'string',
value,
options,
callback
})
): SimplifiedWidget<string> {
return {
name: 'test_textarea',
type: 'string',
value,
options,
callback
}
}
const mountComponent = (
function mountComponent(
widget: SimplifiedWidget<string>,
modelValue: string,
readonly = false,
placeholder?: string
) => {
) {
return mount(WidgetTextarea, {
global: {
plugins: [PrimeVue],
@@ -39,11 +41,11 @@ const mountComponent = (
})
}
const setTextareaValueAndTrigger = async (
async function setTextareaValueAndTrigger(
wrapper: ReturnType<typeof mount>,
value: string,
trigger: 'blur' | 'input' = 'blur'
) => {
) {
const textarea = wrapper.find('textarea')
if (!(textarea.element instanceof HTMLTextAreaElement)) {
throw new Error(