[DataGrid] Ensure referential integrity of focused cell

[Kerry350]

Summary

As part of elastic/kibana#79288 I discovered an issue with the focusedCell / setFocusedCell logic.

It's possible (especially with an overscanRowCount greater than 1) to end up in a loop where the user can't scroll a focused cell out of view, instead it will constantly scroll back in to view, and thus block scrolling up or down properly.

The reason for this is the calls to setFocusedCell pass through a new array instance each time, and this means the hooks that use focusedCell as a dependency (which would rely on referential integrity of the array, or alternatively using the primitive values of the array instead) receive a new reference each time.

This causes the following loop (for a focused cell):

1. Cell mounts
2. Checks if focused
3. If yes, will call .focus
4. This triggers onFocus, which will always pass a new array instance to setFocusedCell.
5. The new reference will always trigger the useScroll useEffect as a dependency change
6. This will call scrollCellIntoView

This is more exaggerated in the case of a higher overscan count as cells can be simultaneously focused, mounted, and out of view for longer. Although it is still visible with the default of 1 too.

This gif tries to show the behaviour (notice the snapping back to the focused cell):

This is the behaviour with the fix applied:

There are a few ways this could be approached, I didn't want to change the tuple nature of focusedCell and switch to passing two primitives around instead as this logic was spread in quite a few places. This change seemed the least intrusive, it ensures the public setFocusedCell only propagates to the private _setFocusedCell state update if the values have actually changed (and should therefore notify locations such as scrolling.ts).

Please let me know if this breaks anything or my assumptions are wrong (as I'm not an EUI developer), as far as I can see this doesn't alter anything else.

Checklist

• Checked in both light and dark modes
• Checked in mobile
• Checked in Chrome, Safari, Edge, and Firefox
• Props have proper autodocs and playground toggles
• Added documentation
• Checked Code Sandbox works for any docs examples
• Added or updated jest and cypress tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• Updated the Figma library counterpart
• A changelog entry exists and is marked appropriately

[cee-chen]

AWESOME debugging and breakdown on this - it was super easy to follow your logic and the issue! I agree your fix works, just wanted to throw this alternative option out there to see what you think - it's a few less characters so may be easier to read/grok.

In scrolling.tsx, in the useEffect dependencies:

eui/src/components/datagrid/utils/scrolling.tsx

Lines 47 to 54 in c1f5de4

	useEffect(() => {
	if (focusedCell) {
	scrollCellIntoView({
	rowIndex: focusedCell[1],
	colIndex: focusedCell[0],
	});
	}
	}, [focusedCell, scrollCellIntoView]);

- }, [focusedCell, scrollCellIntoView]); 
+ }, [focusedCell?.[0], focusedCell?.[1], scrollCellIntoView]);

Would that accomplish the same fix? Or am I oversimplifying? 🤔

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6007/

[Kerry350]

@constancecchen

just wanted to throw this alternative option out there to see what you think

Unfortunately I don't think that will work. It would invalidate the dependencies being exhaustive (focusedCell would still be expected) and useEffect doesn't allow complex expressions.

My thinking was by making it part of setFocusedCell the reference could always be stable, even for future hooks etc (although worrying about things that don't exist probably isn't needed :D).

[cee-chen]

Gotcha, no worries! After thinking on a bit more as well I totally agree that your approach is the most robust solution. Seriously awesome!! :)

I have a few small comments and a suggestions for a unit test, I'll make those here shortly. I think we should add a changelog for this fix as well - you can auto-generate the changelog file by running yarn yo-changelog 6007, and typing Y for the bugfix prompt.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6007/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6007/

[Kerry350]

@constancecchen Thanks for the review, and test addition. I think this is ready now.

As a side note I did hit a bit of an issue with the changelog generation. I got this error running yarn yo-changelog 6007:

$ yo ./generator-eui/chanagelog/index.js 6007
/Users/kerry/Documents/code/elastic/eui/node_modules/macos-release/index.js:26
	const [name, version] = nameMap.get(release);
	                        ^

TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
    at macosRelease (/Users/kerry/Documents/code/elastic/eui/node_modules/macos-release/index.js:26:26)
    at osName (/Users/kerry/Documents/code/elastic/eui/node_modules/os-name/index.js:21:18)
    at new Insight (/Users/kerry/Documents/code/elastic/eui/node_modules/insight/lib/index.js:37:13)
    at Object.<anonymous> (/Users/kerry/Documents/code/elastic/eui/node_modules/yo/lib/cli.js:54:17)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I ended up just manually adding [21, ['Monterey', '12']], to the nameMap in node_modules/macos-release/index.js which worked insofar as letting me generate things. Just wanted to mention it in case there's a dependency that needs updating somewhere.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6007/

[cee-chen]

Whoa, thanks for finding that! Almost certainly an issue of the majority of us being on older Macs/OSes, which we should run into at some point when our machines get upgraded, haha 🙈

Thanks for the comment expansion as well, this looks really great!

[cee-chen]

🎉 Thank you for this fantastic contribution to EUI and EuiDataGrid! Feel free to merge this PR in once CI passes!

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_6007/