-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[data grid] Fix focused column header scroll jump #19323
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.
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
Conversation
Deploy preview: https://deploy-preview-19323--material-ui-x.netlify.app/ Bundle size report
|
elementToFocus?.focus(); | ||
if (apiRef.current.columnHeadersContainerRef?.current) { | ||
apiRef.current.columnHeadersContainerRef.current.scrollLeft = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the context, this was added here: #4280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever it fixed should be irrelevant since position: sticky
? columnHeadersContainer
is not scrollable anyways (anymore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be irrelevant.
React.useLayoutEffect(() => { | ||
const columnMenuState = apiRef.current.state.columnMenu; | ||
if (hasFocus && !columnMenuState.open) { | ||
const focusableElement = | ||
headerCellRef.current!.querySelector<HTMLElement>('[tabindex="0"]'); | ||
const elementToFocus = focusableElement || headerCellRef.current; | ||
elementToFocus?.focus(); | ||
if (apiRef.current.columnHeadersContainerRef?.current) { | ||
apiRef.current.columnHeadersContainerRef.current.scrollLeft = 0; | ||
} | ||
} | ||
}, [apiRef, hasFocus]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used/useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed like a duplicated logic that was already present in the parent components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I've tested and the functionality seems to work even without this.
Need to add a fallback for Chrome on Android it seems, similar to GridCell: https://caniuse.com/mdn-api_htmlelement_focus_options_preventscroll_parameter |
Should be ready to merge now. |
Fixes #19320.
Albeit the focused element jumping in and out of render context part probably is not ideal (also doesn't support keyboard), it fixes the biggest issue for now.