Users/vnbaaij/improve datagrid script #4500
Open
+42
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rt and remove the resizeController for the grid instance. 2. Dynamic AbortSignal: Updated enableColumnResizing to: • Detect if a grid already has a resizeController and abort it before creating a new one (e.g., when columns are re-evaluated). 1. Create a localController if no external signal is provided from Blazor. 2. Use an effectiveSignal for all resizing event listeners. 3. Registry Persistence: Modified the grid registration logic to store the resizeController so it can be managed during re-initialization or disposal. These changes ensure that whenever column resizing is re-enabled (which happens in Blazor when columns change), the old JS listeners are properly cleaned up via the AbortController.
- Port v5 script improvement
|
✅ All tests passed successfully Details on your Workflow / Core Tests page. |
Summary - Unit Tests Code CoverageSummary
CoverageMicrosoft.FluentUI.AspNetCore.Components - 60.9%
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the DataGrid JavaScript interop by enhancing abort controller management for proper cleanup of event listeners, improving keyboard event handling for closing popups with the Escape key, and fixing registry persistence to handle re-initialization scenarios.
Changes:
- Enhanced AbortController management to clean up resize event listeners when columns change
- Added Escape key handling at the body level to close column options and resize popups
- Improved grid registry to update existing entries instead of creating duplicates
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+60
to
64
| if (columnResizeElement) { | ||
| if (event.key === "ArrowRight" || event.key === "ArrowLeft" || event.key === "ArrowDown" || event.key === "ArrowUp") { | ||
| event.stopPropagation(); | ||
| } | ||
| if (event.key === "Escape") { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | ||
| gridElement.focus(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The removal of the contains(event.target) check changes the behavior significantly. Previously, arrow key propagation was only stopped when the event target was inside the column resize element. Now it stops propagation whenever the column resize element merely exists in the DOM, regardless of whether the event originated from within it. This will incorrectly prevent arrow key navigation when column resize is open but focus is elsewhere in the grid.
Comment on lines
+32
to
+39
| if (columnOptionsElement) { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | ||
| gridElement.focus(); | ||
| } | ||
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | ||
| if (columnResizeElement) { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | ||
| gridElement.focus(); |
There was a problem hiding this comment.
When multiple grids exist on the page and Escape is pressed, all grids' bodyKeyDownHandler functions will execute. Each grid that has column options or resize elements open will dispatch close events and call gridElement.focus(). This can cause a focus race condition where the last grid's handler steals focus, even if the user intended to close a popup on a different grid. Consider checking if the event originated from within the specific grid's DOM or if the popup element is currently focused before calling focus.
Suggested change
| if (columnOptionsElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | |
| gridElement.focus(); | |
| } | |
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | |
| if (columnResizeElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | |
| gridElement.focus(); | |
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | |
| // Determine whether this Escape event is relevant to this grid or its popups | |
| const eventPath = typeof event.composedPath === "function" ? event.composedPath() : []; | |
| const eventOriginatesFromThisGrid = eventPath && eventPath.indexOf(gridElement) >= 0; | |
| const activeElement = document.activeElement; | |
| if (columnOptionsElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | |
| const optionsHasFocus = columnOptionsElement.contains(activeElement); | |
| if (eventOriginatesFromThisGrid || optionsHasFocus) { | |
| gridElement.focus(); | |
| } | |
| } | |
| if (columnResizeElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | |
| const resizeHasFocus = columnResizeElement.contains(activeElement); | |
| if (eventOriginatesFromThisGrid || resizeHasFocus) { | |
| gridElement.focus(); | |
| } |
| grid.resizeController.abort(); | ||
| } | ||
|
|
||
| const localController = new AbortController(); |
There was a problem hiding this comment.
When an external signal is provided (e.g., from the init function), a localController is created on line 190 but never used because effectiveSignal will be the external signal. Additionally, line 250 stores undefined for resizeController when an external signal is used. This means when enableColumnResizing is called again (e.g., when columns change), lines 186-188 attempt to abort a controller that is always undefined in this scenario. While the old resize handle divs are removed (line 223) which cleans up their listeners, the localController serves no purpose and creates unnecessary overhead. Consider only creating localController when no external signal is provided.
Suggested change
| const localController = new AbortController(); | |
| const localController = signal ? null : new AbortController(); |
Comment on lines
+31
to
+38
| const columnOptionsElement = gridElement?.querySelector('.col-options'); | ||
| if (columnOptionsElement) { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | ||
| gridElement.focus(); | ||
| } | ||
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | ||
| if (columnResizeElement) { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); |
There was a problem hiding this comment.
The handler calls gridElement.focus() on both lines 34 and 39 when both column options and column resize elements exist simultaneously. This results in redundant focus calls. Consider checking if either element exists first, dispatching the appropriate close events, and then calling focus once at the end.
Suggested change
| const columnOptionsElement = gridElement?.querySelector('.col-options'); | |
| if (columnOptionsElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | |
| gridElement.focus(); | |
| } | |
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | |
| if (columnResizeElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | |
| let shouldFocus = false; | |
| const columnOptionsElement = gridElement?.querySelector('.col-options'); | |
| if (columnOptionsElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | |
| shouldFocus = true; | |
| } | |
| const columnResizeElement = gridElement?.querySelector('.col-resize'); | |
| if (columnResizeElement) { | |
| gridElement.dispatchEvent(new CustomEvent('closecolumnresize', { bubbles: true })); | |
| shouldFocus = true; | |
| } | |
| if (shouldFocus) { |
Comment on lines
+52
to
56
| if (columnOptionsElement) { | ||
| if (event.key === "ArrowRight" || event.key === "ArrowLeft" || event.key === "ArrowDown" || event.key === "ArrowUp") { | ||
| event.stopPropagation(); | ||
| } | ||
| if (event.key === "Escape") { | ||
| gridElement.dispatchEvent(new CustomEvent('closecolumnoptions', { bubbles: true })); | ||
| gridElement.focus(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The removal of the contains(event.target) check changes the behavior significantly. Previously, arrow key propagation was only stopped when the event target was inside the column options element. Now it stops propagation whenever the column options element merely exists in the DOM, regardless of whether the event originated from within it. This will incorrectly prevent arrow key navigation when column options are open but focus is elsewhere in the grid.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
• Detect if a grid already has a resizeController and abort it before creating a new one (e.g., when columns are re-evaluated).
These changes ensure that whenever column resizing is re-enabled (which happens in Blazor when columns change), the old JS listeners are properly cleaned up via the AbortController. It also now close any popups correctly by using ESC key