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?
There was a problem hiding this comment.
Since a function named isElementVisible already exists, I renamed this function to isElementVisibleByAttribute.
|
@cexbrayat |
| it('DetailContent should be invisible when display:none is applied by class', () => { | ||
| const style = document.createElement('style') | ||
| document.head.appendChild(style) | ||
| style.sheet!.insertRule('.hidden { display: none; }') |
There was a problem hiding this comment.
I'm afraid that this will leak into other tests. I would remove this test, the following one should be enough.
There was a problem hiding this comment.
I fixed this point. I remove this test case.
| * @returns boolean | ||
| */ | ||
| function isElementVisibleByAttribute<T extends Element>(element: T) { | ||
| if (element instanceof HTMLInputElement && (element.type === 'hidden' || element.hidden)) { |
There was a problem hiding this comment.
This now only checks hidden for HTMLInputElement whereas it was checking it for all HTMLElements previously.
And as I was mentioning before, the hidden check for inputs is a nice addition but should have a unit test (or even better, a dedicated PR).
There was a problem hiding this comment.
Thanks for pointing that out! You're right — my change unintentionally removed the hidden check for other HTMLElements. I restored the previous behavior and removed the unrelated input hidden change from this PR.
…lated detail display:none test
|
@cexbrayat |
|
@nn-morishita can you make sure format and tests are working (I suspect the removal of |
|
@cexbrayat |
|
@nn-morishita Thanks for the commit, but it looks like the formatting is still failing? |
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