fix: SSR Combobox inner ref lost #7663

snowystinger · 2025-01-24T02:54:55Z

I've removed a bit of code from the contribution around mergeProps, it was deemed too much for this PR. We would like to benchmark mergeProps and see if there is a real benefit and how much instead of a theoretical one. That will help us decide if we can risk changing the behaviour.

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

nwidynski · 2025-01-24T23:05:01Z

@snowystinger Took some time as well to look at this. My first intuition was also the fake DOM, but after noticing the issue being fixed by splicing the "non-collection" content from the builder, i was getting suspicious. Especially since only <ListBox /> content should have the required <Collection /> wrapper to even be hitting the fake DOM.

After additional investigation, it appears the issue is stemming from the switch in rendering strategies after initial hydration. The switch causes refs attached inside template to loose connection after moving to the portal. I tried assigning a stable key for both template and createPortal, but also to no avail.

The issue can only be fixed by maintaining a consistent strategy throughout the render cycle. (e.g. memoizing the useIsSSR() at hydration or by using template in both scenarios). I'm unsure whether this is possibly related to facebook/react#13563 or intended behavior.

In regards to the issuer objective, we currently calculate menuWidth with the rects of inputRef and buttonRef, so fixing this issue won't have impact on that. Unfortunately I can relate to the expectation of var(--trigger-width) to be dependent on triggerRef. Maybe we rename or even consider calculating based off triggerRef while clamping if smaller than input + button width as a courtesy. I feel like this could help avoid confusion moving forward 👍

nwidynski · 2025-02-03T20:20:19Z

@snowystinger Just pinging here in case it was missed. I can pick this up if you lack resources at the moment 👍

snowystinger · 2025-02-03T20:48:41Z

Any help is appreciated, i'm hoping to get a chance to look at this one and [RAC] Table components do not play well with Suspense this week with some of the team.

* feat: auto-adjust menu width for custom triggers * fix: test assert label compatibility * fix: rules of react * feat: popover minWidth for custom trigger elements * fix: use resize observer * chore: reset combobox story

rspbot · 2025-02-07T03:31:37Z

Build successful! 🎉

rspbot · 2025-02-07T03:32:36Z

Build successful! 🎉

snowystinger · 2025-02-07T03:22:26Z

packages/@react-aria/collections/src/Hidden.tsx

