Scroll behavior component

[ForLoveOfCats]

This is the work I outlined in #73 (comment)

Put simply this allows any widget to very easily support scrolling on its own while picking and choosing what behaviour to support yet remaining consistent with other scrolling widgets.

• The original Scroll widget has been renamed to AbsoluteScroll to indicate that it provides bi-directional scrolling for a child which has an absolute size. It can no longer restrict scrolling to a specific axis.
• List now manages its own scrolling instead of having to be wrapped in a Scroll. It has gained the ability to change the layout axis to horizontal with the .horizontal() builder fn. The layout code stretches children to full size on the non-primary axis like the Flutter ListView as this solves some issues specifically with inset scrollbars which will come after this patch.
• The now public methods and such on the scroll_component have been documented, including making some things like scrollbar point hit tests more well defined

There are more things I want to do (specifically inset scrollbars) and a few existing bugs I noticed I want to fix but this patch should be functionally identical to existing behaviour to avoid issues with reviewing and scope.

As an example here is the list example after changing the .fix_height(50.0) to .fix_width(200.0) and adding .horizontal() to the second List construction

[rjwittams]

I currently don’t understand what the actual problem is with the widget composition model (having Scroll decorate its child).
It looks like you end up sort of calling back into ScrollComponent and as such recreating the nesting that existed before.
It feels like everyone who wants “good” scrolling behaviour will need to understand this ...

I think this should be split to 1 PR adding the direction to list that demonstrates what the actual problem is, 1 doing the split if it is justified, and a final one doing the rename which only seems to break backwards compatibility?

[ForLoveOfCats]

Various issues with the currently existing Scroll layout can be seen in #799 and #1031.

I'll use the example of a multi-line text editing widget as that's what I've been building for my application using Druid. A text edit widget wants to consume all paintable size it is given without overflow unless the text bounds exceed that area at which point it needs to scroll on that axis. Currently a child of Scroll cannot find out what the max paintable size is, instead it will get a constraint between 0 and infinity on both axis (unless the scrolling is restricted to a specific axis at which point the infinity only exists on that axis) as logically the child should be able to have a size within those bounds and the scroll is supposed to be able to handle scrolling around that large or small widget. Secondly a text edit widget wants to be able adjust its scrolling in order to keep the editing caret visible or jump to a textual search result ect. This is troublesome because if the widget is a normal widget which expects to be wrapped in Scroll, as List is prior to this patch, it won't always be guaranteed to be the child of a Scroll. In addition adding commands to pass this sort of information around feels very hacky to me specifically but that is personal preference.

This sort of thing was technically already possible by splitting the text edit widget into two parts, one which is the parent to the Scroll and another which is the child of the Scroll. Then the parent half can interact with the scroll's public functions and coordinate the actual drawing with the child half by grabbing it from Scroll::child_mut(&mut self) -> &mut W. However that is a bunch of indirection and complication to achieve the same embedding of scrolling behaviour that this patch makes handle-able in less than 20 lines and near identical widget architecture compared to how it might otherwise be written if it didn't handle its own scrolling.

[ForLoveOfCats]

Sidenote it seems that when I force pushed to fix a spelling mistake the CI has decided that harfbuzz_rs should no longer build. This CI really seems to not like me. I know we've had strange issues with CI not passing and then passing when re-ran with no changes 🤷

Edit: Wait wait what? I just checked the dep trees and neither druid, nor druid-shell, nor druid-derive have any direct or indirect dependencies on harfbuzz_rs though it may be a dep which is not pulled in on Linux.

[rjwittams]

What do you mean by "max paintable size"?

The portion of your widgets logical display that is currently visible is:

let rect = ctx.region().to_rect(); in paint.

I assume you mean something different?
That is what I've used in my druid_table repo.

I don't think its realistic to avoid or have a distaste for widget composition using a widget library.
Complex widgets are going to be composed, and the various parts need to be able to communicate.
If the ways of communication currently available are not sufficient that needs to be articulated.
The usual alternative is each complex widget making its own mini adhoc widget library internally.

[ForLoveOfCats]

