NoriginMedia / react-spatial-navigation

DEPRECATED. HOC-based Spatial Navigation. NEW Hooks version is available here: https://github.com/NoriginMedia/norigin-spatial-navigation
MIT License
226 stars 64 forks source link

SIblings array is empty #80

Closed gmariani closed 2 years ago

gmariani commented 3 years ago

Describe the bug I have a vertical list of rectangles (dvd covers), 4 across and 4 down. The screen can display about 1.25 rows at a time. The first three rows are focusable, the last row is not. Once the focus reaches the bottom row, the siblings array is empty despite visualDebug recognizing the items.

To Reproduce Steps to reproduce the behavior:

  1. Create a container of items that go well off screen
  2. Try to focus on the last items (the most off-screen)
  3. Unable to select last items
  4. See error

Expected behavior To be able to focus on all focusable elements.

Screenshots Screenshot 2021-01-15 09 07 40

Additional context None

gmariani commented 3 years ago

I figured out the issue.The root of the issue is how getRect() calculates in measureLayout.js. It uses offsetWidth, offsetLeft, etc. But all the offset properties round to the nearest integer instead of remaining a floating point. This comes into play when you have items in a row with no gap in between.

In my case, I have a rectangle that was 677.6 pixels tall, rounded to 678 by offsetHeight. It would calculate the cutoff coordinate to be 614 pixels. The next row (in the down direction) would have another rectangle with an offsetTop of 613 pixels. So when it came to filter out nearby siblings, it would ignore the row right below it because of a rounding error. One option is to switch to getBoundingClientRect() but that can incur some rendering penalties. The easiest solution I found was to just add a 2-5px gap between items to avoid the rounding error, thus recognizing the siblings properly.

So again, my solution is specific to my problem, but anyone else with items flush against each other, they could easily run into problems where it skips siblings due to rounding errors. I would suggest possibly finding an efficient way to call getBoundingClientRect() or build in a 1px tolerance when calculating siblings.

micheleb commented 3 years ago

Thanks @gmariani for the hint! I had the exact same issue, and simply ended up using getBoundingClientRect(). Since these calculations are performed on user input, I don't think calling it can really make a noticeable difference in the general case, and it certainly didn't in mine (while being quite a nasty little issue to debug!).

If anyone wants to try it out, the patch to apply (via patch-package) is simply this:

patches/react-spatial-navigation+2.12.6.patch

```diff diff --git a/node_modules/@noriginmedia/react-spatial-navigation/dist/measureLayout.js b/node_modules/@noriginmedia/react-spatial-navigation/dist/measureLayout.js index 8e9ddb8..74b8a4d 100644 --- a/node_modules/@noriginmedia/react-spatial-navigation/dist/measureLayout.js +++ b/node_modules/@noriginmedia/react-spatial-navigation/dist/measureLayout.js @@ -6,19 +6,11 @@ Object.defineProperty(exports, "__esModule", { var ELEMENT_NODE = 1; var getRect = function getRect(node) { - var offsetParent = node.offsetParent; - - var height = node.offsetHeight; - var width = node.offsetWidth; - var left = node.offsetLeft; - var top = node.offsetTop; - - while (offsetParent && offsetParent.nodeType === ELEMENT_NODE) { - left += offsetParent.offsetLeft - offsetParent.scrollLeft; - top += offsetParent.offsetTop - offsetParent.scrollTop; - var _offsetParent = offsetParent; - offsetParent = _offsetParent.offsetParent; - } + var r = node.getBoundingClientRect(); + var height = r.height; + var width = r.width; + var left = r.left; + var top = r.top; return { height: height, ```

After applying it, do check whether getBoundingClientRect() is causing slow reflows!

gmariani commented 3 years ago

What I meant by performance issues is that getBoundingClientRect() will always trigger a reflow for the browser. So if you constantly run it (not saying you are, just for example), the browser will constantly be reflowing the layout of the page (layout thrashing). Which in turn could cause some performance issues. Here is some article i found kind of explaining it https://nolanlawson.com/2018/09/25/accurately-measuring-layout-on-the-web/ So, not saying your solution won't work, but just keep in mind how it's being used.

micheleb commented 3 years ago

Reflows don't always have a cost though, see the appendix here, that's why I added that last disclaimer :) You might get a performance hit, but you might as well not, depending on what your app is doing.

So if one is affected by this rounding error issue, they might as well give the getBoundingClientRect() fix a try (which only requires applying a patch now) and see if it slows things down. If it does and in a meaningful way, then they'll have to wait for a fix (or find an alternative solution), but if it doesn't... Godspeed to them 🚀

asgvard commented 2 years ago

Hello. We have migrated this library to TS and Hooks: https://github.com/NoriginMedia/Norigin-Spatial-Navigation We are also using getBoundingClientRect for measurements now, which might help with this issue you were having.

Closing this due to inactivity and assuming this library is deprecated.