nuclearpasta / react-native-drax

A drag-and-drop system for React Native
MIT License
554 stars 69 forks source link

Clipping leads to wrong sizes #150

Closed FrancoisDupayrat closed 2 years ago

FrancoisDupayrat commented 2 years ago

Hello,

When scrolling a DraxList a bit (so that the top-most elements row is partly off-screen for example), the top-most elements row get their position clipped (in clipMeasurements) but their size isn't changed. This leads to an overlap, which produce an erroneous behavior when using onMonitorDrag and dragging something on elements in this list: when hovering the 2nd row (just below the top-most row), elements in the top-most row receive onMonitorDrag events despite not being hovered, as their position is not right.

A quick fix to this behavior is to modify line 185 of useDraxRegistry from const absoluteMeasurements = getAbsoluteMeasurementsForViewFromRegistry(registry, target, true); to const absoluteMeasurements = getAbsoluteMeasurementsForViewFromRegistry(registry, target, false); (in effect removing the clipping). I've verified in my project (private, I can try to make a public reproduction project on request) that it works and could not see any noticeable side-effect.

This leads to the main question: what's the point of this clipping? Can it safely be removed here? Or should the clipping math be fixed instead?

A proper fix would be to fix clipMeasurements itself. The fix is simple: in the 4 if clauses, reverse the order of the operation so that the size is properly modified. For example:

    if (x0 < cx0) {
        x0 = cx0;
        width -= cx0 - x0;
    }

Becomes:

    if (x0 < cx0) {
        width -= cx0 - x0;
        x0 = cx0;
    }

I've verified as well as it works in my private project.

Would you like a PR for either fix? Feel free to directly make either modification with no attribution to speed up the fix if you prefer.

lafiosca commented 2 years ago

Thanks for looking into this. The sad fact is it's been too long since I've worked on the logic to even remember what you're referring to :) Off the top of my head, without looking at the code at all, I would guess the clipping is necessary to prevent drag logic from triggering outside of the bounds of a parent? If so, I would prefer the fixed math version. The existing logic you shared there is clearly a mistake, as cx0 - x0 will always be 0, so I assume I intended it to look like your suggested change. If you can submit a PR, I can merge it, update the CHANGELOG, and publish a point release. I'd appreciate it greatly if you can verify the behavior after release as well.

FrancoisDupayrat commented 2 years ago

This was my guess as well, I didn't get to the bottom of why clipping was implemented.

I've opened a PR as requested. As you said, it's clearly a mistake that went unnoticed.

Sure, I'll report back after you publish a point release

lafiosca commented 2 years ago

@FrancoisDupayrat 0.10.3 has been released. Looks like the PR auto-closed this issue, but if you see any problems we can reopen it! Thank you again!!

FrancoisDupayrat commented 2 years ago

I just tested 0.10.3, and it correctly fixes the issue. Thanks a lot for the quick merge and release, it's very appreciated!