Merge pull request #2337 from davep/bug/2100/invisible-keys

Move default scroll navigation keys out of `Widget` and into `ScrollableContainer`

CHANGELOG.md

@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+### Changed
+
+- Breaking change: standard keyboard scrollable navigation bindings have been moved off `Widget` and onto a new base class for scrollable containers (see also below addition) https://github.com/Textualize/textual/issues/2332
+- `ScrollView` now inherits from `ScrollableContainer` rather than `Widget` https://github.com/Textualize/textual/issues/2332
+- Containers no longer inherit any bindings from `Widget` https://github.com/Textualize/textual/issues/2331
+
+### Added
+
+- Added `ScrollableContainer`; a container class that binds the common navigation keys to scroll actions (see also above breaking change) https://github.com/Textualize/textual/issues/2332
+
 ### Fixed
 
 - Fixed dark mode toggles in a "child" screen not updating a "parent" screen https://github.com/Textualize/textual/issues/1999

src/textual/containers.py

@@ -3,7 +3,11 @@ Container widgets for quick styling.
 
 """
 
+from __future__ import annotations
 
+from typing import ClassVar
+
+from .binding import Binding, BindingType
 from .widget import Widget
 
 
@@ -19,7 +23,35 @@ class Container(Widget):
     """
 
 
-class Vertical(Widget):
+class ScrollableContainer(Widget, inherit_bindings=False):
+    """Base container widget that binds navigation keys for scrolling."""
+
+    BINDINGS: ClassVar[list[BindingType]] = [
+        Binding("up", "scroll_up", "Scroll Up", show=False),
+        Binding("down", "scroll_down", "Scroll Down", show=False),
+        Binding("left", "scroll_left", "Scroll Up", show=False),
+        Binding("right", "scroll_right", "Scroll Right", show=False),
+        Binding("home", "scroll_home", "Scroll Home", show=False),
+        Binding("end", "scroll_end", "Scroll End", show=False),
+        Binding("pageup", "page_up", "Page Up", show=False),
+        Binding("pagedown", "page_down", "Page Down", show=False),
+    ]
+    """Keyboard bindings for scrollable containers.
+
+    | Key(s) | Description |
+    | :- | :- |
+    | up | Scroll up, if vertical scrolling is available. |
+    | down | Scroll down, if vertical scrolling is available. |
+    | left | Scroll left, if horizontal scrolling is available. |
+    | right | Scroll right, if horizontal scrolling is available. |
+    | home | Scroll to the home position, if scrolling is available. |
+    | end | Scroll to the end position, if scrolling is available. |
+    | pageup | Scroll up one page, if vertical scrolling is available. |
+    | pagedown | Scroll down one page, if vertical scrolling is available. |
+    """
+
+
+class Vertical(Widget, inherit_bindings=False):
     """A container which arranges children vertically."""
 
     DEFAULT_CSS = """
@@ -31,7 +63,7 @@ class Vertical(Widget):
     """
 
 
-class VerticalScroll(Widget, can_focus=True):
+class VerticalScroll(ScrollableContainer, can_focus=True):
     """A container which arranges children vertically, with an automatic vertical scrollbar."""
 
     DEFAULT_CSS = """
@@ -43,7 +75,7 @@ class VerticalScroll(Widget, can_focus=True):
     """
 
 
-class Horizontal(Widget):
+class Horizontal(Widget, inherit_bindings=False):
     """A container which arranges children horizontally."""
 
     DEFAULT_CSS = """
@@ -55,7 +87,7 @@ class Horizontal(Widget):
     """
 
 
-class HorizontalScroll(Widget, can_focus=True):
+class HorizontalScroll(ScrollableContainer, can_focus=True):
     """A container which arranges children horizontally, with an automatic horizontal scrollbar."""
 
     DEFAULT_CSS = """
@@ -67,7 +99,7 @@ class HorizontalScroll(Widget, can_focus=True):
     """
 
 
-class Center(Widget):
+class Center(Widget, inherit_bindings=False):
     """A container which centers children horizontally."""
 
     DEFAULT_CSS = """
@@ -79,7 +111,7 @@ class Center(Widget):
     """
 
 
-class Middle(Widget):
+class Middle(Widget, inherit_bindings=False):
     """A container which aligns children vertically in the middle."""
 
     DEFAULT_CSS = """
@@ -91,7 +123,7 @@ class Middle(Widget):
     """
 
 
-class Grid(Widget):
+class Grid(Widget, inherit_bindings=False):
     """A container with grid alignment."""
 
     DEFAULT_CSS = """
@@ -102,7 +134,7 @@ class Grid(Widget):
     """
 
 
