Skip to content

Commit

Permalink
Fix Scroll Jank (#6394)
Browse files Browse the repository at this point in the history
**Problem:**
When scrolling the canvas in the hydrogen demo project, the canvas
intermittently freezes up for 100ms.

Turns out the reason is that the hydrogen Link component is mounting
elements to the screen when they become visible. And we use a
MutationObserver to listen to elements being mounted / unmounted to
trigger a dom-sampler re-run.
We disable the MutationObserver for continuous interactions with set
elementsToRerender, but the canvas scrolling interaction did not set
elementsToRerender.

`elementsToRerender` has been one of those editor state variables that
is always default value in the unpatchedEditorState, and we only set it
to an actual value in the patchedEditorState using patch commands from
the strategy codebase.
We talked about having some sort of safeguard in place that makes sure
that in the unpatchedEditorState these values are kept at their default.

**Fix:**
I think this PR solves both problems:
- a new `resetUnpatchedEditorTransientFields` function resets the
unpatchedEditorState's `elementsToRerender` field to
`rerender-all-elements`
- `SCROLL_CANVAS` sets unpatchedEditorState's `elementsToRerender` to
`[]` meaning the dom-walker will not interfere with the scroll
performance.
- because unpatchedEditorState elementsToRerender is reset at the end of
every dispatch, the change in SCROLL_CANVAS amounts to a transient
setting, very similar to how the editor state patches emitted by the
strategies work.

so we make sure that:
1. by the end of the dispatch, the UnpatchedEditorState's "transient"
fields are reset
2. non-strategy interactions can set these fields when they need to

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode
  • Loading branch information
balazsbajorics authored Sep 23, 2024
1 parent 4f2faf2 commit 1d38e63
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
13 changes: 12 additions & 1 deletion editor/src/components/editor/store/dispatch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ export function editorDispatchClosingOut(
// START_CHECKPOINT_TIMER likely did.
const transientOrNoChange = (allTransient || result.nothingChanged) && !anyFinishCheckpointTimer

const unpatchedEditorState = result.unpatchedEditor
const unpatchedEditorState = resetUnpatchedEditorTransientFields(result.unpatchedEditor)
const patchedEditorState = result.patchedEditor
const newStrategyState = result.strategyState
const patchedDerivedState = result.patchedDerived
Expand Down Expand Up @@ -1072,3 +1072,14 @@ function filterEditorForFiles(editor: EditorState) {
},
}
}

function resetUnpatchedEditorTransientFields(editor: EditorState): EditorState {
// reset all parts of the state which is meant to be "empty" or "default", and mostly expect the non-default values to come from patchedEditor
return {
...editor,
canvas: {
...editor.canvas,
elementsToRerender: 'rerender-all-elements',
},
}
}
2 changes: 1 addition & 1 deletion editor/src/templates/editor-canvas.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,6 @@ export function runLocalCanvasAction(
builtinDependencies: BuiltInDependencies,
action: CanvasAction,
): EditorState {
// TODO BB horrorshow performance
switch (action.action) {
case 'SCROLL_CANVAS': {
const newCanvasOffset = Utils.offsetPoint(
Expand All @@ -375,6 +374,7 @@ export function runLocalCanvasAction(
...model.canvas,
realCanvasOffset: newCanvasOffset,
roundedCanvasOffset: roundPointToNearestWhole(newCanvasOffset),
elementsToRerender: [], // we set this to [] to maximize the scroll performance, but it needs to be reset after the scroll! Ideally this would be in the patchedEditorState, but scrolling the canvas is not a continuous interaciton
},
}
}
Expand Down

0 comments on commit 1d38e63

Please sign in to comment.