The paint region is accessible specifically on the PaintCtx not the LayoutCtx when performing layout which is where it is needed in order to consume all paint-able space available. Adding it to the LayoutCtx or even on BoxConstraints would not make sense in other cases (a container for multiple widgets can ask each child to decide on a size but until those are all done and then the layout calculated then the container doesn't know the painting region for each widget and passing in the container's own size would be nonsensical).

This isn't a case against widget composition. This is some core functionality which needs to be flexible enough to handle several different cases for different widgets so the simplest solution seems to be refactor out the common logic and allow each widget that needs special handling do a very small amount of work to pull that logic in and handle its own technicalities itself, whether that be an widget which is shipping in Druid or a downstream widget. When you just need to have a set size and have scrolling handled for you can wrap in an AbsoluteScroll and when you need to shove a bunch of widgets in a scrolling list then reach for List.

I'll give another example of where continuing to use a single Scroll widget and composing would lead to a silly situation. You had mentioned adding the directionality to List without the rest of the changes. If that were the case then a horizontal list would require being wrapped in a scroll which is specifically horizontal in order to have the expected results. Put a vertical list in a horizontal scroll and now you have infinite width widgets with the bottom half of your list being clipped. Of course these are logical errors in the developer's layout but they are silly situations which shouldn't need to be possible. Furthermore think of widgets which just don't make sense when not wrapped in a Scroll, one could write the new fn in such a way that it returns it pre-wrapped in a Scroll (this could be applied for the previous example with a list in a scroll) but now it is returning a scroll with the expected widget instead of the expected widget which is surprising and would be less than idea for a builtin widget especially when now reaching into the widget required a call to child or child_mut on the scroll first.

I agree that inter-widget communication is an area which needs work. However I'll defer to Colin on how hacky it feels to use that sort of thing to solve this #799 (comment) (do note that his opinion on the matter may have changed since this comment however much I agree with it, I don't want to put words in his mouth). The existing Scroll was becoming a rats nest of codepaths, especially when I added inset scrollbars in a now stale patch that never reached maturity. That would only get worse by continuing to add support for cases where a child needs some special attention. See #1031 (comment) for an example of a widget which needs more communication with the parent scroll and would also be better suited to always be capable of scrolling.

At the end of the day this is about ergonomics, minimizing silly situations, and attempting to solve the thousand little cuts that have popped up with laying out in a Scroll by just getting out of the widget's way while still having the same outward consistency with much less mental load compared to passing information and requests around with commands.

[luleyleo]

This looks promising! I did only skim it and play with the examples, but I left comments on the things I've noticed.

[rjwittams]

I’ve read it again and I don’t like the approach, sorry.
I think we need a better protocol for the scroll component to talk to it’s children. It may be an event that gets passed down.

If something as simple as a list needs to jam this extra ‘component’ in it then pretty much every growable widget does, and then every widget needs to replicate the configuration parameters of scrolling and the events emission etc.
I’ll look into 799 which has a much more concise explanation of the issue. I’ll look at the mentioned flutter system.

[Zarenor]

I think separating out the machinery of scrolling in a way that we can integrate it into other widgets is great!

This seems good, but I agree that ScrollComponent needs some basic docs to understand what's happening - I had to go back and forth between the new AbsoluteScroll and List impls to grasp what was going on, which we don't want a user to have to do.

Secondly, I see from the List impl the the intended way to make a ScrollComponent one-directional is just to layout within the viewport bounds? I'm not sure if I love that. It feels like the kind of invariant that's easy to accidentally violate, and end up with a container that's supposed to scroll in one direction that now scrolls in two.

Otherwise, I don't see any issues in the implementation that jump out at me. This looks like a good, fairly clean way to break this functionality out. Nice work.

[Zarenor]

To reply to the concerns about children knowing viewport size and that not being a part of this patch - We definitely need the functionality, and it definitely is not something that we're going to push down the lifecycle or event tree unconditionally. That's just too much extra noise.

We've had a few discussions about how and where we want to implement these notifications, but I don't think this PR should be blocked on them. It's a fairly orthogonal change, and it centralizes the place where we would make those changes (if we want them to apply to everything scrollable, and everyone uses ScrollComponent for scrolling).

