Skip to content

Conversation

@danielwe
Copy link
Contributor

@danielwe danielwe commented Nov 1, 2025

Fixes #9444

@danielwe danielwe requested a review from a team as a code owner November 1, 2025 21:41
@danielwe
Copy link
Contributor Author

danielwe commented Nov 2, 2025

Oops, this doesn't restore the scrolled position of the content view, so in the cases where the scrollbar would previously disappear, it now gets redrawn as scrolled to the bottom, regardless of what the surface scroll state is.

Will fix tomorrow.

@danielwe danielwe marked this pull request as draft November 2, 2025 06:23
@danielwe danielwe force-pushed the scrollbar_persistence branch 2 times, most recently from c02cca3 to 8f17a65 Compare November 4, 2025 07:02
@danielwe danielwe changed the title macOS: Cache scrollbar state across SwiftUI rebuilds macOS: Refactor scrollview to preserve state across split tree changes Nov 4, 2025
@danielwe danielwe force-pushed the scrollbar_persistence branch from 8f17a65 to 491d654 Compare November 4, 2025 07:58
@danielwe danielwe force-pushed the scrollbar_persistence branch from 491d654 to afc64f6 Compare November 4, 2025 08:03
@danielwe danielwe marked this pull request as ready for review November 4, 2025 08:46
@danielwe
Copy link
Contributor Author

danielwe commented Nov 4, 2025

OK, this turned into something a little bit bigger than expected. The existing code had a lot of redundant hidden/implicit method calls, and for any single scrollbar update or surface resize, you'd probably take at least a couple of passes through the layout method. Fixing #9444 required synchronizing the full scrollview state in layout, and this turned out to push the redundancy over the edge, making it possible to trigger infinite loops in some cases.

So I did a deep dive into the hidden parts of the call graph (so much printf debugging!) and tried to root out most of the redundancy. Some key points:

  • Explicitly setting the frame size of the view managed by SurfaceRepresentable is unnecessary and in fact strongly warned in the docs: https://developer.apple.com/documentation/swiftui/nsviewrepresentable. So I removed that.
  • Unconditionally setting needsLayout = true in setFrameSize is unnecessary and prone to creating loops in the call graph, because setFrameSize is often called implicitly when you change the frame of subviews, and layout often changes the frame of subviews. In any case, layout will always be called when the frame size is actually changed, so the override isn't needed and I removed it.
  • With layout no longer being called all the time, a more robust approach was needed to handle the NSScrollPocket weirdness from macOS: remove scroll edge styling with hidden titlebar #9317, so I added an observer for this. I also did a deep dive into why these subviews break the hidden titlebar style but not the others. See the docstring comment for the full story; the conclusion is that the right way to handle them is to ensure their frame is zero, unless we're in native fullscreen.
  • With layout no longer being called all the time it also became possible to lose the scrollbar margin via automatic calls to SurfaceView.setSurfaceSize triggered by backing changes, especially when in native fullscreen.1 The solution was to save and reuse the size received through sizeDidChange. (Eventually, once we get something like Add scrollbar margin to surfaces #9291, we'll save the inset instead of the size.)

It's a larger diff than I originally expected for this PR, but not huge in the scheme of things, and if you review it commit by commit I think it should be quite straightforward.

Footnotes

  1. Strictly speaking, this was already possible, these changes just made it more frequent.

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.

macOS: changing the split tree makes scrollbars disappear

1 participant