fix: correct <summary> element behavior#2806
Conversation
✅ Deploy Preview for vue-test-utils-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
cexbrayat
left a comment
cexbrayat
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I don't think this is enough though. The PR makes <summary> skip ancestor visibility checks entirely in isElementVisible.
That means a <summary> can be reported visible even when one of its ancestors is hidden (for example display: none), which is a regression because hidden ancestors should make descendants not visible.
pseudo-test to reproduce (did not run it, so maybe it needs tweaking):
it('should consider a summary as hidden when an ancestor is hidden', () => {
const HiddenAncestorSummary = defineComponent({
template: `
<div style="display: none;">
<details>
<summary>Summary</summary>
</details>
</div>
`
})
const wrapper = mount(HiddenAncestorSummary)
expect(wrapper.find('summary').isVisible()).toBe(false)
})I also think the PR treats any <summary> inside a <details> as visible, regardless of structure.
That is too broad: only the primary summary should be visible in a closed <details>. A nested <summary> inside closed details content should remain hidden.
it('should consider a summary as hidden when nested inside closed details content', () => {
const NestedSummaryInClosedDetails = defineComponent({
template: `
<details>
<summary>Main summary</summary>
<div>
<summary>Nested summary</summary>
</div>
</details>
`
})
const wrapper = mount(NestedSummaryInClosedDetails)
const summaries = wrapper.findAll('summary')
expect(summaries[0].isVisible()).toBe(true)
expect(summaries[1].isVisible()).toBe(false)
});
|
@nn-morishita are you still interested in fixing the issue or should we close the PR? |
|
@cexbrayat Sorry for the delayed response. I’ve just pushed a new commit addressing the feedback. I’d appreciate it if you could take a look when you have a chance. |
commit: |
| ) | ||
| if (element instanceof HTMLInputElement && element.type === 'hidden') { | ||
| return false | ||
| } |
There was a problem hiding this comment.
This change is unrelated to the issue, but an interesting addition. Could you either:
- add a unit test in this PR
- remove this change from this PR and open another one with a unit test
There was a problem hiding this comment.
As the funciton no longer use attribute checking, maybe rename it to isElementVisible?
| }) | ||
| expect(wrapper.isVisible()).toBe(false) | ||
| }) | ||
| it('DetailContent should be visible when summary is visible', () => { |
There was a problem hiding this comment.
the tests are under an unrelated test suite, maybe move them a new test suite:
describe('details and summary elements', () => {
// test cases here
})| expect(wrapper.find('details').isVisible()).toBe(true) | ||
| expect(wrapper.find('summary').isVisible()).toBe(true) | ||
| expect(wrapper.find('div').isVisible()).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Maybe add a test case, like:
template: `<details><summary><span>Summary</span></summary><div>Content</div></details>`
expect(wrapper.find('summary span').isVisible()).toBe(true)| return true | ||
| } | ||
|
|
||
| function isHiddenByClosedDetails<T extends Element>(element: T) { |
There was a problem hiding this comment.
Maybe add a few lines of jsdoc to explain what this function does
2 participants
This PR addresses #2626.
The issue is that
I verified this in a real browser, and the issue still occurs.
Reproduction:
https://stackblitz.com/edit/vitejs-vite-qxxwzrgz?file=src%2Fcomponents%2Ftests%2FDetailsComponent.test.ts