I don't see why we wouldn't want to do this where we can - wrap up a set of behaviors that will be common among a few widgets, and make them usable as a component, rather than adding a layer of indirection. I think this helps with the issues we sometimes face where there are difficulties communicating with widgets that aren't your direct descendants, by making it possible to remove a layer in a lot of instances. I'd be interested if there's a case I'm not thinking of where this is a regression, though.

[rjwittams]

Could this be called ScrollBehaviour then? ScrollComponent is misleading. I also think Scroll should retain its name as AbsoluteScroll adds nothing

[ForLoveOfCats]

I'm not especially attached to any specific names but renaming Scroll to AbsoluteScroll does gain clarity that it is not intended to perform any fancy layout and it will accept a child which has an absolute size between 0 and infinity on either axis. You can't put a widget under it and expect that widget to be able to figure out some reasonable size from the constraints passed down to it as all it is told is somewhere between 0 to infinity which is part of the problem that needed to be solved for widgets which expect to be able to do that. The name specifies what specific focused usage case it has. I'm open to name suggestions but currently I don't want to keep it named Scroll because that sounds much more general when I expect most out in the wild usage cases to be better suited to the new supped up List.

[rjwittams]

AbsoluteScroll is a terrible name.

This is really going to complicate the callbacks in #1107 .
The callbacks would logically live in the scrollbehaviour.
But the containing widget will have to pass on their registration, and so will every owner of a ScrollBehaviour.

I still think we are going to have to actually fix this properly at some point.

[jneem]

I don't think "terrible" is a useful word for advancing this discussion.

Having said that, I also don't see the benefits of renaming Scroll right now. We're not planning to use the name Scroll for anything else yet, so it just introduces API breakage. In the hypothetical future when we have a richer set of scrollable widgets, I would be more inclined to support a rename. (Plus, in that hypothetical future it will be easier to devise a consistent naming scheme.)

[Zarenor]

These callbacks you're talking about aren't going to be registered by some sort of reference or anything. That's not how things are passed around in druid - you don't get a reference to other widgets you don't own across tree traversals.

Either they'll be set up at first instantiation by the setup code, where everything is owned by the function body before being arranged in the tree, or it will be some sort of command-like thing that traverses up the tree, rather than down.

In either of these cases, I don't see how this design hinders this. Is there some obstruction I'm not seeing? Having to pipe things through your own API (i.e. delegation) is common in composed (rather than inherited) systems.. that is, this doesn't seem that weird to me.

