Retry node-selection scroll until the cursor is on screen#291
Retry node-selection scroll until the cursor is on screen#291johannesmutter wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
michael
force-pushed
the
fix/node-scroll-after-relayout
branch
from
9b5864c to
a05d083
Compare
|
Problem description is spot on. setTimeout(fn, 0) was too naive here. Fix definitely improves situation. I'm still testing and reproducing as I still found cases where the active node selection isn't revealed. will document as soon as I can reproduce reliably. We definitely need some notion of "wait until layout has settled" then scroll. Though we might only get heuristics here. So hard to say which approach is the simplest+most robust. Experimenting with an alternative approach (#292) and applying it to all scroll to situations (not just node selections) |
A single deferred scroll computed against an un-settled layout strands the cursor when images / nested mounts continue reflowing after the triggering action — the "still found cases where the active node selection isn't revealed" case on #291. Node selection (__scroll_node_cursor_into_view): - IntersectionObserver on the two nodes flanking the cursor gap re-fires the scroll on each viewport-edge crossing, until the selection moves on, the user scrolls (after a 500ms grace so the settling relayout and any inertial scroll can't abort it), or a 2s safety timeout fires. Replaces the rAF-poll-until-deadline loop with an event-driven watch that catches relayout shifts past the first visibility-pass. Property and text selection: - setTimeout(fn, 0) -> await tick() then a rAF (with 50ms fallback for background-tab rAF throttling). tick() drains Svelte's effect queue so the scroll measures the post-flush DOM, not the pre-flush one. - Job-id cancellation so a fresh render invalidates an older pending scroll instead of letting both yank the viewport. - Text selection captures focusNode.parentElement at run time, not at enqueue time, so a focus that moves during the settle window doesn't scroll a stale element. Node-cursor watch starts also bump the property/text job-id, so a node render cancels any pending property/text scroll scheduled just before it. Adopts the deferred-scroll helper shape from #292 for the property/text generalization; #292 can be closed in favour of this combined approach. svedit-lab "Run Smoke Tests": 25/25 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@michael Pushed a1c4ef6 combining both approaches. Node selection: IntersectionObserver watching the two flanking nodes — re-fires the scroll on every viewport-edge crossing, so the relayout cases that survived the first visibility-pass (your "still found cases") now get caught. Stops on selection change, user scroll (with a 500ms grace so the settling itself can't abort it), or a 2s safety timeout. Property + text selection: adopted your tick() + (rAF || 50ms) + deferred_scroll_job_id pattern from #292 — those don't have the flank-settling structure, so single-shot is the right shape there. A node-scroll-watch start also bumps the job-id to invalidate any pending property/text scroll queued just before it. |
|
This looks good on a test within Svedit in Chrome. But since this introduces quite a bit of machinery and event sources, I'd like to take my time to test against this branch with the EW app for a while, and merge once confident enough. Close to releasing 0.10.0, and this will likely be part of a follow-up patch release. Do you really think binding to wheel and touchmove is necessary here? If I understand correctly this is meant to cancel the automatic scroll once the user interacts. Just my gut says that these additional event sources will cause race conditions. I don't think the scroll-into-view flow should run longer than 500ms, and I think that's an acceptable period to block the UI for scroll input. So maybe the cancel logic isn't needed. Another concern: The intersection observer assumes that the window viewport is scrollable, but Svedit could also be mounted/rendered inside an overflowing container. Found another remaining scroll-into-view issue: When you type inside a button right at the right edge of the button container in story, and then use the right arrow or down key, the last node gap will be selected, but not revealed in the viewport. lala.mov |
A single deferred scroll computed against an un-settled layout strands the cursor when images / nested mounts continue reflowing after the triggering action — the "still found cases where the active node selection isn't revealed" case on #291. Node selection (__scroll_node_cursor_into_view): - IntersectionObserver on the two nodes flanking the cursor gap re-fires the scroll on each viewport-edge crossing, until the selection moves on, the user scrolls (after a 500ms grace so the settling relayout and any inertial scroll can't abort it), or a 2s safety timeout fires. Replaces the rAF-poll-until-deadline loop with an event-driven watch that catches relayout shifts past the first visibility-pass. Property and text selection: - setTimeout(fn, 0) -> await tick() then a rAF (with 50ms fallback for background-tab rAF throttling). tick() drains Svelte's effect queue so the scroll measures the post-flush DOM, not the pre-flush one. - Job-id cancellation so a fresh render invalidates an older pending scroll instead of letting both yank the viewport. - Text selection captures focusNode.parentElement at run time, not at enqueue time, so a focus that moves during the settle window doesn't scroll a stale element. Node-cursor watch starts also bump the property/text job-id, so a node render cancels any pending property/text scroll scheduled just before it. Adopts the deferred-scroll helper shape from #292 for the property/text generalization; #292 can be closed in favour of this combined approach. svedit-lab "Run Smoke Tests": 25/25 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
johannesmutter
force-pushed
the
fix/node-scroll-after-relayout
branch
from
a1c4ef6 to
078fce6
Compare
|
Another issue I found: set this css anywhere and node gaps will not render at all: |
|
tmp: testing new approach. performance:
|
…d when typing in a button in story.buttons node array where the text caret touches the end edge of an overflowing container: use scroll padding to leave "breathing" room
|
This issue is now addressed by adding css scroll padding:
|
Three changes to __scroll_node_cursor_into_view in Svedit.svelte:
- Drop the wheel/touchmove cancellation path and shorten the safety cap
from 2000 ms to 500 ms. At 500 ms a deliberate user scroll inside the
watch isn't a coherent human gesture, so the cancellation event
sources Michael flagged as race-prone are no longer needed.
- Use the nearest scrollable ancestor as the IntersectionObserver root,
and give __intersects_viewport an optional `root` parameter. When
Svedit is mounted inside an overflow:auto container, the observer
and the visibility check both test the surface the user actually
scrolls instead of the window viewport.
- Restructure the edge branches: cursor at offset 0 / trailing now
skips the flank-visibility guard (the cursor sits past its single
flank, so flank visibility tells us nothing about the cursor). Each
edge branch sets the matching internal scroll, then falls through to
scrollIntoView on the flank only when the flank is still off-screen.
This unblocks two cases the previous "max_left === 0 && max_top === 0"
gate missed:
- Typing at the right edge of an overflowing button + arrow-right
-> trailing gap reveal (combined with NodeArrayProperty's scroll-
padding default, the trailing gap stays positioned through typing).
- #288 offscreen-array-undo: 10 buttons -> delete -> scroll to bottom
-> undo. Previously the trailing branch's scrollIntoView was gated
on both max_left and max_top being 0 and skipped in horizontal-
overflow + offscreen-vertically scenarios.
Four new tests pin the regressions:
- selection_scroll: model-driven selection on an offscreen
horizontally-overflowing array scrolls it into view (#288 regression).
- gap_visibility: mid gap stays .positioned when only the prev flank is
in near_map (locks in the wrap/grid prev-only rule).
- gap_visibility: mid gap-marker emits when only the prev flank is in
array_indices (matching rule for NodeGapMarkers).
- gap_visibility: scroll-padding-inline / -block default is non-zero on
every node-array (locks in the typing-edge fix).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@michael I addressed your feedback and a couple other issues I found. |
Is this addressed also already? And can you explain what is/was the issue here? And when would you use this? From the docs I found I still don't understand the use-cases and problems it solves. Mainly a performance optimization I guess? |
|
RE: content-visibility: auto
It breaks node gaps because of how CSS anchor positioning interacts with containment. While the cell is off-screen, So this issue is for another day/ PR. The fix is either:
|
2 participants
Problem. After an undo/insert, the restored nodes' layout (images, content sizing) settles over several frames.
__render_node_selectionscrolled once, at a fixed-too-early moment, against an un-settled layout — leaving the cursor off-screen. Repro: select all buttons in a story → delete → scroll away → undo; previously only a second undo revealed them, once layout had settled.Fix. The render no longer scrolls once and gives up: if the DOM isn't mounted yet it retries on the next frame, and the scroll re-runs each frame until the cursor is genuinely in view (bounded to ~1s).
Why this layer. Earlier attempts each baked in a timing assumption —
setTimeout(0), a 500ms observer window, leading-edge visibility — and each was wrong in some case. This assumes nothing: it's defined by the outcome ("is the cursor in view?") and converges regardless of when or why layout settles. It tolerates the async layout rather than eliminating it — which is correct, since indeterminate content size until load is inherent to the platform. Going deeper would mean deterministic layout from the model (e.g. media nodes carrying dimensions) or an unbounded event-drivenResizeObserverinstead of the bounded poll — both optional refinements, not corrections.Resolves the remaining multi-node case from #288.