-class Content(Widget, can_focus=True, can_focus_children=False):
+class Content(Widget, can_focus=True, can_focus_children=False, inherit_bindings=False):
     """A container for content such as text."""
 
     DEFAULT_CSS = """

src/textual/demo.py

@@ -14,7 +14,7 @@ from rich.text import Text
 
 from textual.app import App, ComposeResult
 from textual.binding import Binding
-from textual.containers import Container, Horizontal
+from textual.containers import Container, Horizontal, ScrollableContainer
 from textual.reactive import reactive
 from textual.widgets import (
     Button,
@@ -189,7 +189,7 @@ JSON_EXAMPLE = """{
 """
 
 
-class Body(Container):
+class Body(ScrollableContainer):
     pass
 
 

src/textual/scroll_view.py

@@ -7,13 +7,13 @@ from __future__ import annotations
 
 from rich.console import RenderableType
 
+from .containers import ScrollableContainer
 from .geometry import Size
-from .widget import Widget
 
 
-class ScrollView(Widget):
+class ScrollView(ScrollableContainer):
     """
-    A base class for a Widget that handles it's own scrolling (i.e. doesn't rely
+    A base class for a Widget that handles its own scrolling (i.e. doesn't rely
     on the compositor to render children).
 
     """

src/textual/widget.py

@@ -54,7 +54,6 @@ from ._segment_tools import align_lines
 from ._styles_cache import StylesCache
 from .actions import SkipAction
 from .await_remove import AwaitRemove
-from .binding import Binding
 from .box_model import BoxModel
 from .css.query import NoMatches, WrongType
 from .css.scalar import ScalarOffset
@@ -237,17 +236,6 @@ class Widget(DOMNode):
 
     """
 
-    BINDINGS = [
-        Binding("up", "scroll_up", "Scroll Up", show=False),
-        Binding("down", "scroll_down", "Scroll Down", show=False),
-        Binding("left", "scroll_left", "Scroll Up", show=False),
-        Binding("right", "scroll_right", "Scroll Right", show=False),
-        Binding("home", "scroll_home", "Scroll Home", show=False),
-        Binding("end", "scroll_end", "Scroll End", show=False),
-        Binding("pageup", "page_up", "Page Up", show=False),
-        Binding("pagedown", "page_down", "Page Down", show=False),
-    ]
-
     DEFAULT_CSS = """
     Widget{
         scrollbar-background: $panel-darken-1;

tests/test_binding_inheritance.py

@@ -47,9 +47,8 @@ async def test_just_app_no_bindings() -> None:
 # An application with a single alpha binding.
 #
 # Sticking with just an app and the default screen: this configuration has a
-# BINDINGS on the app itself, and simply binds the letter a -- in other
-# words avoiding anything to do with movement keys. The result should be
-# that we see the letter a, ctrl+c, and nothing else.
+# BINDINGS on the app itself, and simply binds the letter a. The result
+# should be that we see the letter a, ctrl+c, and nothing else.
 
 
 class AlphaBinding(App[None]):
@@ -114,22 +113,13 @@ class AppWithScreenThatHasABinding(App[None]):
 async def test_app_screen_with_bindings() -> None:
     """Test a screen with a single key binding defined."""
     async with AppWithScreenThatHasABinding().run_test() as pilot:
-        # The screen will contain all of the movement keys, because it
-        # inherits from Widget. That's fine. Let's check they're there, but
-        # also let's check that they all have a non-priority binding.
-        assert all(
-            pilot.app.screen._bindings.get_key(key).priority is False
-            for key in MOVEMENT_KEYS
-        )
-        # Let's also check that the 'a' key is there, and it *is* a priority
-        # binding.
         assert pilot.app.screen._bindings.get_key("a").priority is True
 
 
 ##############################################################################
 # A non-default screen with a single low-priority alpha key binding.
 #
-# As above, but because Screen sets akk keys as high priority by default, we
+# As above, but because Screen sets all keys as high priority by default, we
 # want to be sure that if we set our keys in our subclass as low priority as
 # default, they come through as such.
 
@@ -152,13 +142,7 @@ class AppWithScreenThatHasALowBinding(App[None]):
 async def test_app_screen_with_low_bindings() -> None:
     """Test a screen with a single low-priority key binding defined."""
     async with AppWithScreenThatHasALowBinding().run_test() as pilot:
-        # Screens inherit from Widget which means they get movement keys
-        # too, so let's ensure they're all in there, along with our own key,
-        # and that everyone is low-priority.
-        assert all(
-            pilot.app.screen._bindings.get_key(key).priority is False
-            for key in ["a", *MOVEMENT_KEYS]
-        )
+        assert pilot.app.screen._bindings.get_key("a").priority is False
 
 
 ##############################################################################