@rjwittams I understand some frustration at having the design you just built (using the changes you're proposing in #1108) have APIs changed under it, but these kinds of changes are exactly why we are in 0.x versions; world-breaking changes are run-of-the-mill between releases.
I'm having a hard time understanding the reasons for your strong opposition here - I just want to understand what you're seeing that I'm not seeing.

[cmyr]

I think that this is addressing a significant problem, and I do not think that the direction here represents a long-term solution, but this does not mean I am opposed to merging.

I think the concerns about ad-hoc composition are reasonable ones, and the fact that we are doing ad-hoc composition suggests to me that there are some fundamental architectural issues that we will need to address to solve this problem.

But also: I do not know what the correct solution is, and I do not currently have the month+ I think it would take to try and address this systematically, to research other solutions, to experiment with APIs, etcetera.

I do have some hunches about what the solution will be, though, based on experience with other APIs.

The major thing I expect to see, long-term, is a separation of the 'viewport' from the scrolling machinery. The 'viewport' (Viewport in flutter, NSClipView in Cocoa) is basically a widget that has a child, and an offset, and shows some region of the child based on the offset and its own size.

In this world, a Scroll widget would have a Viewport child; it would be responsible for handling gestures and setting the correct offset on the viewport. I think that the scroll bars would also be a separate widget, although this raises an interesting issue around how these components would communicate, although this isn't super hard to imagine in the current architecture; the scroll widget would pass mouse events to the scrollbars, and could see if they were handled and if the scrollbar position had changed, and then it could communicate this to the viewport.

Looking at flutter again, it looks like they have something like Scroll/ScrollComponent, that they call Scrollable; it's a widget that seems to only really be used as part of other widgets. I think this makes sense; the majority of the time that people want a scrollview it is for a scrollable list or a scrollable document, and having these cases be handled separately makes sense to me.

There are some additional concerns about how to communicate back up the tree, and I do also have some ideas here; I'll write them up separately.

Basically: I think this patch is a bandaid that enables a few concrete concerns, but longer term we're going to need to redo this work. Bandaids aren't bad, though; I have a box of them at home, and sometimes I need to use them. I want to be clear, though, that I don't think this is the direction we want to take in the long-term; so for instance if this is intended to be the first in a series of patches that continues in a similar direction I would perhaps discourage that.

@ForLoveOfCats how does this sound, to you? Are we on a similar page?

[ForLoveOfCats]

Sorry for the delay, I've been exceedingly busy for the past few days.

I think the apprehension about this patch potentially having the affect of downstream developers embedding this component instead of trying to compose Scroll first is valid and understandable. However I think that not composing with widgets being treated as such a red flag is focusing on an ideal over potential practicality. I'm not saying that we should not explore more robust scrolling widget composition but instead that it shouldn't be grounds for dismissing the merits of other solutions out of hand.

I don't want to come off as though I hold this patch to be the one true way forward or any nonsense like that. If a better solution presents itself then that's awesome and this PR should go straight into the trash. I'm just not convinced that any of the ideas that have been tossed around to date using widget composition solve the problems that this PR does and they introduce additional complexity which feels like over-designing something that doesn't seem to require it.

You mention that one potential way forward can be to have a dedicated viewport widget which paints a child with an offset and then have a scrollbar widget housed in the custom widget. I have several practical and ergonomic issues with that approach. Firstly the viewport widget's logic is roughly three lines of code (with_save { transform; clip; paint as usual }). Initially I thought that it might have potential usage outside this specific domain and thus would justify existing to be able to layer atop any arbitrary widget but when inspecting the Cocoa and Flutter docs that doesn't seem to be the case in either of those toolkits. At that point it might as well exist as part of the scrollbar widget except at that point it's the same situation we have with Scroll in current master. That doesn't fix the issues that I'm attempting to solve here, mainly the difficulty in coordinating size between your widget and the scrolling widget that your widget is wrapped in. In addition to that the approach I took significantly simplifies any potential approach to allowing a widget to set its scroll offset (it owns the ScrollComponent), getting the current scroll offset for example to load more content as the user nears the bottom of the page.

The potential path that you brought up would not solve these issues, only add more layers of abstraction to the current system. I realize that you mentioned improvements to inter-widget communication, especially upwards, which is definitely needed but regardless of how much that improves, the general changes wouldn't address the primary motivators of this patch. Earlier in this thread I tossed out an example of a technique which would currently be required that I felt was undesirable and it appears that it would still apply in this hypothetical system. That technique was the idea that you would build two widgets and sandwich a Scroll in the middle, the top widget would control overall size and offset while the bottom widget would do all the painting. I find that a poor approach because now your widget logic is needlessly strewn between two widgets. The internal state probably ends up in the bottom widget and the logic in the top widget which is cumbersome to reach through the scroll to get a reference. The bottom widget is a glorified mime, just painting the data assigned to it by the top widget and not benefiting from being a widget. Add in the extra boilerplate and it just feels more clumsy. It's not outright horrible but in my opinion distasteful due to complicating and adding more layers to code which is already trying to solving some other task.

This PR is conceptually simple, doesn't rely on the runtime widget tree, and doesn't require much modification of an existing widget (such as List) to add the functionality. I generally value the simplicity and ease of application above following some pre-existing idiom, especially when it feels like the idiom fits the problem poorly. Here it feels like the goal of solving this via composing whole widgets is getting in the way of looking at the current papercuts and trying to solve them in the most straightforward way possible. Again I'm not against a different approach if it proves to be a better solution in the big picture. It's just frustrating having brainstormed a solution to a set of persistent issues, ask for feedback, get none, go ahead and come out with a simple and widely applicable solution but have it be held up because it doesn't solve it in the way others might want but without providing any alternative solutions to the actual issues on the table. That is not the fault of anyone, I just felt I had to clarify the specific frustrations I'm feeling in the hopes that it can give more context to the rest that I've said here.

I may be misinterpreting what you mean or missing some context about Cocoa or Flutter and I want to acknowledge that possibility. If this is the case please do let me know. I do know that regardless of the outcome of this PR I'm going to take a break from the topic of scrolling for a while. It's an area with plenty of potential for improvement (inset scrollbars, animated scrolling, various bugfixes) but there is a lot of disagreement about the direction forward without much investment from many others in the specific issues that are being solved here. Going on a bug fixing binge seems like fun :)

