search

mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.01k stars 1.24k forks source link

[data grid] tests fail when calling setCellFocus #13811

Closed kyle-seely closed 3 weeks ago

kyle-seely commented 1 month ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/devbox/wandering-sea-t38q4v?file=%2Fsrc%2FDemo.test.tsx

Steps:

  1. Load the codesandbox link. Notice that the cell with ID 1 is focused.
  2. Run the unit test that renders the DataGridDemo
  3. Test fails.

Current behavior

Test fails with stack trace:

Error: Uncaught [TypeError: Cannot read properties of undefined (reading 'current')]
        at reportException (/project/workspace/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
        at innerInvokeEventListeners (/project/workspace/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:353:9)
        at invokeEventListeners (/project/workspace/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:286:3)
        at HTMLUnknownElementImpl._dispatch (/project/workspace/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:233:9)
        at HTMLUnknownElementImpl.dispatchEvent (/project/workspace/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:104:17)
        at HTMLUnknownElement.dispatchEvent (/project/workspace/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:241:34)
        at Object.invokeGuardedCallbackDev (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:4213:16)
        at invokeGuardedCallback (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:4277:31)
        at reportUncaughtErrorInDEV (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:22877:5)
        at captureCommitPhaseError (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:27165:5)
        at commitPassiveMountEffects_complete (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24932:9)
        at commitPassiveMountEffects_begin (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24917:7)
        at commitPassiveMountEffects (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24905:3)
        at flushPassiveEffectsImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:27078:3)
        at flushPassiveEffects (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:27023:14)
        at commitRootImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26974:5)
        at commitRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26721:5)
        at performSyncWorkOnRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26156:3)
        at flushSyncCallbacks (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:12042:22)
        at commitRootImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26998:3)
        at commitRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26721:5)
        at finishConcurrentRender (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26020:9)
        at performConcurrentWorkOnRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:25848:7)
        at flushActQueue (/project/workspace/node_modules/react/cjs/react.development.js:2667:24)
        at act (/project/workspace/node_modules/react/cjs/react.development.js:2582:11)
        at /project/workspace/node_modules/@testing-library/react/dist/act-compat.js:47:25
        at renderRoot (/project/workspace/node_modules/@testing-library/react/dist/pure.js:180:26)
        at render (/project/workspace/node_modules/@testing-library/react/dist/pure.js:271:10)
        at Object. (/project/workspace/src/Demo.test.tsx:7:9)
        at Promise.then.completed (/project/workspace/node_modules/jest-circus/build/utils.js:298:28)
        at new Promise ()
        at callAsyncCircusFn (/project/workspace/node_modules/jest-circus/build/utils.js:231:10)
        at _callCircusTest (/project/workspace/node_modules/jest-circus/build/run.js:316:40)
        at _runTest (/project/workspace/node_modules/jest-circus/build/run.js:252:3)
        at _runTestsForDescribeBlock (/project/workspace/node_modules/jest-circus/build/run.js:126:9)
        at run (/project/workspace/node_modules/jest-circus/build/run.js:71:3)
        at runAndTransformResultsToJestFormat (/project/workspace/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
        at jestAdapter (/project/workspace/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
        at runTestInternal (/project/workspace/node_modules/jest-runner/build/runTest.js:367:16)
        at runTest (/project/workspace/node_modules/jest-runner/build/runTest.js:444:34) {
      detail: TypeError: Cannot read properties of undefined (reading 'current')
          at Object.scroll (/project/workspace/node_modules/@mui/x-data-grid/node/hooks/features/scroll/useGridScroll.js:110:75)
          at /project/workspace/node_modules/@mui/x-data-grid/node/components/cell/GridCell.js:246:24
          at commitHookEffectListMount (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:23189:26)
          at commitPassiveMountOnFiber (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24970:11)
          at commitPassiveMountEffects_complete (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24930:9)
          at commitPassiveMountEffects_begin (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24917:7)
          at commitPassiveMountEffects (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:24905:3)
          at flushPassiveEffectsImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:27078:3)
          at flushPassiveEffects (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:27023:14)
          at commitRootImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26974:5)
          at commitRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26721:5)
          at performSyncWorkOnRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26156:3)
          at flushSyncCallbacks (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:12042:22)
          at commitRootImpl (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26998:3)
          at commitRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26721:5)
          at finishConcurrentRender (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:26020:9)
          at performConcurrentWorkOnRoot (/project/workspace/node_modules/react-dom/cjs/react-dom.development.js:25848:7)
          at flushActQueue (/project/workspace/node_modules/react/cjs/react.development.js:2667:24)
          at act (/project/workspace/node_modules/react/cjs/react.development.js:2582:11)
          at /project/workspace/node_modules/@testing-library/react/dist/act-compat.js:47:25
          at renderRoot (/project/workspace/node_modules/@testing-library/react/dist/pure.js:180:26)
          at render (/project/workspace/node_modules/@testing-library/react/dist/pure.js:271:10)
          at Object. (/project/workspace/src/Demo.test.tsx:7:9)
          at Promise.then.completed (/project/workspace/node_modules/jest-circus/build/utils.js:298:28)
          at new Promise ()
          at callAsyncCircusFn (/project/workspace/node_modules/jest-circus/build/utils.js:231:10)
          at _callCircusTest (/project/workspace/node_modules/jest-circus/build/run.js:316:40)
          at _runTest (/project/workspace/node_modules/jest-circus/build/run.js:252:3)
          at _runTestsForDescribeBlock (/project/workspace/node_modules/jest-circus/build/run.js:126:9)
          at run (/project/workspace/node_modules/jest-circus/build/run.js:71:3)
          at runAndTransformResultsToJestFormat (/project/workspace/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
          at jestAdapter (/project/workspace/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
          at runTestInternal (/project/workspace/node_modules/jest-runner/build/runTest.js:367:16)
          at runTest (/project/workspace/node_modules/jest-runner/build/runTest.js:444:34),
      type: 'unhandled exception'
    }

Expected behavior

Test passes.

Context

I'm trying to unit test a component that auto-focuses a given cell in a data grid.

Your environment

npx @mui/envinfo System: OS: Linux 6.1 Ubuntu 20.04.6 LTS (Focal Fossa) Binaries: Node: 20.12.1 - /home/codespace/nvm/current/bin/node npm: 10.5.0 - /home/codespace/nvm/current/bin/npm pnpm: 8.15.6 - /home/codespace/nvm/current/bin/pnpm Browsers: Chrome: Not Found npmPackages: @emotion/react: latest => 11.11.4 @emotion/styled: latest => 11.11.5 @mui/base: 5.0.0-beta.40 @mui/core-downloads-tracker: 5.16.1 @mui/material: latest => 5.16.1 @mui/private-theming: 5.16.1 @mui/styled-engine: 5.16.1 @mui/system: 5.16.1 @mui/types: 7.2.15 @mui/utils: 5.16.1 @mui/x-data-grid: latest => 7.10.0 @mui/x-internals: 7.10.0 @types/react: latest => 18.3.3 react: latest => 18.3.1 react-dom: latest => 18.3.1 typescript: latest => 5.5.3

Search keywords: setCellFocus

Order ID:

90051

oliviertassinari commented 1 month ago

It didn't look super close, but I like to take random issues and timebox myself into them (30 minutes here), it helps me stay anchored into the ground of what's going on in the different parts of the project. I don't intend to own the answer, consider my comment as a random comment from a community member.

It seems to be that the problem is that:

https://github.com/mui/mui-x/blob/c0af84d52e4464110cc0b1ab0020a4145da35deb/packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts#L60

is wrong. No, we can't do this because columnHeadersContainerRef is defined in the <GridHeaders> render function, which runs after useGridScroll() runs.

diff --git a/packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts b/packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts
index 8e5f2ba8d..81e6cb104 100644
--- a/packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts
+++ b/packages/x-data-grid/src/hooks/features/scroll/useGridScroll.ts
@@ -57,7 +57,6 @@ export const useGridScroll = (
 ): void => {
   const theme = useTheme();
   const logger = useGridLogger(apiRef, 'useGridScroll');
-  const colRef = apiRef.current.columnHeadersContainerRef!;
   const virtualScrollerRef = apiRef.current.virtualScrollerRef!;
   const visibleSortedRows = useGridSelector(apiRef, gridExpandedSortedRowEntriesSelector);

@@ -144,6 +143,7 @@ export const useGridScroll = (

   const scroll = React.useCallback<GridScrollApi['scroll']>(
     (params: Partial<GridScrollParams>) => {
+      const colRef = apiRef.current.columnHeadersContainerRef!;
       if (virtualScrollerRef.current && params.left !== undefined && colRef.current) {
         const direction = theme.direction === 'rtl' ? -1 : 1;
         colRef.current.scrollLeft = params.left;
@@ -156,7 +156,7 @@ export const useGridScroll = (
       }
       logger.debug(`Scrolling, updating container, and viewport`);
     },
-    [virtualScrollerRef, theme.direction, colRef, logger],
+    [virtualScrollerRef, theme.direction, logger],
   );

   const getScrollPosition = React.useCallback<GridScrollApi['getScrollPosition']>(() => {

would make more sense to me.

As for why it works in the browser and fails in jsdom? It seems that it's simply because in jsdom the grid can't measure anything, it gets tricked into thinking that he needs to scroll, so it only fires scroll() in jsdom and not in the browser, which surfaces the wrong function dependency. We could potentially fix this too (give up on scroll if there is no layouting capabilities in the environment the test runs into).

@kyle-seely Why are you running this component in jsdom? What are you trying to test? I'm asking to help the team understand to which extent this is a valid use case for jsdom vs. it should be a test that runs in a real browser environment.

samwato commented 1 month ago

I too am facing this issue with jsdom

Digging through the code, would it make sense to add columnHeadersContainerRefto the list of registered private refs in useGridRefs.ts.

diff --git a/packages/x-data-grid/src/hooks/core/useGridRefs.ts b/packages/x-data-grid/src/hooks/core/useGridRefs.ts
--- a/packages/x-data-grid/src/hooks/core/useGridRefs.ts
+++ b/packages/x-data-grid/src/hooks/core/useGridRefs.ts
@@ -7,6 +7,7 @@
   const rootElementRef = React.useRef<HTMLDivElement>(null);
   const mainElementRef = React.useRef<HTMLDivElement>(null);
   const virtualScrollerRef = React.useRef<HTMLDivElement>(null);
+  const columnHeadersContainerRef = React.useRef<HTMLDivElement>(null);

   apiRef.current.register('public', {
     rootElementRef,
@@ -15,5 +16,6 @@
   apiRef.current.register('private', {
     mainElementRef,
     virtualScrollerRef,
+    columnHeadersContainerRef,
   });
 };
diff --git a/packages/x-data-grid/src/components/GridHeaders.tsx b/packages/x-data-grid/src/components/GridHeaders.tsx
--- a/packages/x-data-grid/src/components/GridHeaders.tsx
+++ b/packages/x-data-grid/src/components/GridHeaders.tsx
@@ -54,11 +54,7 @@
     cellTabIndexState === null
   );

-  const columnsContainerRef = React.useRef<HTMLDivElement>(null);
-
-  apiRef.current.register('private', {
-    columnHeadersContainerRef: columnsContainerRef,
-  });
+  const columnsContainerRef = apiRef.current.columnHeadersContainerRef;

   return (
     <rootProps.slots.columnHeaders
github-actions[bot] commented 1 month ago

The issue has been inactive for 7 days and has been automatically closed.

github-actions[bot] commented 3 weeks ago

:warning: This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@kyle-seely: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.