Improve selection behavior of the Logs tab#5756
Conversation
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
… the tabs Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
mythical-fred
left a comment
mythical-fred
left a comment
There was a problem hiding this comment.
The userSelect.ts refactor is a real improvement — data-rowindex anchors + Range.toString() for offsets beats the old recursive sibling-walking by a mile. The visited SvelteSet in TabsPanel and the IntersectionObserver in useReverseScrollContainer are coherent, necessary follow-ups to enabling keepAlive for Logs.
Ongoing web-console gap: the selection logic in particular (drag-extend, shift-click, DOM recycling edge cases) is exactly what Vitest + @testing-library/svelte tests would catch before they reach production.
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
I have actually stopped reviewing midway. First I would like to make sure that this is the right way to solve this problem. Maybe having a "pause" button for the logs is a better and much simpler solution.
| } | ||
|
|
||
| export const injectOnCopyAll = (node: HTMLElement, props: { value: string }) => {} | ||
| type RowLocation = { |
There was a problem hiding this comment.
This type seems to be misnamed, unless you have a matrix with ROW values in every cell.
| } else if (node.nodeType === Node.ELEMENT_NODE && node instanceof HTMLElement) { | ||
| textLen += node.innerText.length | ||
| /** Walk up the DOM to find the nearest ancestor with a `data-rowindex` attribute */ | ||
| const findRowAncestor = (node: Node): HTMLElement | null => { |
There was a problem hiding this comment.
This function name is very generic, it does not really reflect what it is doing.
| row: number | ||
| col: number | ||
| /** Get the character offset from the start of a row element to a specific DOM position */ | ||
| const getCharOffset = (rowElement: HTMLElement, node: Node, offset: number): number => { |
There was a problem hiding this comment.
same for this function name.
| index: number[] // [firstChild, ...restChildren] firstChild - index in the list of .children, restChildren - indices in the list of .childNodes | ||
| offsetLocal: number // Location in the leaf Node.TEXT_NODE | ||
| offsetAbsolute: number // Location in the root.item(i).innerText | ||
| /** Convert a DOM selection point to a logical {row, col} position */ |
There was a problem hiding this comment.
position within what?
Function named "toLogical", but result is RowLocation. Maybe it should be called toRowLocation?
| ): Location | null => { | ||
| if (!node.parentNode) { | ||
| return null | ||
| /** Iterate over row elements in the virtualizer root, calling fn for each row index and its wrapper */ |
There was a problem hiding this comment.
I don't know what a virtualizer is, maybe I missed it.
| // Track when the mouse leaves the container during a drag-selection, so the MO | ||
| // callback knows to extend the focus toward newly visible rows. | ||
| let dragOutside: 'above' | 'below' | null = null | ||
| // Set when handleSelectionChange detects that Firefox placed the anchor on the |
There was a problem hiding this comment.
Yes, unfortunately the behavior between differs between it and Chrome-based browsers. I am actually yet to test Safari.
| // This signals the MO callback that the browser can't maintain the selection | ||
| // natively, so we must reapply and extend focus ourselves even during a drag | ||
| // inside the container. Cleared on pointerup when the drag ends. | ||
| let browserSelectionBroken = false |
There was a problem hiding this comment.
It seems weird to me that you have to build this whole machinery to handle gestures. You seem to imply that the dom keeps changing while you handle the gestures. Maybe there is a different solution, like a way to pause updates to the view while user performs these actions. Could be implicit (start of a gesture) or explicit (through some "pause" button).
|
|
||
| /** Like getVisibleRange but only includes rows actually within the scroll container's viewport, | ||
| * excluding virtua's overscan/buffer rows that are in the DOM but scrolled off-screen. */ | ||
| function getViewportVisibleRange() { |
There was a problem hiding this comment.
can you write a type for the return value?
|
|
||
| /** | ||
| * Merge browser-reported anchor/focus with tracked state to handle off-screen endpoints. | ||
| * When virtua recycles DOM nodes, the browser creates fallback endpoints at visible boundaries. |
| /** | ||
| * Merge browser-reported anchor/focus with tracked state to handle off-screen endpoints. | ||
| * When virtua recycles DOM nodes, the browser creates fallback endpoints at visible boundaries. | ||
| * This detects those fallbacks and substitutes the tracked logical positions. |
There was a problem hiding this comment.
substitutes the position into what?
There was a problem hiding this comment.
what does the function return?
The code in userSelect.ts is about simulating mouse selection behavior of a plain text in the context of shifting, re-purposed and deleted HTML nodes in a virtual list. This us unrelated to the logs content changing or not. |
3 participants
userSelect.ts now contains a complex but working implementation for consistent user selection within a virtual list. In the future I will try to simplify the implementation while preserving the UX. This fixes many issues like drag-to-select not working properly, etc.
Also, made it so that the Logs tab does not lose state when leaving it, avoiding having to re-load logs when switching between tabs of the same pipeline. When switching between pipeline logs are still reloaded.
Manual testing: extensively tested selection behavior using mouse in Firefox and Chromium-based browser. Tested the Logs tab correctly preserve and reload state when needed while switching the tabs and pipelines