As a final note: if this PR ends up going through I am perfectly willing to strip out the changes to List which would allow them to have their own discussion of merits (I'm not 100% sure myself that List should be scrollable on it's own, it may be useful in some situations to not be able to). This thing needs a squash anyway...

[cmyr]

Just want to directly address a few comments here; I'm going to avoid talking much about a possible alternative design, since I think that isn't especially relevant to whether or not we land this particular PR.

I think the apprehension about this patch potentially having the affect of downstream developers embedding this component instead of trying to compose Scroll first is valid and understandable. However I think that not composing with widgets being treated as such a red flag is focusing on an ideal over potential practicality. I'm not saying that we should not explore more robust scrolling widget composition but instead that it shouldn't be grounds for dismissing the merits of other solutions out of hand.

Happy to hear; I think we're all on the same page here about the fact that everything involves compromise, and that the real discussion here is about whether this compromise makes sense.

It's just frustrating having brainstormed a solution to a set of persistent issues, ask for feedback, get none, go ahead and come out with a simple and widely applicable solution but have it be held up because it doesn't solve it in the way others might want but without providing any alternative solutions to the actual issues on the table. That is not the fault of anyone, I just felt I had to clarify the specific frustrations I'm feeling in the hopes that it can give more context to the rest that I've said here.

I very much hear you here: I've personally definitely been pretty MIA the past month and a half, and haven't been able to participate in all of these discussions as much as I would have liked, and I recognize how frustrating it can be to want to move in some direction and feel blocked by other people's being busy.

I may be misinterpreting what you mean or missing some context about Cocoa or Flutter and I want to acknowledge that possibility. If this is the case please do let me know. I do know that regardless of the outcome of this PR I'm going to take a break from the topic of scrolling for a while. It's an area with plenty of potential for improvement (inset scrollbars, animated scrolling, various bugfixes) but there is a lot of disagreement about the direction forward without much investment from many others in the specific issues that are being solved here. Going on a bug fixing binge seems like fun :)

Scrolling is a fundamentally hard problem. I know that the flutter scrolling mechanics were rewritten multiple times, and were multi-month projects. It isn't a problem I've thought about in a lot of depth, which is why I'm reluctant to try and offer concrete proposals; all I know is that the flutter API was arrived at via contributions from multiple people smarter than I am, and there will be a strong rationale of some kind for the design they ended up with.

As a final note: if this PR ends up going through I am perfectly willing to strip out the changes to List which would allow them to have their own discussion of merits (I'm not 100% sure myself that List should be scrollable on it's own, it may be useful in some situations to not be able to). This thing needs a squash anyway...

This is a good question. I do suspect that list changes should, at the very least, be reviewed separately, but I'm not opposed to the idea of having scroll logic built into the list widget; this is also something that is the case in flutter, with the ListView widget.

[cmyr]

Okay so: I am happy to get this in shortly. Some quick stuff:

• this should get a rebase, so that you get the all the doc warnings, and those should be addressed
• if it is feasible, the changes to list should be split out and done as a follow-up PR.

So basically: let's get this into a state where it's ready to merge, and then I'll do a last pass. 👌

[cmyr]

Okay two little nits.

Is my understanding correct that this is not a breaking change? That is, the existing scroll widget continues to work exactly as before?

[ForLoveOfCats]

Yes now that the List changes have been removed this is no longer a breaking change.

I had one concern I was looking into which turned out to be well founded. There were more cases where the component needed to consume events.

[cmyr]

With this not being a breaking change I am happy to get it in. Thank you for your patience and for the discussion!