Merge pull request #3912 from Textualize/navigation

Fix keyboard navigation in option list (& selection list), radio set, list view

Author: rodrigogiraoserrao

CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### Changed
+
+- Breaking change: keyboard navigation in `RadioSet`, `ListView`, `OptionList`, and `SelectionList`, no longer allows highlighting disabled items https://github.com/Textualize/textual/issues/3881
+
 ## [0.48.1] - 2023-02-01
 
 ### Fixed

src/textual/_widget_navigation.py

@@ -0,0 +1,198 @@
+"""
+Utilities to move index-based selections backward/forward.
+
+These utilities concern themselves with selections where not all options are available,
+otherwise it would be enough to increment/decrement the index and use the operator `%`
+to implement wrapping.
+"""
+
+from __future__ import annotations
+
+from functools import partial
+from itertools import count
+from typing import TYPE_CHECKING, Literal, Protocol, Sequence
+
+from typing_extensions import TypeAlias, TypeVar
+
+if TYPE_CHECKING:
+    from .widget import Widget
+
+
+class Disableable(Protocol):
+    """Non-widgets that have an enabled/disabled status."""
+
+    disabled: bool
+
+
+Direction: TypeAlias = Literal[-1, 1]
+"""Valid values to determine navigation direction.
+
+In a vertical setting, 1 points down and -1 points up.
+In a horizontal setting, 1 points right and -1 points left.
+"""
+_DisableableType = TypeVar("_DisableableType", bound=Disableable)
+_WidgetType = TypeVar("_WidgetType", bound="Widget")
+
+
+def get_directed_distance(
+    index: int, start: int, direction: Direction, wrap_at: int
+) -> int:
+    """Computes the distance going from `start` to `index` in the given direction.
+
+    Starting at `start`, this is the number of steps you need to take in the given
+    `direction` to reach `index`, assuming there is wrapping at 0 and `wrap_at`.
+    This is also the smallest non-negative integer solution `d` to
+    `(start + d * direction) % wrap_at == index`.
+
+    The diagram below illustrates the computation of `d1 = distance(2, 8, 1, 10)` and
+    `d2 = distance(2, 8, -1, 10)`:
+
+    ```
+    start ────────────────────┐
+    index ────────┐           │
+    indices   0 1 2 3 4 5 6 7 8 9
+    d1        2 3 4           0 1
+              > > >           > > (direction == 1)
+    d2            6 5 4 3 2 1 0
+                  < < < < < < <   (direction == -1)
+    ```
+
+    Args:
+        index: The index that we want to reach.
+        start: The starting point to consider when computing the distance.
+        direction: The direction in which we want to compute the distance.
+        wrap_at: Controls at what point wrapping around takes place.
+
+    Returns:
+        The computed distance.
+    """
+    return direction * (index - start) % wrap_at
+
+
+def find_first_enabled(
+    candidates: Sequence[_DisableableType] | Sequence[_WidgetType],
+) -> int | None:
+    """Find the first enabled candidate in a sequence of possibly-disabled objects.
+
+    Args:
+        candidates: The sequence of candidates to consider.
+
+    Returns:
+        The first enabled candidate or `None` if none were available.
+    """
+    return next(
+        (index for index, candidate in enumerate(candidates) if not candidate.disabled),
+        None,
+    )
+
+
+def find_last_enabled(
+    candidates: Sequence[_DisableableType] | Sequence[_WidgetType],
+) -> int | None:
+    """Find the last enabled candidate in a sequence of possibly-disabled objects.
+
+    Args:
+        candidates: The sequence of candidates to consider.
+
+    Returns:
+        The last enabled candidate or `None` if none were available.
+    """
+    total_candidates = len(candidates)
+    return next(
+        (
+            total_candidates - offset_from_end
+            for offset_from_end, candidate in enumerate(reversed(candidates), start=1)
+            if not candidate.disabled
+        ),
+        None,
+    )
+
+
+def find_next_enabled(
+    candidates: Sequence[_DisableableType] | Sequence[_WidgetType],
+    anchor: int | None,
+    direction: Direction,
+    with_anchor: bool = False,
+) -> int | None:
+    """Find the next enabled object if we're currently at the given anchor.
+
+    The definition of "next" depends on the given direction and this function will wrap
+    around the ends of the sequence of object candidates.
+
+    Args:
+        candidates: The sequence of object candidates to consider.
+        anchor: The point of the sequence from which we'll start looking for the next
+            enabled object.
+        direction: The direction in which to traverse the candidates when looking for
+            the next enabled candidate.
+        with_anchor: Consider the anchor position as the first valid position instead of
+            the last one.
+
+    Returns:
+        The next enabled object. If none are available, return the anchor.
+    """
+
+    if anchor is None:
+        if candidates:
+            return (
+                find_first_enabled(candidates)
+                if direction == 1
+                else find_last_enabled(candidates)
+            )
+        return None
+
+    start = anchor + direction if not with_anchor else anchor
+    key_function = partial(
+        get_directed_distance,
+        start=start,
+        direction=direction,
+        wrap_at=len(candidates),
+    )
+    enabled_candidates = [
+        index for index, candidate in enumerate(candidates) if not candidate.disabled
+    ]
+    return min(enabled_candidates, key=key_function, default=anchor)
+
+
+def find_next_enabled_no_wrap(
+    candidates: Sequence[_DisableableType] | Sequence[_WidgetType],
+    anchor: int | None,
+    direction: Direction,
+    with_anchor: bool = False,
+) -> int | None:
+    """Find the next enabled object starting from the given anchor (without wrapping).
+
+    The meaning of "next" and "past" depend on the direction specified.
+
+    Args:
+        candidates: The sequence of object candidates to consider.
+        anchor: The point of the sequence from which we'll start looking for the next
+            enabled object.
+        direction: The direction in which to traverse the candidates when looking for
+            the next enabled candidate.
+        with_anchor: Whether to consider the anchor or not.
+
+    Returns:
+        The next enabled object. If none are available, return None.
+    """
+
+    if anchor is None:
+        if candidates:
+            return (
+                find_first_enabled(candidates)
+                if direction == 1
+                else find_last_enabled(candidates)
+            )
+        return None
+
+    start = anchor if with_anchor else anchor + direction
+    counter = count(start, direction)
+    valid_candidates = (
+        candidates[start:] if direction == 1 else reversed(candidates[: start + 1])
+    )
+
+    for idx, candidate in zip(counter, valid_candidates):
+        if candidate.disabled:
+            continue
+        return idx
+    return None

