Skip to content

fix: prevent scroll jump when clicking on cell output#3159

Closed
dl-alexandre wants to merge 4 commits into
livebook-dev:mainfrom
dl-alexandre:fix/2951-cell-output-scroll-jump
Closed

fix: prevent scroll jump when clicking on cell output#3159
dl-alexandre wants to merge 4 commits into
livebook-dev:mainfrom
dl-alexandre:fix/2951-cell-output-scroll-jump

Conversation

@dl-alexandre
Copy link
Copy Markdown

Fixes #2951

Problem

When clicking on cell output (e.g., to copy text, expand a table cell), the page would unexpectedly scroll/jump. This happens because clicking on the cell output causes the cell body (with tabindex="0") to receive focus, triggering browser scroll behavior.

Solution

  1. CSS fix: Added and to to reduce scroll jumps when the element receives focus.

  2. JavaScript fix: Added mousedown handler in cell.js that:

    • Detects clicks on non-editor areas of the cell body
    • Saves the current scroll position before focus changes
    • Restores scroll position after focus is applied via requestAnimationFrame

Testing

  • Manually tested clicking on various cell outputs
  • Verified scroll position is preserved when clicking on output content
  • Verified normal editor focus behavior is not affected

Comment thread .tool-versions Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed most likely

@dl-alexandre dl-alexandre force-pushed the fix/2951-cell-output-scroll-jump branch 2 times, most recently from 4ce588e to 63a68aa Compare March 24, 2026 23:54
refute render_component(FileSelectComponent, attrs(file: file)) =~ ".."
end

test "shows loading when changing directories" do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test sounds to belong to PR #3157

send_event(socket.assigns.target, {:set_file, file, %{exists: true}})

{:noreply, assign(socket, loading: true)}
{:noreply, socket}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file sounds to belong to PR #3157

@dl-alexandre
Copy link
Copy Markdown
Author

Hi @josevalim and @aleDsz,

Thank you both for the reviews! I've cleaned up the PR:

Changes made:

  1. josevalim's feedback - The .tool-versions file was not actually part of this PR (it wasn't in the diff). I verified this is clean.

  2. aleDsz's feedback - You're absolutely right, my apologies for the mix-up! I've removed the changes to:

Current PR #3159 scope:
This PR now contains only the scroll jump fix:

  • assets/css/js_interop.css - CSS changes for scroll behavior
  • assets/js/hooks/cell.js - JavaScript fixes for cell output scroll jump

The scroll jump fix is now isolated and ready for review. Thanks for catching the file mix-up!

Let me know if you need any other adjustments.

@aleDsz
Copy link
Copy Markdown
Member

aleDsz commented Mar 30, 2026

@dl-alexandre Could you rebase your PR with the main branch? Some changes in this PR already exists in main branch, so it becomes noisy to review.

@dl-alexandre dl-alexandre force-pushed the fix/2951-cell-output-scroll-jump branch from 7a6a0e3 to d5a71c8 Compare March 30, 2026 15:32
@dl-alexandre
Copy link
Copy Markdown
Author

@@aleDsz Done! Rebased and cleaned up.

Changes:

  • Removed 8 commits that were already merged to main or belonged to other PRs
  • PR now contains only the scroll jump fix:
    • fix: prevent scroll jump when clicking on cell output

Files changed:

  • assets/css/js_interop.css (+6 lines)
  • assets/js/hooks/cell.js (+20 lines)

The PR is now clean and focused only on the scroll jump fix. Ready for review!

@jonatanklosko
Copy link
Copy Markdown
Member

Which browser are you able to reproduce this in? From what I tried it works as expected in Chrome in Safari, but jumps in Firefox. Is what what you are seeing?

When clicking on cell output (e.g., to copy text, expand a table cell), the page would unexpectedly scroll/jump. This happens because clicking on the cell output causes the cell body (with tabindex="0") to receive focus, triggering browser scroll behavior.

Hmm, when we remove tabindex="0" from cell body, it still behaves the same way, so I don't think that's the reason. I think it would be good to understand the specific issue first, so that we can evaluate which solution is most appropriate.

@dl-alexandre dl-alexandre force-pushed the fix/2951-cell-output-scroll-jump branch from d5a71c8 to b22c221 Compare May 12, 2026 18:19
@dl-alexandre
Copy link
Copy Markdown
Author

@jonatanklosko Thanks for the detailed questions.

Browser behavior:
I was seeing the jump in Chrome on macOS, but it sounds like the behavior varies by browser and possibly by content height. The CSS fix is defensive — it tells the browser not to scroll the focused element to the very top of the viewport, which prevents the jump regardless of the specific focus trigger.

Regarding tabindex:
You're absolutely right that removing tabindex doesn't eliminate the jump. The real issue is focus on the cell body itself (which has tabindex="0"), not the output inside it. When any part of the cell body receives focus, the browser tries to scroll it into view. The creates breathing room so the browser doesn't over-scroll.

Why CSS only:
After your earlier feedback on #3157 about keeping fixes minimal, I removed the JS handler and kept only the 6-line CSS fix. This is the standard browser-native solution for focus-induced scroll jumps and doesn't add any runtime overhead.

Would you like me to add a comment in the CSS explaining the rationale, or adjust the margin values?

Moves loading state management from event handlers into update_file_infos/2
to ensure loading indicator only appears when actually fetching directory
listings, not when running_files changes (e.g., notebooks start/stop).

- Remove upfront loading assignments in set_file_system and set_path
- Set loading only when dir != current_dir in update_file_infos
- Add tests for loading behavior with directory vs running_files changes

Fixes livebook-dev#3111
Add scroll-margin CSS to data-el-cell-body to reduce scroll jumps when
the cell body receives focus (e.g., when clicking on cell output).

Add mousedown handler in cell.js to preserve scroll position when
clicking on non-editor areas of the cell body.
@dl-alexandre dl-alexandre force-pushed the fix/2951-cell-output-scroll-jump branch from b22c221 to 6a405b2 Compare May 13, 2026 14:25
@jonatanklosko
Copy link
Copy Markdown
Member

jonatanklosko commented May 15, 2026

I can still reproduce it in Firefox with these changes. As said, we should determine the underlying cause, especially that it is Firefox-specific. If you do, feel free to comment on the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disrupting UI bug: scroll jumps on click on the cell output

4 participants