DisclosurePanel doesn't show when Disclosure is expanded and disabled
vicky-comeau opened this issue · 9 comments
Provide a general summary of the issue here
When creating a basic Disclosure component with react aria components, the DisclosurePanel gets a hidden
attribute when defaultExpanded
and isDisabled
are set on the disclosure. It is not hidden="until-found"
it is only hidden
.
I have no idea what sets this and I didn't see it in spectrum's V2 storybook: https://reactspectrum.blob.core.windows.net/reactspectrum/6de1ad4d4bab6d6debacf576b8ba325871064e7e/storybook-s2/index.html?path=/docs/accordion--docs
🤔 Expected Behavior?
I'd expect the disclosure panel to be visible if defaultExpanded
or isExpanded
is set to true and the disclosure is disabled. No hidden
attribute should be added.
😯 Current Behavior
A hidden
attribute is added to the disclosure panel and it is not visible when disabled and expanded.
💁 Possible Solution
Find why the hidden attribute is added and remove it if the panel is expanded.
🔦 Context
I am creating a Disclosure component for our design system and I was creating some chromatic tests when I noticed this issue. It may not be something that comes up, but the fact that it's possible to do, I think it should behave as expected.
🖥️ Steps to Reproduce
https://codesandbox.io/p/devbox/p26t7n?file=%2Fsrc%2FApp.tsx%3A11%2C8
This was the code:
<Disclosure isDisabled defaultExpanded>
<Button>test</Button>
<DisclosurePanel>test</DisclosurePanel>
</Disclosure>
Version
react-aria-components: 1.5.0
What browsers are you seeing the problem on?
Microsoft Edge
If other, please specify.
No response
What operating system are you using?
Windows 11
🧢 Your Company/Team
No response
🕷 Tracking Issue
No response
Off topic but the S2 storybook link you shared seems outdated? I'm not sure where/when you got that link but I noticed that the hidden=until-found
wasn't working so here's an updated version from the latest on main . At least in Chrome, you should see hidden=until-found
applied on the panel.
Anyway, if we remove isDisabled
from this line, then it works as you suggested.
react-spectrum/packages/@react-aria/disclosure/src/useDisclosure.ts
Lines 77 to 80 in c266dc2
However, if we do that, then those using find-on-page search will be able to open a closed and disabled disclosure which I don't think we want. I think the team will need to discuss whether this is the intended behavior or if this should be allowed.
What version of RAC are you using? We recently had a release, please check if you're on the latest.
I double checked, it says version 1.5.0
I think you might previously have been using an older version as the sandbox you shared uses an old API
I've updated the sandbox https://codesandbox.io/p/devbox/react-aria-disclosure-forked-vtyv4v
Now the button disables correctly.
As for not applying hidden. It would appear to be a difference in which browsers support beforeMatch
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/disclosure/src/useDisclosure.ts#L117
I think if the disclosure is disabled, then hidden should be true.
Thanks, yes the button disables now.
Are you saying that even if we tell the disclosure to be expanded, since it's disabled, we should not show the panel?
Yes, I think so. Though welcome to discuss it more.
One use case I'm basing my thoughts on is this https://www.w3.org/WAI/ARIA/apg/patterns/disclosure/examples/disclosure-navigation/
Which we may eventually support with these components.
The popover shouldn't be able to be open if the trigger is disabled, otherwise there's no way to close it.
In addition, what is the point of a disabled and open disclosure? I don't think it makes much sense, the whole purpose of a disclosure is to hide content until the user reveals it. It makes the most sense for it to always be hidden by default. If it must be visible from the get go, then it needs to be hideable so the user can at least choose to hide it.
Finally, if we should support the optional keyboard navigation between the disclosures in a disclosure group at some point, then it'd skip a disabled disclosure and that would make the content harder to find. A blind user may be completely unaware that they are skipping a section of content.
If you need to disable the disclosure but show the content, then I think the best thing to do is not use a disclosure.
I agree with you. I think you make some very logical points.
I'm just wondering how it would behave if for example,
- The user opens the disclosure but does something and now it has become disabled. Does data-expanded get set back to false? (This does not happen at the moment)
- A dev sets
defaultExpanded
on all disclosures on the page and some of them are also disabled by default. This is similar to the previous example. Should RAC's Disclosure set data-expanded to false since it is now disabled? Is it required that the dev setisExpanded
back to false?
I guess I'm just trying to think of all possible scenarios. We may forget to set isExpanded back to false if it suddenly becomes disabled. Is RAC missing something or is there no bug here at all?
- I think this is a bug, it should probably close if it becomes disabled, as some users will not be able to access the information
- I'm confused.
defaultExpanded
is uncontrolled, butisExpanded
is controlled. The data attribute should match whatever state the disclosure is actually rendering in.