Improve contextDocument handling in ColumnSizing feature

[parassantra]

What's this PR

#5746

Enhance the contextDocument assignment logic in the ColumnSizing feature:

• Use provided _contextDocument if available
• Fall back to event target's ownerDocument if possible
• Use global document as a last resort
• Handle cases where document is undefined

This change improves compatibility with different environments and provides
more robust document reference handling for column resizing events.

[parassantra]

@KevinVandy Please review when you have time.

[antoniobeltran]

Plus 1 on this. Following for updates

[parassantra]

@KevinVandy Please review this PR.

[lastrader]

Any update on this?

[rgollila]

Also interested in an update

[autoamesy]

Please review this PR.

[KevinVandy]

last time we touched this code, it caused server-side errors when window was being improperly referenced. We may need to add a nextjs or remix example to properly test this.

[parassantra]

@KevinVandy are we required to add a nextjs or remix example with fix in to check this is working?
That's what you are mentioning.

[riccardoperra]

last time we touched this code, it caused server-side errors when window was being improperly referenced. We may need to add a nextjs or remix example to properly test this.

@KevinVandy I think we can make use of globalThis to access data regardless of the environment

[parassantra]

@KevinVandy Please specify what next steps, I should take in order to get this reviewed and merged.

[parassantra]

@KevinVandy @riccardoperra any update on this, this is a critical issue on new windows created through portal. we should merge this fix asap. Please suggest next steps.

[lastrader]

@KevinVandy @riccardoperra any update on this, this is a critical issue on new windows created through portal. we should merge this fix asap. Please suggest next steps.

I've been watching this and hoping for the fix to be merged.

[KevinVandy]

I have not had the chance to make an ssr example to test this

[riccardoperra]

@KevinVandy I can make this weekend an example maybe using Angular SSR since it's a bit strict about how you use DOM Api (cannot reference document/window directly etc)

I never did this as I used React only on browser: could we add some ssr tests using something like renderToString etc? I could invest some time on it

EDIT: adding some test cases in table-core package using Node as environment for some test cases may help us to detect some errors

[riccardoperra]

@KevinVandy after some investigation while debugging with provided example in #5746, I found out that this line may be broken

In fact, even if _contextDocument is provided, it still goes to the next condition, which always returns the main document. We should wrap that condition in parenthesis.

_contextDocument || (typeof document !== 'undefined' ? document : null)

I think we can move this in an external function in order to do unit testing and be sure to reuse this logic if needed elsewhere

A possible fix here: 803ccf9 (just removed jsdom environment in order to run test without polluted globals. Using "typeof" should be safe. By doing this, it's no longer necessary to get the event's ownerDocument, but it might still be useful to have a utility for it. In this case

---

Anyway, @parassantra Your PopupWindow implementation may be revisited since it seems the table created in the window still have a popupDocument reference as null. I tried to do a bit of changes but I'm not really an expert in React so someone else should validate this.

interface PopoutWindowProps {
  children: (document: Document) => ReactNode // <-- a signature like exoticComponent / context provider
  closeWindow: () => void;
  setPopoutDocument: (doc: Document) => void;
  width?: number;
  height?: number;
}

// then in the return fn

return container && windowDocument // <-- should use some state ref here
    ? ReactDOM.createPortal(
        <>
          {children(windowDocument)}
          <button onClick={() => popoutWindowRef.current?.close()}>
            Close Window
          </button>
        </>,
        container
      )
    : null;

// outside 

<PopupWindow>
 {document => (
   <ResizableTable popupDocument={document}/>
 )}
</PopupWindow>

[parassantra]

@riccardoperra @KevinVandy
Here are three examples:

1. tried on my local with React [803ccf9] fix(803ccf9)

issue is still there see video attached:

803ccf9_not_working.mov

---

2. tried on my local with my proposed fix on React:
it is working fine on react:

my_fix_working.mov

3. tried on my local with my proposed fix on Next Js:
it is working fine on Next Js also, so there is no SSR issue.

my_fix_next_js.mov

Please review on your end. This issue is causing us trouble since a long time.

[riccardoperra]

@parassantra I forked your example: https://codesandbox.io/p/sandbox/nostalgic-bhaskara-z6n8gf?file=%2Fsrc%2Fcomponents%2FPopoutWidow.tsx%3A19%2C1

using packages from pkg-pr-new (#5989) seems fixing the bug, but as I said I had to refactor PopupWindow

[parassantra]

@parassantra I forked your example: https://codesandbox.io/p/sandbox/nostalgic-bhaskara-z6n8gf?file=%2Fsrc%2Fcomponents%2FPopoutWidow.tsx%3A19%2C1

using packages from pkg-pr-new (#5989) seems fixing the bug, but as I said I had to refactor PopupWindow

Oh ok, I got it, I will try with updated code.