Add scrollbar margin to surfaces

[danielwe]

Alright, second attempt to fix #9248. This gives surfaces a right-hand margin that is distinct from padding, solely for keeping the terminal content out of the way of scrollbars. That way, the surface itself can fill the whole SurfaceScrollView and keep drawing the background behind the transparent scrollbar.

On the macOS app side, this means that surfaceView cannot be a child view of documentView, since the latter is automatically resized to avoid overlap with the scrollbar. Instead, surfaceView is now a direct subview of the SurfaceScrollView, stacked behind scrollView. This actually simplifies the scrollbar implementation quite a bit, since the origin of the surfaceView frame no longer has to change as we scroll, as it's relative to the fixed SurfaceScrollView rather than the moving documentView. That means we no longer have to listen and synchronize to bounds changes on the contentView. The only time we need to worry about the surfaceView frame is when there are layout changes.

This also provides #9255 for free. It would be more work to condition the margin on whether the legacy scrollbar is visible or hidden.

Along the way, I fixed a bug where the layout recalculation wasn't taking into account the change of padding around the terminal grid when the surface is resized. This change must be reflected in the height of the documentView even if the number of rows haven't changed, otherwise the scrollbar will pop up out of nowhere when resizing an empty surface (see #9241).

Also committed some drive-by simplification of the padding math from #9241.

---

I hope this can be a step towards addressing the concern I voiced here: #9254 (reply in thread). Basically, I think the terminal/surface should ignore the scrollbar margin for the alternate screen (or, more generally, when max scrollback is zero). With this PR, all that could be done in the core without the macOS app's involvement. But that's a future PR.

[danielwe]

[mitchellh]

I haven't read the full thing yet, but I think we don't need to introduce a dedicated scroller width awareness into core. I'd rather allow the apprt to document "safe areas" similar to macOS and let the core reuse that concept. This would cover more than just one side (right) and allow padding any side.

Not exactly sure what that API looks like yet.

[danielwe]

I think we don't need to introduce a dedicated scroller width awareness into core. I'd rather allow the apprt to document "safe areas" similar to macOS and let the core reuse that concept.

I considered a more general implementation along these lines, but the abstraction felt premature (YAGNI et cetera) so I went with the minimal solution. In any case, I would argue that it's desirable for the core to know that an unsafe area is scrolling specific in order to address the following:

I think the terminal/surface should ignore the scrollbar margin for the alternate screen (or, more generally, when max scrollback is zero).

I find it surprisingly grating to have this superfluous margin in a text editor. The effect depends on window-padding-color, but it's not good either way:

Solving this problem either requires the core to know that an unsafe area can be ignored for terminal screens that don't have scrollback, or requires the apprt to know whether or not the active terminal screen has scrollback. I thought the former seemed cleaner.

[danielwe]

Solving this problem either requires the core to know that an unsafe area can be ignored for terminal screens that don't have scrollback, or requires the apprt to know whether or not the active terminal screen has scrollback. I thought the former seemed cleaner.

I take this back. Playing around I realize that by far the simplest way of keeping this in sync is use a sentinel value in the scrollbar state, scrollbar.total == 0, to indicate that the current screen isn't scrollable at all, and let the apprt adjust its safe area accordingly.

EDIT: This can be done completely independently of this PR, so I submitted #9295 separately.

[danielwe]

I factored out the pure bugfix part of this PR into #9296. Converting this to draft for now.

[danielwe]

OK, I've pushed a revised approach. Here are some key points, probably repeating a few things from earlier, but that way you don't have to scroll(!) back and reread.

• The apprt can now tell the core to add edge insets (margins/unsafe areas) to any edge of the surface using a struct SurfaceEdgeInsets. In fact, surface.sizeCallback now requires this argument, but the default is all-zero, so apprts that don't use it can just pass .{}.
• The C API provides functions both with and without the inset argument, so you don't have to write out redundant zero structs when using this API without insets (e.g., the iOS/UIKit app).

The macOS app uses this API to make room for the legacy scroller overlaid on the surface. For this to work properly, surfaceView can not:

• Be a subview of scrollView.{contentView,documentView}, as it would be clipped or resized to not overlap with the scrollbar, defeating the purpose.
• Be a direct subview of SurfaceScrollView, as it would either be drawn over the entire scrollView, obscuring the scroller and blocking it from mouse events, or under the entire scrollView, which means you wouldn't be able to focus the surface by clicking on it, due to how hitTest works. Also it could end up behind some background styling views of the kind discussed in macOS: remove scroll edge styling with hidden titlebar #9317.

The solution is for surfaceView to be a subview of scrollView, sandwiched below the NSScrollers but above scrollView.contentView. To this end I implemented a subclass of NSScrollView that overrides addSubview to maintain this invariant. This class also subsumes the subview filtering from #9317, as we can take care of that too in the overridden addSubview.

Moving surfaceView out of the contentView/documentView hierarchy has the side benefit that it now has a fixed frame and no longer requires any synchronization logic to compensate for the scrolling of its superviews.

Naming is hard. Looking forward to the bikeshedding.

Btw. this includes the fix from #9296 because it would conflict anyway. If #9296 is merged first I'll rebase. Done.

[danielwe]

Pushed a rebase after the #9296 merge