src/textual/types.py

@@ -12,6 +12,7 @@
     UnusedParameter,
     WatchCallbackType,
 )
+from ._widget_navigation import Direction
 from .actions import ActionParseResult
 from .css.styles import RenderStyles
 from .widgets._directory_tree import DirEntry
@@ -32,6 +33,7 @@
     "CSSPathError",
     "CSSPathType",
     "DirEntry",
+    "Direction",
     "DuplicateID",
     "EasingFunction",
     "IgnoreReturnCallbackType",

src/textual/widgets/_list_item.py

@@ -25,6 +25,9 @@ class ListItem(Widget, can_focus=False):
         background: $panel-lighten-1;
         overflow: hidden hidden;
     }
+    ListItem > :disabled {
+        background: $panel-darken-1;
+    }
     ListItem > Widget :hover {
         background: $boost;
     }

src/textual/widgets/_list_view.py

@@ -4,15 +4,15 @@
 
 from typing_extensions import TypeGuard
 
-from textual.await_remove import AwaitRemove
-from textual.binding import Binding, BindingType
-from textual.containers import VerticalScroll
-from textual.events import Mount
-from textual.geometry import clamp
-from textual.message import Message
-from textual.reactive import reactive
-from textual.widget import AwaitMount, Widget
-from textual.widgets._list_item import ListItem
+from .. import _widget_navigation
+from ..await_remove import AwaitRemove
+from ..binding import Binding, BindingType
+from ..containers import VerticalScroll
+from ..events import Mount
+from ..message import Message
+from ..reactive import reactive
+from ..widget import AwaitMount
+from ..widgets._list_item import ListItem
 
 
 class ListView(VerticalScroll, can_focus=True, can_focus_children=False):
