refactor: Centralize expandedKeys logic in TreeCollection#9711
refactor: Centralize expandedKeys logic in TreeCollection#9711devongovett merged 2 commits intomainfrom
Conversation
|
Build successful! 🎉 |
| get level(): number { | ||
| if (this.parentNode instanceof ElementNode) { | ||
| return this.parentNode.level + (this.node?.type === 'item' ? 1 : 0); | ||
| return this.parentNode.level + (this.parentNode.node?.type === 'item' ? 1 : 0); |
There was a problem hiding this comment.
The logic here was incorrect: we should increment the level if the parent is an item, not if the child is. This means that loaders and other elements have the same level as their siblings.
|
Build successful! 🎉 |
reidbarber
left a comment
reidbarber
left a comment
There was a problem hiding this comment.
LGTM, tested Tree and GridList components.
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Behavior looks good to me so far, nothing amiss that I can tell about the collection structure or any of the behaviors/attributes applied from querying the collection.
| let [lastCollection, setLastCollection] = useState(collection); | ||
| let [lastExpandedKeys, setLastExpandedKeys] = useState(expandedKeys); | ||
| let [flattenedCollection, setFlattenedCollection] = useState(() => new TreeCollection<object>({collection, lastExpandedKeys: new Set(), expandedKeys})); | ||
| let [flattenedCollection, setFlattenedCollection] = useState(() => collection.withExpandedKeys(lastExpandedKeys, expandedKeys)); |
There was a problem hiding this comment.
I was initially concerned about this since now the "flattened" collection isn't actually flattened any more and thus things like itemCount always include rows that are in collapsed parents, but so far it doesn't seem like there are any issues (it helps that most of the collection size checks are if the collection is empty).
The only change in behavior I've found so far is with DnD where starting a keyboard drag on the first child of a parent and pressing ArrowUp actually loops you back to the drop position after the last child of the parent. You need to then traverse up through all of the children before escaping the parent, but the previous behavior was also broken but in a different way so we'll have to revisit
There was a problem hiding this comment.
hmm I guess aria-rowcount would be affected for virtualized trees. Seems like we don't have any tests for that? That's very unfortunate. I wonder what is correct in that case?
There was a problem hiding this comment.
yeah I'm surprised I don't have a test for that either ugh. Testing https://www.w3.org/WAI/ARIA/apg/patterns/treegrid/examples/treegrid-1/, it seems to omit the rows in collapsed sections in the announcement in VO (note this treegrid doesn't use aria-rowcount) so it seems like we'll still have to somehow account for that. Maybe we can address this via the eventual "number of item of type" change we've always talked about adding to collection nodes
There was a problem hiding this comment.
yeah it's an interesting case because the rows do still exist, they are just collapsed. So I could see it both ways.
There was a problem hiding this comment.
yeah I can definitely see that being correct too, I assume a screen reader user would pass by collapsed rows and could be clued in that the row count is the total row count, not what is available immediately. The blurb about aria-rowcount in https://www.w3.org/WAI/ARIA/apg/practices/grid-and-table-properties/ could also be read in favor of being the absolute total of rows:
When the number of rows represented by the DOM structure is not the total number of rows available for a table, grid, or treegrid, the aria-rowcount property is used to communicate the total number of rows available, and it is accompanied by the aria-rowindex property to identify the row indices of the rows that are present in the DOM.
The aria-rowcount is specified on the element with the table, grid, or treegrid role. Its value is an integer equal to the total number of rows available, including header rows. If the total number of rows is unknown, a value of -1 may be specified. Using a value of -1 indicates that more rows are available to include in the DOM without specifying the size of the available supply.
Using -1 could be an approach we could use as well, especially when it comes down to async loading combined with virtualization
There was a problem hiding this comment.
OK I think the argument for the total number is fairly strong. We can test and get feedback.
…aseline-tracker * origin/main: feat(S2): S2 ListView (adobe#8878) refactor: Centralize expandedKeys logic in TreeCollection (adobe#9711) chore: Warn if user has interactive elements in their custom Picker value (adobe#9710) feat: S2 unavailable menu item (adobe#9657) fix: Ensure that opening a submenu via enter/space moves focus to first item in submenu (adobe#9691) fix: prevent docs crash by making template elements always append children into .content (adobe#9703) docs(RAC): Add TreeSection docs (adobe#9699) docs(S2): add Typography search view (adobe#9524) docs(S2): fix clipping in Picker custom value AvatarGroup example (adobe#9702) fix: patch additional methods so React doesnt break with template elements (adobe#9385) tentative fix (adobe#9635) docs(S2): fix icon import clipboard content to add underscore for icons starting with number (adobe#9698) feat(S2): add ActionBar support to TreeView (adobe#9695) fix: combobox interactoutside (adobe#9646) fix: skip native Date fast path when local timezone is overridden via setLocalTimeZone (adobe#9678) chore: update storybook to 9 (adobe#8634) docs: improve custom render value S2 Picker example (adobe#9682) # Conflicts: # yarn.lock
5 participants
I noticed during release audit that we were handling
expandedKeysin bothTreeCollectionandListKeyboardDelegate. Also, we are still doing a full traversal over the entire collection on the initial render to adjust some indices. I have now centralized all of this withinTreeCollection.getLastKeyinTreeCollectionto traverse to the deepest expanded descendant, and removed this fromListKeyboardDelegate.TreeCollectionto extendBaseCollectioninstead of wrapping one. This allows us to access some of the internal properties by making themprotectedinstead ofprivate(avoiding making them externally public). UpdatedCollectionBuilderto construct it viacreateCollection, and setexpandedKeyslater.indexandlevelproperties intouseGridListItemduring rendering instead of updating the collection nodes themselves.