fix vdom scroll #4796
fix vdom scroll #4796
Conversation
|
@azmy60 this still draft, or you want me to review it? |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #4730 related to scroll position maintenance in Tabulator's virtual DOM implementation. The changes focus on fixing scroll behavior when programmatically adding large batches of rows, though manual scrolling issues persist.
- Enhances virtual DOM scroll position calculations for better bottom padding handling
- Implements scroll position preservation logic for large batch row additions (100+ rows)
- Adds comprehensive test coverage for various scrolling scenarios including the specific issue #4730
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/e2e/index.html | Adds height constraint to enable scrolling in test environment |
| test/e2e/basic.spec.js | Comprehensive test suite for scrolling operations and issue #4730 scenarios |
| src/js/core/rendering/renderers/VirtualDomVertical.js | Fixes virtual DOM bottom padding calculation and scroll height management |
| src/js/core/RowManager.js | Implements scroll position preservation for large batch row additions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Store initial state for large batch additions (issue #4730) | ||
| var initialScrollInfo; | ||
| if(data.length >= 100 && this.renderer && this.renderer.elementVertical){ |
There was a problem hiding this comment.
The magic number 100 should be extracted to a named constant or configuration option to make the threshold for scroll position preservation configurable and more maintainable.
| this.regenerateRowPositions(); | ||
|
|
||
| // Preserve relative scroll position for large batch additions (issue #4730) | ||
| if(initialScrollInfo && !pos && data.length >= 100){ |
There was a problem hiding this comment.
The same magic number 100 is repeated here. This should use the same constant as defined for the condition above to avoid duplication and ensure consistency.
| if(initialMaxScroll > 0 && newMaxScroll > 0){ | ||
| // Don't use relative positioning if we were at the exact bottom | ||
| // This prevents the scrollbar from appearing to be at the bottom when more content exists | ||
| if(Math.abs(initialScrollInfo.scrollTop - initialMaxScroll) < 2){ |
There was a problem hiding this comment.
The magic number 2 for determining if the user was at the bottom should be extracted to a named constant with a descriptive name like BOTTOM_SCROLL_TOLERANCE for better readability and maintainability.
| if(Math.abs(initialScrollInfo.scrollTop - initialMaxScroll) < 2){ | ||
| // We were at the bottom, but don't stick exactly to new bottom | ||
| // Leave some space to indicate there's more content | ||
| this.renderer.elementVertical.scrollTop = Math.max(0, newMaxScroll - 50); |
There was a problem hiding this comment.
The magic number 50 representing the offset from bottom should be extracted to a named constant like BOTTOM_OFFSET_PIXELS to make the intent clear and allow for easier adjustment.
|
@rathboma Sure. I've hit the wall with this one. The tests make it seems it fixes the issue, but if you try it yourself manually, it doesn't. |
In attempt to fix #4730. However, this fixes the issue when scrolling in program, not manually like dragging the scrollbar with the mouse.