Open Aaronius opened 3 years ago
it appears to be that the act of opening a leaf node causes the issue
given that all the elements within an Accordion
will be leafs, we either need to fix that code in useTreeState
so it doesn’t break or we need to manage our own open state, I haven’t looked at useTreeState
to see if it’s better to fix it there or not
in addition, instead of item.props.children
, we should really be using item.rendered
like MenuItem
does
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/accordion/src/Accordion.tsx#L93 -> https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/menu/src/MenuItem.tsx#L74
a simple way to verify what I’m saying is to comment out the useAccordion
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/accordion/src/useAccordion.ts#L50
and set isOpen
to true in AccordionItem
, it renders open just fine, it’s only when you click to toggle that things go south
@Aaronius just copying our conversation here as well Author: Okay, interesting. Well, I’ll share what I’ve found, though it’s not entirely clear how it intersects with what you wrote and you probably already are aware of it. When you expand the AccordionItem, TreeCollection is being created and visits each item that is expanded (https://github.com/adobe/react-spectrum/blob/4b75cbe84157bcd735f7097a41f397d2c5c58543/packages/%40react-stately/tree/src/TreeCollection.ts#L29-L33). In that same snippet, it calls node.childNodes in order to iterate over each child node (because those child nodes may also be expanded items, I’m assuming). The node.childNodes property is this one here: https://github.com/adobe/react-spectrum/blob/4b75cbe84157bcd735f7097a41f397d2c5c58543/packages/%40react-stately/collections/src/CollectionBuilder.ts#L181-L200 which calls builder.getFullNode for the p child, which is where it errors out because type is p and there’s no getCollectionNode on the p component. The reason we don’t get the issue with Menu is because there’s never an item node in expandedKeys, so node.childNodes is never accessed on the item (see the first snippet I shared).
I think the intersection here is that the TreeCollection lines you noted are for things like Section, which would have a structure like this -> <Section><Item/><Item/></Section>
the check for expandedKeys.has(node.key) isn’t quite right though for things that should be a leaf, i think the distinction and reason I’m suggesting it may be better to track this state separately of the treeState is that expandedKeys has a meaning that it expects elements like Section and like Item inside those keys
any workaround for this issue to show jsx content inside accordion body.
It apparently has to do with JSX with non-empty title
so here's my hacky workaround. Not a good solution if you need the title to still appear when it's expanded.
https://codesandbox.io/s/accordion-content-forked-ncf8lo?file=/src/App.js
<Accordion
// Setting Accordion item title to empty when it's expanded
// to work around the issue that it breaks on rendering
// JSX content with non-empty title
// https://github.com/adobe/react-spectrum/issues/1989
onExpandedChange={(keys) => {
keys.forEach((key) => {
accordionItemTitles = { ...accordionItemTitles, [key]: "" };
});
Object.keys(titles).forEach((key) => {
if (!keys.has(key)) {
accordionItemTitles = {
...accordionItemTitles,
[key]: titles[key]
};
}
});
setAccordionItemTitles(accordionItemTitles);
}}
>
<Item key="item 1" title={accordionItemTitles["item 1"]}>
<p>Item 1 Content</p>
</Item>
<Item key="item 2" title={accordionItemTitles["item 2"]}>
<p>Item 2 Content</p>
</Item>
<Item key="item 3" title={accordionItemTitles["item 3"]}>
<p>Item 3 Content</p>
</Item>
</Accordion>
It apparently has to do with JSX with non-empty
title
so here's my hacky workaround. Not a good solution if you need the title to still appear when it's expanded. https://codesandbox.io/s/accordion-content-forked-ncf8lo?file=/src/App.js<Accordion // Setting Accordion item title to empty when it's expanded // to work around the issue that it breaks on rendering // JSX content with non-empty title // https://github.com/adobe/react-spectrum/issues/1989 onExpandedChange={(keys) => { keys.forEach((key) => { accordionItemTitles = { ...accordionItemTitles, [key]: "" }; }); Object.keys(titles).forEach((key) => { if (!keys.has(key)) { accordionItemTitles = { ...accordionItemTitles, [key]: titles[key] }; } }); setAccordionItemTitles(accordionItemTitles); }} > <Item key="item 1" title={accordionItemTitles["item 1"]}> <p>Item 1 Content</p> </Item> <Item key="item 2" title={accordionItemTitles["item 2"]}> <p>Item 2 Content</p> </Item> <Item key="item 3" title={accordionItemTitles["item 3"]}> <p>Item 3 Content</p> </Item> </Accordion>
This is possible workaround but affects the component accessibility. I would really appreciate if this issue could be solved.
Just discovered this issue. I submitted a duplicate (#3882) just a few hours ago. The accordian/disclosure component is a very widely used component and its a damn shame that ive had my team going gang busters with react-aria until we got a flying head kick to the face with a +12 month old issue. Completely understand devs are under the pump, but this is such a prevalent component pattern its a shame that its been tucked away in the backlog basement for so long...
Related https://github.com/adobe/react-spectrum/issues/3817
We do accept contributions. Accordion isn’t particularly high on our list at the moment because there are well-defined aria patterns using dl
, dd
, and dt
elements. If there are cross-browser inconsistencies with it, that would be a good candidate for contributing something.
Answered in #3882, the quick workaround is to specify hasChildItems={false}
on every Item
component. I believe the problem comes from this statement.
Obviously, it is just a workaround until a real fix is done.
<Accordion
{...props}
expandedKeys={openKeys}
onExpandedChange={setOpenKeys}
>
<Item key="files" title="Your files" hasChildItems={false}>
<p>files</p>
</Item>
<Item key="shared" title="Shared with you" hasChildItems={false}>
<p>shared</p>
</Item>
<Item key="last" title="Last item" hasChildItems={false}>
<p>last</p>
</Item>
</Accordion>
Answered in #3882, the quick workaround is to specify
hasChildItems={false}
on everyItem
component. I believe the problem comes from this statement. Obviously, it is just a workaround until a real fix is done.<Accordion {...props} expandedKeys={openKeys} onExpandedChange={setOpenKeys} > <Item key="files" title="Your files" hasChildItems={false}> <p>files</p> </Item> <Item key="shared" title="Shared with you" hasChildItems={false}> <p>shared</p> </Item> <Item key="last" title="Last item" hasChildItems={false}> <p>last</p> </Item> </Accordion>
It worked perfectly! Thank you @jbltx. Thank you react-spectrum for open-source all these great solutions. Cheers
Just to reiterate on what @snowystinger mentioned previously: Accordion isn't on our near term milestones so we are open to contributions for this/Accordion in general.
there are well-defined aria patterns using
dl
,dd
, anddt
elements.
I can't find these patterns or I misunderstood that they accomplish the accordion use case. Would you clarify and share some docs or examples?
Looks like I misspoke from memory. This is what I was actually thinking of https://github.com/adobe/react-spectrum/discussions/6635#discussioncomment-9990027
🐛 Bug Report
The Accordion component doesn't seem to support JSX content.
🤔 Expected Behavior
I would expect that when I render this:
that I would get an accordion that works properly by allowing me to expand and collapse the item.
😯 Current Behavior
I get the error
Unknown element <p> in collection.
. If I remove thep
tag and leave theparagraph content
text, the accordion works fine.💁 Possible Solution
🔦 Context
I'm trying to display content that's more complex than a simple string of text.
💻 Code Sample
https://codesandbox.io/s/accordion-content-j4q5i?file=/src/App.js
🌍 Your Environment
🧢 Your Company/Team
Adobe Experience Platform Data Collection
🕷 Tracking Issue (optional)