Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accordion defaultValue is not respected during SSR #226

Closed
katywings opened this issue Jun 17, 2023 · 0 comments · Fixed by #228
Closed

Accordion defaultValue is not respected during SSR #226

katywings opened this issue Jun 17, 2023 · 0 comments · Fixed by #228

Comments

@katywings
Copy link

Describe the bug
During SSR the Accordion.Roots defaultValue is ignored, resulting in a cumulative layout shift during client side hydration.

To Reproduce
Steps to reproduce the behavior:

  1. Create an Accordion component
  2. Make sure to set a proper defaultValue on the Accordion.Root
  3. Check the client side result of the accordion, it should properly open the corresponding Accordion item
  4. Disable Javascript in the Browser & Refresh the browser
  5. If you are in an environment supporting SSR, the rendered Accordion will be closed

Expected behavior
During SSR the defaultValue should be respected, and the corresponding items should be opened.

Additional context
Copied from: https://discord.com/channels/722131463138705510/1063803756388548709/1119218061715701822
This is a detailed analysis of the bug and proposals how to solve it:

To summarize the situation:

  1. isSelected of SelectionManager uses getKey before checking if the key is actually in selectedKeys()
  2. getKey only returns the key, if an item exists with such a key. In several places getKey is used as an existence check.
  3. Items only get set during mount via createDomCollectionItem which for obvious reasons doesnt happen in SSR.
  4. Since items dont get set in SSR but isSelected through getKey expects the corresponding item to exist, isSelected always returns false during SSR.

Proposals:
A: Since getKey is used in many places as an item existence check, removing this existence check in getKey by enabling the line ade42ab#diff-ff55d66528c6c8cad6d85d60a65d1a639e39c9b5f33d4c644bb4410d83b5750dR217 would likely break a lot of functionality. I propose adding an optional parameter ignoreCollection to getKey. Then selectedKeys can use getKey({ ignoreCollection: true }) which would return the key even if it doesnt exist in the items collection.

B: As an alternative to A, isSelected could be changed to compare selectedKeys in any case, like so:

isSelected(key) {
    if (this.state.selectionMode() === "none") {
        return false;
    }
    const retrievedKey = this.getKey(key);
    //if (retrievedKey == null) {
    //    return false;
    //}
    return this.state.selectedKeys().has(retrievedKey != null ? retrievedKey: key);
}

C: As a third alternative, we could implemented a getItem function. All places which currently use getKey as existence check, could then use getItem instead of getKey and so we could enable the

if (!item) {
    return key;
}

lines in getKey, without breaking other functionality.

Personally I prefer option B, but let me know what you think :D. If you are interested I can create a PR with a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant