fix(modal): [modal] add aria-* attribute #3941
Conversation
WalkthroughRefined end-to-end test selectors and UI labels across modal/dialog demos; added ARIA attributes and unique IDs to modal and dialog components for accessibility; test files updated to use demo-scoped locators and visible/state-specific selectors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
examples/sites/demos/pc/app/modal/basic-usage.spec.ts (1)
8-8: Suggest: Rename variable for clarity.The
contentvariable now targets the entire modal container (.tiny-modal.type__alert.is__visible) rather than the content element. Consider renaming it tomodaloralertModalfor better code readability.🔎 Proposed variable rename
- const content = page.locator('.tiny-modal.type__alert.is__visible') + const alertModal = page.locator('.tiny-modal.type__alert.is__visible')Then update references on lines 27 and 32:
- await expect(content).toHaveText(/窗口内容1/) + await expect(alertModal).toHaveText(/窗口内容1/) - await expect(content).toHaveText(/窗口内容2/) + await expect(alertModal).toHaveText(/窗口内容2/)examples/sites/demos/pc/app/modal/modal-header.spec.ts (1)
7-7: Suggest: Rename variable for clarity.The
headervariable now targets the entire modal container (.tiny-modal.is__visible) rather than the header/title element. Consider renaming tomodalfor better code readability, or use.tiny-modal.is__visible .tiny-modal__titleif you want to specifically target the title.🔎 Proposed variable rename
- const header = page.locator('.tiny-modal.is__visible') + const modal = page.locator('.tiny-modal.is__visible')Then update line 11:
- await expect(header).toHaveText(/自定义弹窗标题/) + await expect(modal).toHaveText(/自定义弹窗标题/)examples/sites/demos/pc/app/modal/status.spec.ts (1)
6-8: Suggest: Simplify fragile nested selector.The deeply nested selector is brittle and will break if any intermediate DOM structure changes. Consider a more resilient approach using attribute selectors or less specific nesting.
🔎 Proposed more resilient selector
- const modal = page.locator( - '.tiny-modal__wrapper.is__visible > .tiny-modal__box > .tiny-modal__header > .tiny-modal__status-wrapper > svg' - ) + const modal = page.locator('.tiny-modal.is__visible .tiny-modal__status-wrapper svg')This achieves the same targeting with less fragility by using descendant selectors instead of direct child selectors.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/sites/demos/pc/app/modal/basic-usage.spec.tsexamples/sites/demos/pc/app/modal/modal-footer-composition-api.vueexamples/sites/demos/pc/app/modal/modal-footer.spec.tsexamples/sites/demos/pc/app/modal/modal-footer.vueexamples/sites/demos/pc/app/modal/modal-header.spec.tsexamples/sites/demos/pc/app/modal/modal-size.spec.tsexamples/sites/demos/pc/app/modal/status.spec.tspackages/vue/src/modal/src/pc.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (8)
examples/sites/demos/pc/app/modal/modal-footer.vue (1)
5-5: LGTM! Improved demo button labels.The updated button labels clearly distinguish between functional, template-based, and slot-based modal invocation patterns, making the demo more intuitive.
Also applies to: 9-9, 23-23
examples/sites/demos/pc/app/modal/modal-footer-composition-api.vue (1)
5-5: LGTM! Consistent label improvements.The button labels match the updates in the Options API version, maintaining consistency across demo variants.
Also applies to: 9-9, 23-23
packages/vue/src/modal/src/pc.vue (2)
215-215: LGTM! Appropriate aria-labels for interactive controls.The
aria-labelattributes on the Zoom and Close buttons provide clear accessibility labels for assistive technologies.Also applies to: 224-224, 282-282
194-194: LGTM! Dynamic status icon labels.The dynamic generation of
aria-labelfor status icons using the constants mapping is a good approach for maintaining meaningful, context-aware accessibility labels.Also applies to: 249-249
examples/sites/demos/pc/app/modal/basic-usage.spec.ts (1)
8-8: LGTM! Improved selector targeting visible modals.The use of
.is__visiblestate selector ensures the test targets only the active modal, reducing potential flakiness from multiple modal instances. The simplified assertions (removing.nth()) indicate more precise targeting.Also applies to: 27-27, 32-32
examples/sites/demos/pc/app/modal/modal-header.spec.ts (1)
7-7: LGTM! Consistent use of visible-state selector.Using
.is__visiblealigns with the test suite pattern of targeting only active modals, improving test reliability.Also applies to: 11-11
examples/sites/demos/pc/app/modal/modal-footer.spec.ts (1)
6-7: LGTM! Test updates align with demo changes.The updated test properly covers all three modal footer customization approaches:
- Functional invocation (函数式)
- Template-based (标签式)
- Slot-based (插槽)
The new
footerLinklocator and assertions correctly verify the slot-based footer content. The more specific footer button selector (.tiny-modal__footer > .tiny-button--info) improves targeting precision.Also applies to: 10-20
examples/sites/demos/pc/app/modal/status.spec.ts (1)
36-54: LGTM! Expanded message status coverage.The additional assertions comprehensively verify all message status types (info, success, warning, error, loading), improving test coverage for the modal component's various states.
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('modal#modal-size') | ||
| const modal = page.locator('.tiny-modal') | ||
| const modal = page.locator('.status__question') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the test file to see the actual changes
head -20 examples/sites/demos/pc/app/modal/modal-size.spec.tsRepository: opentiny/tiny-vue
Length of output: 178
🏁 Script executed:
# Search for the modal demo HTML/template files
fd -e html -e vue examples/sites/demos/pc/app/modal/ | head -20Repository: opentiny/tiny-vue
Length of output: 502
🏁 Script executed:
# Search for .status__question selector in the modal demo directory
rg "status.*question|status__question" examples/sites/demos/pc/app/modal/ -B 3 -A 3Repository: opentiny/tiny-vue
Length of output: 814
🏁 Script executed:
# Corrected fd command to search in modal directory
fd . --full-path 'examples/sites/demos/pc/app/modal/' --type f | grep -E '\.(vue|html|jsx|tsx)$'Repository: opentiny/tiny-vue
Length of output: 1802
🏁 Script executed:
# Read the entire modal-size.spec.ts file to see context
cat examples/sites/demos/pc/app/modal/modal-size.spec.tsRepository: opentiny/tiny-vue
Length of output: 727
🏁 Script executed:
# Search for where .status__question class is defined or applied in modal-related files
rg "status__question" examples/sites/demos/pc/app/modal/Repository: opentiny/tiny-vue
Length of output: 163
🏁 Script executed:
# Check the modal-size.vue demo to see if .status__question class is present
cat examples/sites/demos/pc/app/modal/modal-size.vueRepository: opentiny/tiny-vue
Length of output: 1490
🏁 Script executed:
# Also check the composition-api version
cat examples/sites/demos/pc/app/modal/modal-size-composition-api.vueRepository: opentiny/tiny-vue
Length of output: 1375
🏁 Script executed:
# Search for status__question in all modal demo files to understand where it comes from
rg "status__question" examples/sites/demos/pc/app/modal/ -B 5 -A 5Repository: opentiny/tiny-vue
Length of output: 1133
🏁 Script executed:
# Check the status.vue file since it's likely related to status classes
cat examples/sites/demos/pc/app/modal/status.vueRepository: opentiny/tiny-vue
Length of output: 3034
🏁 Script executed:
# Also check the status-composition-api version
cat examples/sites/demos/pc/app/modal/status-composition-api.vueRepository: opentiny/tiny-vue
Length of output: 2874
🏁 Script executed:
# Search for where .status__question class is defined in the component library
rg "status__question" --type vue --type ts --type js -B 3 -A 3 | head -50Repository: opentiny/tiny-vue
Length of output: 88
🏁 Script executed:
# Check if modal-size demo uses any status class at all
rg "status" examples/sites/demos/pc/app/modal/modal-size.vue examples/sites/demos/pc/app/modal/modal-size-composition-api.vueRepository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Look at other modal test files to see what selectors they use
fd -e spec.ts --full-path 'examples/sites/demos/pc/app/modal/' | xargs grep -h "locator\|goto" | head -30Repository: opentiny/tiny-vue
Length of output: 1594
🏁 Script executed:
# Search comprehensively for status__question in the entire codebase
rg "status__question" --type-listRepository: opentiny/tiny-vue
Length of output: 450
🏁 Script executed:
# Try without type restrictions
rg "status__question"Repository: opentiny/tiny-vue
Length of output: 396
🏁 Script executed:
# Check if there's a test output or expected markup showing what .status__question should be
rg "question" examples/sites/demos/pc/app/modal/ | grep -v node_modulesRepository: opentiny/tiny-vue
Length of output: 163
🏁 Script executed:
# Let me check what the modal component generates at runtime - look at the Modal component source
fd -e vue -e ts --full-path "packages/*/modal" | head -10Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check the theme file to see what status classes exist
cat packages/theme/src/modal/index.less | grep -A 5 "status__"Repository: opentiny/tiny-vue
Length of output: 784
🏁 Script executed:
# Look at the Modal component to see what status values are available
fd -e ts -e js "modal" packages/vue/src | head -20Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Find the modal component implementation
find packages -name "*modal*" -type f | grep -E '\.(ts|js|vue)$' | grep -E 'component|src' | head -20Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check if modal-size demo ever sets status to "question"
rg "question" examples/sites/demos/pc/app/modal/modal-size.vue examples/sites/demos/pc/app/modal/modal-size-composition-api.vue -iRepository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check what classes the actual modal in modal-size demo would have
# Look for the modal structure that would be rendered
rg "type__confirm\|tiny-modal" examples/sites/demos/pc/app/modal/modal-size.vue examples/sites/demos/pc/app/modal/modal-size-composition-api.vue -B 2 -A 2Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Search across all modal test files to see what other selectors target the modal element
rg "locator\(" examples/sites/demos/pc/app/modal/*.spec.ts | grep -i "modal"Repository: opentiny/tiny-vue
Length of output: 2928
🏁 Script executed:
# Check if there's a way status="question" is set dynamically in modal-size
rg "Modal\.\|status.*=" examples/sites/demos/pc/app/modal/modal-size.vue examples/sites/demos/pc/app/modal/modal-size-composition-api.vue -B 1 -A 1Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check the status.spec.ts test to confirm what selector it uses
cat examples/sites/demos/pc/app/modal/status.spec.ts | head -20Repository: opentiny/tiny-vue
Length of output: 856
🏁 Script executed:
# Verify the actual CSS class that gets applied to modals created by Modal.confirm()
rg "type__confirm|status__" packages/theme/src/modal/index.less -B 2 -A 5 | head -40Repository: opentiny/tiny-vue
Length of output: 1143
🏁 Script executed:
# Confirm what status/type the Modal.confirm() call creates by default
rg "Modal.confirm" packages/vue/src -A 10 | head -30Repository: opentiny/tiny-vue
Length of output: 2339
🏁 Script executed:
# Check the test more carefully - does line 13 make sense with the selector from line 6?
cat examples/sites/demos/pc/app/modal/modal-size.spec.tsRepository: opentiny/tiny-vue
Length of output: 727
Selector .status__question is incorrect—modal-size demo never applies this class.
The test creates a modal via Modal.confirm() without a status parameter, so the modal will never have the .status__question class. Use .type__confirm or .tiny-modal.is__visible instead, consistent with other modal tests (e.g., modal-resize.spec.ts, basic-usage.spec.ts).
Also applies to: 13-13
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/modal/modal-size.spec.ts around lines 6 and 13,
the test is selecting `.status__question` which is never applied in this demo
(Modal.confirm() is called without a status), so replace those selectors with
the correct ones used by other modal tests — e.g. use `.type__confirm` or the
visible modal selector `.tiny-modal.is__visible` to target the created modal;
update both occurrences to one of these valid selectors so the test reliably
finds the modal.
| // ----消息状态示例---- | ||
| await page.getByRole('button', { name: '信息提示图标' }).nth(1).click() | ||
| await expect(message).toHaveClass(/status__info/) | ||
|
|
||
| // ----消息状态示例---- | ||
| await page.getByRole('button', { name: '成功提示图标' }).nth(1).click() | ||
| await expect(message.nth(1)).toHaveClass(/status__success/) | ||
|
|
||
| // ----消息状态示例---- | ||
| await page.getByRole('button', { name: '警告提示图标' }).nth(1).click() | ||
| await expect(message.nth(2)).toHaveClass(/status__warning/) | ||
|
|
||
| // ----消息状态示例---- | ||
| await page.getByRole('button', { name: '错误提示图标' }).nth(1).click() | ||
| await expect(message.nth(3)).toHaveClass(/status__error/) | ||
|
|
||
| // ----消息状态示例---- | ||
| await page.getByRole('button', { name: '加载提示图标' }).nth(1).click() | ||
| await expect(message).toHaveClass(/status__loading/) | ||
| await expect(message.nth(4)).toHaveClass(/status__loading/) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to understand the full context
cat -n examples/sites/demos/pc/app/modal/status.spec.tsRepository: opentiny/tiny-vue
Length of output: 2593
🏁 Script executed:
# Search for message implementation with duration/timeout
fd -e 'ts' -e 'js' -e 'tsx' -e 'jsx' packages/vue/src -type f | xargs grep -l "type.*message" | head -10Repository: opentiny/tiny-vue
Length of output: 231
🏁 Script executed:
# Search for message component or factory that might handle auto-dismiss
find packages/vue/src -name "*message*" -type fRepository: opentiny/tiny-vue
Length of output: 107
🏁 Script executed:
# Look for duration property in message-related code
rg "duration" packages/vue/src --type ts --type js -B 3 -A 3 | grep -i message -A 3 -B 3 | head -50Repository: opentiny/tiny-vue
Length of output: 1558
🏁 Script executed:
# Check the modal implementation
head -200 packages/vue/src/modal/src/index.tsRepository: opentiny/tiny-vue
Length of output: 3270
🏁 Script executed:
# Look for duration-related logic in modal
rg "duration" packages/vue/src/modal/src/index.ts -B 2 -A 5Repository: opentiny/tiny-vue
Length of output: 317
🏁 Script executed:
# Find the setup implementation for modal
find packages/vue/src/modal -name "*.ts" -o -name "*.tsx" -o -name "*.js" | grep -v test | grep -v __tests__Repository: opentiny/tiny-vue
Length of output: 128
🏁 Script executed:
# Search for where duration is used in the modal component
rg "duration" packages/vue/src/modal -B 3 -A 3 --type-listRepository: opentiny/tiny-vue
Length of output: 450
🏁 Script executed:
# Search for where duration is used in the modal component
rg "duration" packages/vue/src/modal -B 3 -A 3Repository: opentiny/tiny-vue
Length of output: 3183
🏁 Script executed:
# Look at the full duration test to understand the auto-dismiss behavior
sed -n '60,80p' packages/vue/src/modal/__tests__/modal.test.tsxRepository: opentiny/tiny-vue
Length of output: 560
🏁 Script executed:
# Search for the actual implementation that handles auto-dismiss (likely onMounted or watch)
rg "onMounted\|onBeforeUnmount\|watch\|setTimeout" packages/vue/src/modal/src/pc.vue -B 2 -A 8 | head -100Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Look for the modal setup logic - check what $setup does
rg "\$setup" packages/vue/src/modal/src/index.ts -B 5 -A 5Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check if there's a setup.ts or similar file
find packages/vue/src/modal -name "*setup*" -type fRepository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Look for JavaScript/TypeScript implementation files in modal
ls -la packages/vue/src/modal/src/Repository: opentiny/tiny-vue
Length of output: 357
🏁 Script executed:
# Read the pc.vue implementation to find duration-based auto-dismiss logic
cat -n packages/vue/src/modal/src/pc.vueRepository: opentiny/tiny-vue
Length of output: 13871
🏁 Script executed:
# Look for the renderless modal implementation
find . -path "*/vue-renderless/modal*" -type f 2>/dev/null | head -20Repository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Search for renderless modal in node_modules or source
rg "vue-renderless/modal" packages/vue/src -lRepository: opentiny/tiny-vue
Length of output: 170
🏁 Script executed:
# Find the renderless implementation in the repo
find . -name "modal" -type d | grep -i renderlessRepository: opentiny/tiny-vue
Length of output: 92
🏁 Script executed:
# Check if renderless is a separate package directory
ls -la packages/ | grep renderlessRepository: opentiny/tiny-vue
Length of output: 122
🏁 Script executed:
# Search for where the modal duration auto-dismiss might be implemented
rg "setTimeout\|setInterval" packages/ -l --max-count 5 | grep -i modalRepository: opentiny/tiny-vue
Length of output: 43
🏁 Script executed:
# Check the renderless modal implementation
ls -la packages/renderless/src/modal/Repository: opentiny/tiny-vue
Length of output: 298
🏁 Script executed:
# Look for the modal setup logic in renderless
find packages/renderless/src/modal -name "*.ts" -o -name "*.js"Repository: opentiny/tiny-vue
Length of output: 136
🏁 Script executed:
# Check the vue.ts file for duration handling
cat -n packages/renderless/src/modal/vue.tsRepository: opentiny/tiny-vue
Length of output: 5317
🏁 Script executed:
# Search for duration in the renderless modal implementation
rg "duration" packages/renderless/src/modal/ -B 3 -A 10Repository: opentiny/tiny-vue
Length of output: 1921
🏁 Script executed:
# Get more context around the duration setTimeout calls
sed -n '1,150p' packages/renderless/src/modal/index.ts | tail -80Repository: opentiny/tiny-vue
Length of output: 2167
🏁 Script executed:
# Get the specific section with the first duration setTimeout
sed -n '200,300p' packages/renderless/src/modal/index.tsRepository: opentiny/tiny-vue
Length of output: 2797
🏁 Script executed:
# Look for the mouseEnterEvent and mouseLeaveEvent handlers
rg "mouseEnterEvent|mouseLeaveEvent" packages/renderless/src/modal/index.ts -B 2 -A 10Repository: opentiny/tiny-vue
Length of output: 444
Add explicit timing controls to avoid race conditions between auto-dismiss and assertions.
Messages auto-dismiss after duration (default 3000ms) and pause on hover. However, the rapid succession of button clicks (lines 36-54) creates a race condition: assertions on accumulated messages may fail if the auto-dismiss timer fires before they complete. Consider either:
- Adding explicit waits (
await page.waitForTimeout()) between test blocks - Using
await expect().toHaveClass()with explicit polling or timeout options - Waiting for messages to dismiss between assertions if testing them individually
🤖 Prompt for AI Agents
In examples/sites/demos/pc/app/modal/status.spec.ts around lines 36-54, the test
clicks several buttons in rapid succession which can race with the messages'
auto-dismiss timers; update the test to add explicit timing controls by
inserting short awaits between actions (e.g., await page.waitForTimeout(200-500)
after each click) or by using expect with explicit timeout/polling (e.g., await
expect(message.nth(i)).toHaveClass(..., { timeout: 2000 })) and/or wait for each
message to be dismissed before proceeding; ensure each assertion waits long
enough for the message to appear and stabilise to avoid flakiness.
| role: 'dialog', | ||
| 'aria-modal': 'true', | ||
| 'aria-labelledby': 'modal_unique_0', | ||
| 'aria-describedby': 'modal_unique_1', |
There was a problem hiding this comment.
Critical: Hardcoded IDs cause duplicate ID violations.
The hardcoded IDs modal_unique_0 and modal_unique_1 will create duplicate IDs if multiple modal instances exist on the page (even if only one is visible at a time), violating HTML specifications and breaking ARIA relationships.
The component receives an id prop (line 43) but doesn't use it. Generate unique IDs per modal instance using either the provided id prop or a unique identifier (e.g., using a counter or UUID).
🔎 Proposed fix using component id and unique suffix
setup(props, context) {
- return setup({ props, context, renderless, api }) as unknown as IModalApi
+ const instance = setup({ props, context, renderless, api }) as unknown as IModalApi
+ // Generate unique IDs for ARIA attributes
+ const uniqueId = props.id || `modal_${Math.random().toString(36).substr(2, 9)}`
+ instance.titleId = `${uniqueId}_title`
+ instance.contentId = `${uniqueId}_content`
+ return instance
},Then update the ARIA attributes to use the generated IDs:
{
role: 'dialog',
'aria-modal': 'true',
- 'aria-labelledby': 'modal_unique_0',
- 'aria-describedby': 'modal_unique_1',
+ 'aria-labelledby': this.titleId,
+ 'aria-describedby': this.contentId, {
- class: 'tiny-modal__title',
- id: 'modal_unique_0'
+ class: 'tiny-modal__title',
+ id: this.titleId
}, {
- class: 'tiny-modal__content',
- id: 'modal_unique_1'
+ class: 'tiny-modal__content',
+ id: this.contentId
},Also applies to: 208-208, 262-262
🤖 Prompt for AI Agents
packages/vue/src/modal/src/pc.vue lines 163-166 (also update 208 and 262): the
component uses hardcoded IDs "modal_unique_0" and "modal_unique_1" causing
duplicate-ID and broken ARIA relationships; generate per-instance unique IDs by
using the incoming id prop if provided or creating a unique suffix (e.g., an
internal counter or UUID) and compose labelId and descId (e.g., `${id}-label` /
`${id}-desc`), replace the hardcoded strings in aria-labelledby and
aria-describedby with those generated IDs, and ensure the corresponding
title/description elements use the same IDs so ARIA attributes reference the
correct elements for each modal instance.
Major: aria-labelledby references potentially non-existent element.
The aria-labelledby attribute always references modal_unique_0 (line 165), but the title element with this ID is only rendered when title !== '' (line 203). When there's no title, the ARIA reference points to a non-existent element, which creates an accessibility issue.
Consider conditionally setting aria-labelledby only when a title exists, or use aria-label as a fallback when no title is provided.
🔎 Proposed fix with conditional aria-labelledby
{
role: 'dialog',
'aria-modal': 'true',
- 'aria-labelledby': 'modal_unique_0',
+ ...(title !== '' ? { 'aria-labelledby': this.titleId } : { 'aria-label': t('ui.alert.title') }),
'aria-describedby': this.contentId,Also applies to: 203-211
🤖 Prompt for AI Agents
In packages/vue/src/modal/src/pc.vue around lines 163-166 (and related title
rendering at 203-211), aria-labelledby is always set to "modal_unique_0" even
when the title element with that ID is not rendered, creating an accessibility
issue; fix by only adding the aria-labelledby attribute when title is non-empty
(i.e., conditionally include aria-labelledby="modal_unique_0" alongside the
title render), and when title is empty provide an accessible fallback such as
aria-label (e.g., use a prop or localized string) or aria-describedby alone so
the dialog never references a non-existent ID.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/sites/demos/pc/app/dialog-box/basic-usage.spec.tsexamples/sites/demos/pc/app/dialog-box/form-in-dialog.spec.tspackages/vue/src/dialog-box/src/pc.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
examples/sites/demos/pc/app/dialog-box/form-in-dialog.spec.ts (1)
7-7: LGTM! Improved test isolation.Scoping the dialog locator to the demo element improves test isolation and reduces the risk of selecting unintended elements if multiple dialogs exist on the page.
examples/sites/demos/pc/app/dialog-box/basic-usage.spec.ts (1)
8-23: LGTM! Consistent test scoping improvements.All button interactions are now scoped to the demo element, which improves test isolation and aligns with the broader test refactoring pattern across dialog-box tests.
| role="dialog" | ||
| :aria-modal="true" | ||
| aria-labelledby="tiny_dialog_unique_0" | ||
| aria-describedby="tiny_dialog_unique_1" | ||
| > | ||
| <div v-if="showHeader" ref="header" class="tiny-dialog-box__header" @mousedown="handleDrag"> | ||
| <slot name="title"> | ||
| <span class="tiny-dialog-box__title">{{ title }}</span> | ||
| <span class="tiny-dialog-box__title" role="heading" id="tiny_dialog_unique_0">{{ title }}</span> |
There was a problem hiding this comment.
Critical: Hardcoded IDs will collide with multiple dialog instances.
The hardcoded IDs tiny_dialog_unique_0 and tiny_dialog_unique_1 violate the HTML specification requirement that IDs must be unique within a document. When multiple dialog instances exist simultaneously, these duplicate IDs will:
- Break ARIA relationships (aria-labelledby/aria-describedby will reference ambiguous elements)
- Fail accessibility validation
- Create unpredictable screen reader behavior
🔎 Proposed fix using dynamic IDs
Generate unique IDs per component instance using a counter or the component's unique identifier:
+<script lang="ts">
+let dialogIdCounter = 0
+</script>
+
<script lang="ts">
import { renderless, api } from '@opentiny/vue-renderless/dialog-box/vue'
import { props, setup, defineComponent } from '@opentiny/vue-common'
// ...
export default defineComponent({
// ...
+ data() {
+ return {
+ dialogId: `tiny_dialog_${dialogIdCounter++}`
+ }
+ },
setup(props, context) {
return setup({ props, context, renderless, api }) as unknown as IDialogBoxApi
}
})
</script>Then update the template to use dynamic IDs:
<div
ref="dialog"
v-show="visible"
v-if="destroyOnClose ? visible : true"
:class="[...]"
:style="state.style"
class="tiny-dialog-box"
data-tag="tiny-dialog-box"
:data-dialog-box-draggable="draggable"
:key="state.key"
role="dialog"
- :aria-modal="true"
- aria-labelledby="tiny_dialog_unique_0"
- aria-describedby="tiny_dialog_unique_1"
+ aria-modal="true"
+ :aria-labelledby="`${dialogId}_title`"
+ :aria-describedby="`${dialogId}_body`"
>
<div v-if="showHeader" ref="header" class="tiny-dialog-box__header" @mousedown="handleDrag">
<slot name="title">
- <span class="tiny-dialog-box__title" role="heading" id="tiny_dialog_unique_0">{{ title }}</span>
+ <span class="tiny-dialog-box__title" role="heading" :id="`${dialogId}_title`">{{ title }}</span>
</slot>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/vue/src/dialog-box/src/pc.vue around lines 43 to 50 the template
uses hardcoded IDs tiny_dialog_unique_0 and tiny_dialog_unique_1 which will
collide across multiple dialog instances; replace them with per-instance dynamic
IDs (e.g., generate a unique suffix in the component setup/data using the
component uid or an incrementing counter) and bind those generated IDs to
aria-labelledby and aria-describedby and to the title/description element id
attributes so each dialog has stable, unique IDs across renders. Ensure the ID
generation runs once per component instance (not on every render) and is used
everywhere the template currently references the hardcoded IDs.
| </div> | ||
| </div> | ||
| <div class="tiny-dialog-box__body"> | ||
| <div class="tiny-dialog-box__body" id="tiny_dialog_unique_1"> |
There was a problem hiding this comment.
Critical: Hardcoded ID will collide with multiple dialog instances.
The hardcoded ID tiny_dialog_unique_1 will collide when multiple dialog instances exist. This is part of the same critical issue identified in the dialog root element (lines 43-50). Use a dynamic, instance-specific ID instead.
🤖 Prompt for AI Agents
In packages/vue/src/dialog-box/src/pc.vue around line 82, the body element uses
a hardcoded id "tiny_dialog_unique_1" which will collide across multiple
instances; replace it with a dynamic, instance-specific id (e.g., bind :id to a
generated property). Generate the unique id in the component (setup() or
data/created) using a per-instance counter or a random/UUID/crypto value and
assign it to a reactive property like dialogId, then change the template to bind
id to that property; also update any other places that reference the hardcoded
id (root element or JS logic) to use the new dialogId so all instance-specific
references remain consistent.
James-9696
force-pushed
the
fix-modal-aria
branch
from
f073479 to
21d7426
Compare
| role: 'dialog', | ||
| 'aria-modal': 'true', | ||
| 'aria-labelledby': 'modal_unique_0', | ||
| 'aria-describedby': 'modal_unique_1', |
There was a problem hiding this comment.
硬编码的ID mode_unique_0和mode_unique_1将创建重复的ID，这违反了HTML规范并破坏了ARI关系。
| @@ -250,7 +258,8 @@ export default defineComponent({ | |||
| h( | |||
| 'div', | |||
| { | |||
| class: 'tiny-modal__content' | |||
| class: 'tiny-modal__content', | |||
| id: 'modal_unique_1' | |||
2 participants
PR
添加后：
未添加：
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.