Move default scroll navigation keys out of Widget and into ScrollableContainer
#2337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea here is that not every widget will scroll, and as such not every widget needs to have default bindings for calling the scrolling methods. Generally scrolling is something done in a container. These days we have *Scroll containers. As such it makes sense to introduce the bindings in a common parent class for those containers. This commit moves the binding from widget and creates that common parent class, and then has HorizontalScroll and VerticalScroll inherit from it. This is, it should be noted, a breaking change. Any code that creates a scrolling widget that assumes that the bindings are just there, where that widget doesn't inherit either from HorizontalScroll or VerticalScroll, will suddenly find that scrolling with the keyboard is no longer possible. See Textualize#2332.
The `Body` class inherited from `Container` rather than one of the scrolling containers; until now it had worked because `Widget` provided the bindings to make this happen, now that they've moved into `ScrollableContainer` that stopped working.
This is, to some degree, rendered moot by Textualize#2332, but for the moment it still feels worth doing. The initial intention was to make sure that non-scrolling containers and their child classes don't have bindings that may mask other uses for navigation keys. However, it was realised that the "problem" affected more than just containers (hence Textualize#2332). But... on the off chance we add any more default bindings to `Widget` (unlikely, but still), this will mean that they don't leak into the containers unless we intend them to. See Textualize#2331.
…oach Now that navigation bindings don't pollute the whole widget hierarchy any more some of these tests can be tidied up.
I'd accidentally started adding things under v0.20.1 rather than under a new unreleased heading.
This was
linked to
issues
Apr 20, 2023
rodrigogiraoserrao
approved these changes
Apr 20, 2023
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
willmcgugan
approved these changes
Apr 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the default bindings for navigation keys -- binding them to various scroll actions -- from
Widgetand introduces them in a newScrollableContainercontainer. Primarily this is to address #2100; the problem here being that not all widgets want to scroll, but many non-scrolling widgets will want to do other things with navigation keys and the bindings can end up clashing when it comes to sorting out descriptions and entries (or lack of them) in theFooterof an application.This is a breaking change. Any widget that scrolls, and expects keyboard-driven scrolling "out of the box", will cease to be navigable via the keyboard. This is easily solved by switching to
ScrollableContainer,VerticalScrollorHorizontalScrollas the parent (so far any examples I've seen where this is an issue have inherited fromWidgetorContainerso changing the parent class won't be much of a change).This PR also goes on to change the demo, plus
ScrollView, to ensure scrolling works as before there.I've run through the main examples and they all look fine.
While the main thrust of this PR sort of renders the need to do it moot, I've also rolled #2331 in here too; mostly now this will work as a guard against any default bindings that may get added to
Widgetin the future, such that we should think about the consequences for containers.