Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix scrollbar thumb crashing in React@next (v19) #718

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Fix scrollbar thumb crashing in React@next (v19) #718

merged 1 commit into from
Apr 10, 2024

Conversation

pradeepnschrodinger
Copy link
Collaborator

@pradeepnschrodinger pradeepnschrodinger commented Mar 30, 2024 •
edited
Loading

Description

Trying to interact with FDT's scrollbar on latest React (react@next or react@19 at the time of writing) crashes the whole app with the following error:

fixed-data-table-2.js:5137 Uncaught TypeError: (0 , ReactDOM.default).findDOMNode is not a function
    at Scrollbar.eval [as _blur] (fixed-data-table-2.js:5137:38)
    at Scrollbar.eval [as _setNextState] (fixed-data-table-2.js:5168:58)
    at Scrollbar.componentDidUpdate (fixed-data-table-2.js:5186:14)

This confirms that ReactDOM.findDOMNode which was already marked as deprecated is now removed in a future version of React.

Furthermore, once a scrollbar is active, it never goes into an inactive state even after stopping scroll or moving the cursor away from the scrollbar. Animations are also semi-broken...

Context

We've been using ReactDOM.findDOMNode in our scrollbar component to get a reference to the scrollbar DOM so as to blur it out.
Digging through the code, this looks like it was simply done to figure out if the scrollbar is in an active state of use.

Technically we don't need to any of this, and we can instead rely on DOMMouseMoveTracker to figure out if the user is dragging and using the scrollbar.

Fixes

I'm fixing all of this by cleaning up a bit of the Scrollbar component such that:

  • Any code associated with blurring the scrollbar is removed
    • ReactDOM.findDOMNode isn't used anymore
    • We no longer have to maintain a reference to the last used scrollbar
  • Fixes scrollbar UI so that scrollbar thumb correctly indicates that it is active
    • I simplified the CSS and removed the CSS var scrollbar-size-large

Screenshot

Inactive scrollbars
Screenshot 2024-03-30 at 12 17 11 PM

Active horizontal scrollbar
image

Active vertical scrollbar
image

How Has This Been Tested?

Tested on local examples having multiple scrollbars.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@pradeepnschrodinger pradeepnschrodinger added bug regression issue that was "fixed" but got reopened later on labels Mar 30, 2024
pradeepnschrodinger
pradeepnschrodinger commented Mar 30, 2024
View reviewed changes
src/css/layout/ScrollbarLayout.css
@@ -27,23 +27,18 @@
.ScrollbarLayout/mainHorizontal {
height: var(--scrollbar-size);
left: 0;
transition-property: background-color height;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to the thumb element (see :after) which is the actual element that needs the animation

src/css/layout/ScrollbarLayout.css
/* Touching the scroll-track directly makes the scroll-track bolder */
.ScrollbarLayout/mainHorizontal.public/Scrollbar/mainActive,
.ScrollbarLayout/mainHorizontal:hover {
height: var(--scrollbar-size-large);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed scrollbar-size-large since it was changing the height of the scrollbar wrapper element thus in turn very slightly modifying the layout of the table.

src/css/layout/ScrollbarLayout.css
Comment on lines +55 to +57
transition-duration: 250ms;
transition-timing-function: ease;
transition-property: background-color, height, width;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should this be turned into a single line?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have any advantage of that?
code wise i think its fine

src/css/layout/ScrollbarLayout.css
@@ -67,10 +64,8 @@
}

.ScrollbarLayout/faceHorizontal:after {
bottom: var(--scrollbar-face-margin);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, top, bottom, left, right were set to align the thumb in the center of the scrollbar face element.
This is a bit hard to read and manage, and we can instead simplify this with flex layout.

src/plugins/Scrollbar.js
Comment on lines -185 to -186
onFocus={this._onFocus}
onBlur={this._onBlur}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need to manage blurring or focusing the scrollbar.

src/plugins/Scrollbar.js
@@ -244,9 +240,6 @@ class Scrollbar extends React.PureComponent {
this._mouseMoveTracker.releaseMouseMoves();
this._mouseMoveTracker = null;
}
if (_lastScrolledScrollbar === this) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need to track the last scrolled scrollbar.

@karry08
Copy link
Collaborator

karry08 commented Apr 9, 2024

LGTM

@pradeepnschrodinger pradeepnschrodinger merged commit a5bee49 into master Apr 10, 2024
10 checks passed
@pradeepnschrodinger pradeepnschrodinger deleted the fix-scrollbar-thumb branch April 10, 2024 23:53
@pradeepnschrodinger
Copy link
Collaborator Author

Released with v2.0.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhisheksingh2410 abhisheksingh2410 Awaiting requested review from abhisheksingh2410

@karry08 karry08 Awaiting requested review from karry08

@AmanGupta2708 AmanGupta2708 Awaiting requested review from AmanGupta2708

Assignees

@karry08 karry08

Labels
bug regression issue that was "fixed" but got reopened later on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pradeepnschrodinger @karry08