@@ -43,6 +43,8 @@ const hiddenFragment = typeof DocumentFragment !== 'undefined' ? new DocumentFra
 export function Hidden(props: {children: ReactNode}) {
  let isHidden = useContext(HiddenContext);
  let isSSR = useIsSSR();
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  let wasSSR = useMemo(() => isSSR, []);


See comment here for why #7710 (comment)
Concern being that we'll now always render into template if it's an SSR app. This may come with a performance hit, we'll need to figure out a way to benchmark this.

Hm, a quick MDN check tells me a template element without shadow root basically is a DocumentFragment.
Do you have specific concerns on what may cause reflow or repaint or is it just sanity check for now?

I was also chewing on whether useMemo may be unsuited here, as React may discard the cache in some scenarios causing our issue to reappear. Maybe a ref would be the better alternative.

sanity check mostly

even though it's just a document fragment, the nodes would be bigger in memory and there may be more to the logic of parenting/validation/etc, so it may be slower to add and remove nodes even without the consideration of reflow or paint

That's a good call out about the useMemo, I'll update it to a ref

snowystinger · 2025-02-07T03:23:28Z

packages/react-aria-components/src/Popover.tsx

@@ -92,6 +92,27 @@ export const Popover = /*#__PURE__*/ (forwardRef as forwardRefType)(function Pop
  let isHidden = useIsHidden();
  let {direction} = useLocale();

+  // We can set minWidth to the trigger width as a courtesy for custom trigger elements.


is this something we want to do?

decided it's not, if i have a large trigger but want a smaller popover, then I can't do that now

I purposefully spread the users style over the minWidth to allow for overrides in case someone needs a smaller popover.

Ah yes, that is correct.

Unfortunately I think it'd also be a breaking change. People may be relying on it being smaller than the trigger width right now. They'd have to override it to get that behaviour back.

It would also add an extra resize observer to every popover which seems excessive.

Maybe it'd be useful to encapsulate the logic from there into a utility hook to help people manage the width. But I don't think we can default so it in every popover.

rspbot · 2025-02-07T04:26:56Z

Build successful! 🎉

snowystinger · 2025-02-07T05:11:02Z

I verified the fix not just with the test. I also ran verdaccio and added the stackblitz example to it

interface MyComboBoxProps<T extends object>
  extends Omit<ComboBoxProps<T>, 'children'> {
  label?: string;
  description?: string | null;
  errorMessage?: string | ((validation: ValidationResult) => string);
  children: React.ReactNode | ((item: T) => React.ReactNode);
}

function MyComboBox<T extends object>({
  label,
  description,
  errorMessage,
  children,
  ...props
}: MyComboBoxProps<T>) {
  const triggerRef = React.useRef<HTMLDivElement>(null);
  const [triggerWidth, setTriggerWidth] = useState<string | null>(null);

  const onResize = React.useCallback(() => {
    console.log(triggerRef.current);
    if (triggerRef.current) {
      setTriggerWidth(triggerRef.current.offsetWidth + 'px');
    }
  }, [triggerRef]);

  useLayoutEffect(() => {
    onResize();
  }, []);

  useResizeObserver({
    ref: triggerRef,
    onResize: onResize,
  });
  console.log(triggerWidth);

  return (
    <RACComboBox {...props}>
      <RACLabel>{label}</RACLabel>
      <div ref={triggerRef} className="my-combobox-container">
        <RACInput />
        <RACButton>▼</RACButton>
      </div>
      {description && <Text slot="description">{description}</Text>}
      <RACFieldError>{errorMessage}</RACFieldError>
      <RACPopover triggerRef={triggerRef}>
        <RACListBox>{children}</RACListBox>
      </RACPopover>
    </RACComboBox>
  );
}

function MyItem(props: ListBoxItemProps) {
  return (
    <RACListBoxItem
      {...props}
      className={({ isFocused, isSelected }) =>
        `my-item ${isFocused ? 'focused' : ''} ${isSelected ? 'selected' : ''}`
      }
    />
  );
}

rspbot · 2025-02-07T20:20:57Z

Build successful! 🎉

nwidynski · 2025-02-10T18:46:35Z

@snowystinger I became curios why React reconciliation was breaking down here so I began digging further. My first suspicion was our template element patch, but turns out its not and there is a way to keep our client portals 😄

Here is what is happening underneath, which I validated by patching HTMLDivElement to debug fiber creation:

React creates 2 fiber trees for each collection render pass (1 hidden AST & 1 visible AST)
When exiting hydration, the hidden AST is moved and is assigned new fibers while the visible fibers are recycled
As the ref is basically the last assigned fiber, the result becomes it pointing to the hidden AST

With that knowledge, we can attempt to force CollectionBuilder to spawn new fibers for the visible AST:

<CollectionInner render={props.children} collection={collection} key={useIsSSR() ? 'server' : 'client'} />

It should allow us to revert the fix in Hidden as it becomes obsolete. The only downside to this new approach is that all state will be re-constructed upon the second client render (e.g. parent state update) due to aforementioned key change.

Hopefully this is less impactful on the metrics while also preserving a prettier output 👍

snowystinger · 2025-02-11T00:57:05Z

It looks like it's correct; I've updated the test to reflect the reality a bit better. Thank you so incredibly much for diving into that.

rspbot · 2025-02-11T01:21:53Z

Build successful! 🎉

rspbot · 2025-02-11T01:40:11Z

Build successful! 🎉

snowystinger added 2 commits January 24, 2025 13:53

fix: SSR Combobox inner ref lost

55063b0

fix lint

0784211

nwidynski mentioned this pull request Jan 27, 2025

[RAC] Refs in collection components point to their "hidden" node after SSR #7250

Open

nwidynski mentioned this pull request Feb 3, 2025

feat: auto-adjust menu width for custom triggers #7710

Merged

5 tasks

feat: auto-adjust menu width for custom triggers (#7710)

2cb2cf6

* feat: auto-adjust menu width for custom triggers * fix: test assert label compatibility * fix: rules of react * feat: popover minWidth for custom trigger elements * fix: use resize observer * chore: reset combobox story

snowystinger marked this pull request as ready for review February 7, 2025 03:21

Merge branch 'main' into fix-ssr-combobox-inner-ref

7d0573b

snowystinger commented Feb 7, 2025

View reviewed changes

remove minimum size on popovers

b71e558

use more stable ref instead of memo

c7953b7

update based on new information

8728a98

Merge branch 'main' into fix-ssr-combobox-inner-ref

248ee90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SSR Combobox inner ref lost #7663

fix: SSR Combobox inner ref lost #7663

snowystinger commented Jan 24, 2025 •

edited

Loading

nwidynski commented Jan 24, 2025 •

edited

Loading

nwidynski commented Feb 3, 2025 •

edited

Loading

snowystinger commented Feb 3, 2025

rspbot commented Feb 7, 2025

rspbot commented Feb 7, 2025

snowystinger Feb 7, 2025

nwidynski Feb 7, 2025

snowystinger Feb 7, 2025

snowystinger Feb 7, 2025

snowystinger Feb 7, 2025

nwidynski Feb 7, 2025

snowystinger Feb 7, 2025

rspbot commented Feb 7, 2025

snowystinger commented Feb 7, 2025 •

edited

Loading

rspbot commented Feb 7, 2025

nwidynski commented Feb 10, 2025 •

edited

Loading

snowystinger commented Feb 11, 2025

rspbot commented Feb 11, 2025

rspbot commented Feb 11, 2025

fix: SSR Combobox inner ref lost #7663

Are you sure you want to change the base?

fix: SSR Combobox inner ref lost #7663

Conversation

snowystinger commented Jan 24, 2025 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

nwidynski commented Jan 24, 2025 • edited Loading

nwidynski commented Feb 3, 2025 • edited Loading

snowystinger commented Feb 3, 2025

rspbot commented Feb 7, 2025

rspbot commented Feb 7, 2025

snowystinger Feb 7, 2025

Choose a reason for hiding this comment

nwidynski Feb 7, 2025

Choose a reason for hiding this comment

snowystinger Feb 7, 2025

Choose a reason for hiding this comment

snowystinger Feb 7, 2025

Choose a reason for hiding this comment

snowystinger Feb 7, 2025

Choose a reason for hiding this comment

nwidynski Feb 7, 2025

Choose a reason for hiding this comment

snowystinger Feb 7, 2025

Choose a reason for hiding this comment

rspbot commented Feb 7, 2025

snowystinger commented Feb 7, 2025 • edited Loading

rspbot commented Feb 7, 2025

nwidynski commented Feb 10, 2025 • edited Loading

snowystinger commented Feb 11, 2025

rspbot commented Feb 11, 2025

rspbot commented Feb 11, 2025

snowystinger commented Jan 24, 2025 •

edited

Loading

nwidynski commented Jan 24, 2025 •

edited

Loading

nwidynski commented Feb 3, 2025 •

edited

Loading

snowystinger commented Feb 7, 2025 •

edited

Loading

nwidynski commented Feb 10, 2025 •

edited

Loading