@@ -38,7 +38,7 @@ class ListView(VerticalScroll, can_focus=True, can_focus_children=False):
     | down | Move the cursor down. |
     """
 
-    index = reactive[Optional[int]](0, always_update=True)
+    index = reactive[Optional[int]](0, always_update=True, init=False)
     """The index of the currently highlighted item."""
 
     class Highlighted(Message):
@@ -117,7 +117,12 @@ def __init__(
         super().__init__(
             *children, name=name, id=id, classes=classes, disabled=disabled
         )
-        self._index = initial_index
+        # Set the index to the given initial index, or the first available index after.
+        self._index = _widget_navigation.find_next_enabled(
+            self._nodes,
+            anchor=initial_index - 1 if initial_index is not None else None,
+            direction=1,
+        )
 
     def _on_mount(self, _: Mount) -> None:
         """Ensure the ListView is fully-settled after mounting."""
@@ -142,17 +147,17 @@ def validate_index(self, index: int | None) -> int | None:
         Returns:
             The clamped index.
         """
-        if not self._nodes or index is None:
+        if index is None or not self._nodes:
             return None
-        return self._clamp_index(index)
+        elif index < 0:
+            return 0
+        elif index >= len(self._nodes):
+            return len(self._nodes) - 1
 
-    def _clamp_index(self, index: int) -> int:
-        """Clamp the index to a valid value given the current list of children"""
-        last_index = max(len(self._nodes) - 1, 0)
-        return clamp(index, 0, last_index)
+        return index
 
     def _is_valid_index(self, index: int | None) -> TypeGuard[int]:
-        """Return True if the current index is valid given the current list of children"""
+        """Determine whether the current index is valid into the list of children."""
         if index is None:
             return False
         return 0 <= index < len(self._nodes)
@@ -164,16 +169,14 @@ def watch_index(self, old_index: int | None, new_index: int | None) -> None:
             assert isinstance(old_child, ListItem)
             old_child.highlighted = False
 
-        new_child: Widget | None
-        if self._is_valid_index(new_index):
+        if self._is_valid_index(new_index) and not self._nodes[new_index].disabled:
             new_child = self._nodes[new_index]
             assert isinstance(new_child, ListItem)
             new_child.highlighted = True
+            self._scroll_highlighted_region()
+            self.post_message(self.Highlighted(self, new_child))
         else:
-            new_child = None
-
-        self._scroll_highlighted_region()
-        self.post_message(self.Highlighted(self, new_child))
+            self.post_message(self.Highlighted(self, None))
 
     def extend(self, items: Iterable[ListItem]) -> AwaitMount:
         """Append multiple new ListItems to the end of the ListView.
@@ -222,19 +225,30 @@ def action_select_cursor(self) -> None:
 
     def action_cursor_down(self) -> None:
         """Highlight the next item in the list."""
-        if self.index is None:
-            self.index = 0
-            return
-        self.index += 1
+        candidate = _widget_navigation.find_next_enabled(
+            self._nodes,
+            anchor=self.index,
+            direction=1,
+        )
+        if self.index is not None and candidate is not None and candidate < self.index:
+            return  # Avoid wrapping around.
+
+        self.index = candidate
 
     def action_cursor_up(self) -> None:
         """Highlight the previous item in the list."""
-        if self.index is None:
-            self.index = 0
-            return
-        self.index -= 1
+        candidate = _widget_navigation.find_next_enabled(
+            self._nodes,
+            anchor=self.index,
+            direction=-1,
+        )
+        if self.index is not None and candidate is not None and candidate > self.index:
+            return  # Avoid wrapping around.
+
+        self.index = candidate
 
     def _on_list_item__child_clicked(self, event: ListItem._ChildClicked) -> None:
+        event.stop()
         self.focus()
         self.index = self._nodes.index(event.item)
         self.post_message(self.Selected(self, event.item))
@@ -244,5 +258,6 @@ def _scroll_highlighted_region(self) -> None:
         if self.highlighted_child is not None:
             self.scroll_to_widget(self.highlighted_child, animate=False)
 
-    def __len__(self):
+    def __len__(self) -> int:
+        """Compute the length (in number of items) of the list view."""
         return len(self._nodes)