DisclosurePanel doesn't show when Disclosure is expanded and disabled

[vicky-comeau]

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

[yihuiliao]

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

	if (supportsBeforeMatch && ref.current && !isDisabled) {
	if (state.isExpanded) {
	ref.current.removeAttribute('hidden');
	} else {

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.

[vicky-comeau]

Thanks for the updated link.
But why is it that spectrum works as intended:

but in my example, the panel is hidden and we see the hidden attribute

I also noticed that the hidden attribute is not added in Firefox.

[snowystinger]

What version of RAC are you using? We recently had a release, please check if you're on the latest.

[vicky-comeau]

I double checked, it says version 1.5.0

[snowystinger]

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.

[vicky-comeau]

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?

[snowystinger]

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.

[vicky-comeau]

I agree with you. I think you make some very logical points.
I'm just wondering how it would behave if for example,

1. 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)
2. 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 set isExpanded 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?

[snowystinger]

1. 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
2. I'm confused. defaultExpanded is uncontrolled, but isExpanded is controlled. The data attribute should match whatever state the disclosure is actually rendering in.

[jakubriedl]

I have another variant of this bug with the same cause. When I want to disable and close disclosure at the same time it doesn't close. Basically setting isExpanded: false and isDisabled: true in the same render disables the Disclosure but doesn't close it.

Repro: https://codesandbox.io/p/sandbox/gracious-tristan-4qpxt4?file=%2Fsrc%2FApp.js%3A32%2C11

I'd suggest remove the isDisabled from the condition mentioned in comment above and let the developer control these fields independently. But I understand that there might be some reason why to control these together and for example prevent disabled disclosure from being expanded.

Workaround I've used for now is adding "display:none" on the panel when !isExpanded && isDisabled

<Disclosure {...}>
  {({isDisabled, isExpanded}) => (
      // ...
      <DisclosurePanel
            className={clsx(
              !isExpanded && isDisabled && "hidden",
            )}
          >
            {children}
      </DisclosurePanel>
  )}
</Disclosure>