fix: Propogation of Scroll Events when Scroll Position is at a Boundary by AkshatJawne · Pull Request #2166 · deephaven/web-client-ui

AkshatJawne · 2024-07-22T17:47:17Z

codecov · 2024-07-22T17:53:19Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 46.66%. Comparing base (7875d03) to head (59b2466).
Report is 7 commits behind head on main.

Files	Patch %	Lines
packages/grid/src/Grid.tsx	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2166      +/-   ##
==========================================
- Coverage   46.68%   46.66%   -0.02%     
==========================================
  Files         691      692       +1     
  Lines       38588    38625      +37     
  Branches     9806     9814       +8     
==========================================
+ Hits        18014    18025      +11     
- Misses      20563    20589      +26     
  Partials       11       11

Flag	Coverage Δ
unit	`46.66% <0.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmingles · 2024-07-23T14:38:43Z

-    event.stopPropagation();
-    event.preventDefault();
+    // Check if we're at the top or bottom of the grid
+    if (top >= 0 && topOffset !== 0) {


Scrolling seems to be broken in IrisGrid sample in styleguide. I think something isn't working as expected here. Also, a little confused how this is checking top or bottom? Is this a stale comment, or is there something missing in the code here?

Taking a look rn

bmingles · 2024-07-23T14:38:47Z


+    // Check if at the top and attempting to scroll up
+    // Or at the bottom and attempting to scroll down
+    if ((top === 0 && deltaY < 0) || (top >= rowCount - 1 && deltaY > 0)) {


Something doesn't seem right here that you are introducing a return that wasn't there before.

Well we return because we are no longer looking to scroll, and thus handle movement of the wheel, if we are attempting to scroll down if we are at the bottom, or scroll up if we are at the top.

But I am looking into to modify. the logic right now, since as per your observation, the IrisGrid is behaving quite differently then the Grid in terms of the scroll.

bmingles

IrisGrid example in styleguide is broken

AkshatJawne · 2024-07-23T20:07:13Z

Still working on implementing fix, the initial approach of returning early or having it stop propogation at the bottom using the topOffset is not working

AkshatJawne · 2024-07-25T14:06:59Z

Ready for re-review

bmingles · 2024-07-25T17:26:28Z


+    // Check if at the top and attempting to scroll up
+    // Or at the bottom and attempting to scroll down
+    if ((top === 0 && deltaY < 0) || (top === lastTop && deltaY > 0)) {


This seems to work, but I'm not the most familiar with how Grid was implemented. Would probably be good to check with @mofojed to make sure there aren't any edge cases we are missing here.

ah, I think this can break horizontal scrolling when at top / bottom boundary since it's hard to scroll horizontally exactly without at least some deltaY if you are scrolling via a trackpad

bmingles · 2024-07-25T18:25:19Z

+    // Or at the bottom and attempting to scroll down
+    // Or at the left and attempting to scroll left
+    // Or at the right and attempting to scroll right
+    if (


I think you need something that only returns if neither vertical or horizontal movement will result from the delta.

e.g. I haven't tested but something like:

const willScrollX = (deltaX < 0 && left > 0) || (deltaX > 0 && left < lastLeft) const willScrollY = (deltaY < 0 && top > 0) || (deltaY > 0 && top < lastTop) if (!willScrollX && !willScrollY) { return }

It is behaving the same as my commit, but makes more sense semantically, pushed change.

bmingles

Looks good to me. I tested scenarios where there were page level horizontal + vertical scrollbars. Grid maintains control while you can still control, and then passes control to page at boundaries. Also verified it no longer stutters.

bmingles · 2024-07-25T19:27:22Z

      metrics.rowHeight
    );

+    // Check if we're at the edge of the grid and attempting to scroll


@mofojed this works for me with all cases I tested. I'm not very familiar with how lastTop and lastLeft are actually implemented or if there are nuances here we are missing. Any concerns on your part?

There is one nuance that's missing. To give a little bit of context:

The way we handle scrolling is by keeping track of the cell index that is in the top left, and then keeping track of the pixel offset within that cell. We do that so we can support very large grids and grids with variable width/height cells (since the width of a column may change after we've fetched data for it, we resize to match the data).

left/top are the index of the cell in the top left

leftOffset/topOffset is the pixel offset within that cell that's in the top left

Current scroll position is the position of the top/left cell + the offset

Here we're just checking left, but you're not checking leftOffset or topOffset. You should check if (left > 0 || leftOffset > 0) && deltaX < 0 to check if you can scroll left (same with the top).

Actually just to be safe - I'd just track the initial values for all those variables left, top, leftOffset, and topOffset, and then just at the end of this method before this.setViewState check if they haven't changed, and if they haven't, don't bother setting them or eating the event. That seems like the safest.

@mofojed I thought I saw some scenarios where scrolling really fast could result in no changes to those values for a particular pass and allowing the event to propagate when the user is actually still scrolling. Possible I wasn't incorporating all 4 checks, but just calling out as something we'll want to test.

I think @mofojed suggestion here was to remove this section and just have the check at the end that you added below. I'll let him confirm, but I tested again using only the code at the end, and that seems to work as expected. I don't think we need this code at the beginning anymore.

Ahh I might have misinterpreted, I thought just to be safe meant that the check at the bottom would be a failsafe, not the only check. Will test by removing initial code and repush accordingly.

mofojed · 2024-07-29T15:46:05Z

      metrics.rowHeight
    );

+    // Check if we're at the edge of the grid and attempting to scroll


There is one nuance that's missing. To give a little bit of context:

The way we handle scrolling is by keeping track of the cell index that is in the top left, and then keeping track of the pixel offset within that cell. We do that so we can support very large grids and grids with variable width/height cells (since the width of a column may change after we've fetched data for it, we resize to match the data).

left/top are the index of the cell in the top left

leftOffset/topOffset is the pixel offset within that cell that's in the top left

Current scroll position is the position of the top/left cell + the offset

Here we're just checking left, but you're not checking leftOffset or topOffset. You should check if (left > 0 || leftOffset > 0) && deltaX < 0 to check if you can scroll left (same with the top).

Actually just to be safe - I'd just track the initial values for all those variables left, top, leftOffset, and topOffset, and then just at the end of this method before this.setViewState check if they haven't changed, and if they haven't, don't bother setting them or eating the event. That seems like the safest.

bmingles

Changed look good. Tested in styleguide. Things seem to work as expected.

mofojed

Just some cleanup but looks good

fix: Propogation of Scroll Events when Scroll Position is at a Boundary

3cfa737

AkshatJawne requested review from bmingles and mofojed July 22, 2024 17:47

AkshatJawne self-assigned this Jul 22, 2024

bmingles reviewed Jul 23, 2024

View reviewed changes

bmingles requested changes Jul 23, 2024

View reviewed changes

address changes

a82889e

AkshatJawne requested a review from bmingles July 25, 2024 14:07

AkshatJawne added 3 commits July 25, 2024 11:00

fix bug

d6303cf

change solution

da7015e

revert to old solution

ea546bb

bmingles reviewed Jul 25, 2024

View reviewed changes

fix horizontal scroll bug

9e0716d

AkshatJawne requested a review from bmingles July 25, 2024 17:47

bmingles reviewed Jul 25, 2024

View reviewed changes

change conditional

660febe

AkshatJawne requested a review from bmingles July 25, 2024 18:50

bmingles previously approved these changes Jul 25, 2024

View reviewed changes

bmingles reviewed Jul 25, 2024

View reviewed changes

mofojed requested changes Jul 29, 2024

View reviewed changes

make change to logic based off review

87e0eb9

AkshatJawne dismissed bmingles’s stale review via 87e0eb9 July 30, 2024 01:02

AkshatJawne requested review from bmingles and mofojed July 30, 2024 01:02

modify check

f442821

bmingles previously approved these changes Jul 30, 2024

View reviewed changes

mofojed requested changes Jul 30, 2024

View reviewed changes

Comment thread packages/grid/src/Grid.tsx Outdated

Comment thread packages/grid/src/Grid.tsx Outdated

remove unneccsary initialMetrics

59b2466

AkshatJawne dismissed bmingles’s stale review via 59b2466 July 30, 2024 17:09

AkshatJawne requested review from bmingles and mofojed July 30, 2024 17:09

mofojed approved these changes Jul 30, 2024

View reviewed changes

AkshatJawne merged commit cb72d29 into deephaven:main Jul 30, 2024

github-actions Bot locked and limited conversation to collaborators Jul 30, 2024

Conversation

AkshatJawne commented Jul 22, 2024

Uh oh!

codecov Bot commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles left a comment

Choose a reason for hiding this comment

Uh oh!

AkshatJawne commented Jul 23, 2024

Uh oh!

AkshatJawne commented Jul 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmingles left a comment

Choose a reason for hiding this comment

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 22, 2024 •

edited

Loading

bmingles Jul 25, 2024 •

edited

Loading

bmingles Jul 25, 2024 •

edited

Loading

bmingles Jul 29, 2024 •

edited

Loading