Merge pull request #3872 from Textualize/fix-keyboard-navigation-radi…

…o-buttons

Ignore disabled radio buttons when moving selection.

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
+
+### Fixed
+
+- Disabled radio buttons could be selected with the keyboard https://github.com/Textualize/textual/issues/3839
+
 
 ## [0.45.1] - 2023-12-12
 

src/textual/widgets/_radio_set.py

@@ -2,7 +2,8 @@
 
 from __future__ import annotations
 
-from typing import ClassVar, Optional
+from contextlib import suppress
+from typing import ClassVar, Literal, Optional
 
 import rich.repr
 
@@ -151,9 +152,8 @@ def __init__(
     def _on_mount(self, _: Mount) -> None:
         """Perform some processing once mounted in the DOM."""
 
-        # If there are radio buttons, select the first one.
-        if self._nodes:
-            self._selected = 0
+        # If there are radio buttons, select the first available one.
+        self.action_next_button()
 
         # Get all the buttons within us; we'll be doing a couple of things
         # with that list.
@@ -248,24 +248,58 @@ def action_previous_button(self) -> None:
 
         Note that this will wrap around to the end if at the start.
         """
-        if self._nodes:
-            if self._selected == 0:
-                self._selected = len(self.children) - 1
-            elif self._selected is None:
-                self._selected = 0
-            else:
-                self._selected -= 1
+        self._move_selected_button(-1)
 
     def action_next_button(self) -> None:
         """Navigate to the next button in the set.
 
         Note that this will wrap around to the start if at the end.
         """
-        if self._nodes:
-            if self._selected is None or self._selected == len(self._nodes) - 1:
-                self._selected = 0
-            else:
-                self._selected += 1
+        self._move_selected_button(1)
+
+    def _move_selected_button(self, direction: Literal[-1, 1]) -> None:
+        """Move the selected button to the next or previous one.
+
+        Note that this will wrap around the start/end of the button list.
+
+        We compute the available buttons by ignoring the disabled ones and then
+        we induce an ordering by computing the distance to the currently selected one if
+        we start at the selected button and then start moving in the direction indicated.
+
+        For example, if the direction is `1` and self._selected is 2, we have this:
+        selected:     v
+        buttons:  X X X X X X X
+        indices:  0 1 2 3 4 5 6
+        distance: 5 6 0 1 2 3 4
+
+        Args:
+            direction: `1` to move to the next button and `-1` for the previous.
+        """
+
+        candidate_indices = (
+            index
+            for index, button in enumerate(self.children)
+            if not button.disabled and index != self._selected
+        )
+
+        if self._selected is None:
+            with suppress(StopIteration):
+                self._selected = next(candidate_indices)
+        else:
+            selected = self._selected
+
+            def distance(index: int) -> int:
+                """Induce a distance between the given index and the selected button.
+
+                Args:
+                    index: The index of the button to consider.
+
+                Returns:
+                    The distance between the two buttons.
+                """
+                return direction * (index - selected) % len(self.children)
+
+            self._selected = min(candidate_indices, key=distance, default=None)
 
     def action_toggle(self) -> None:
         """Toggle the state of the currently-selected button."""

tests/toggles/test_radioset.py

@@ -135,3 +135,49 @@ async def test_there_can_only_be_one():
     async with BadRadioSetApp().run_test() as pilot:
         assert len(pilot.app.query("RadioButton.-on")) == 1
         assert pilot.app.query_one(RadioSet).pressed_index == 0
+
+
+class RadioSetDisabledButtonsApp(App[None]):
+    def compose(self) -> ComposeResult:
+        self.selected = []
+        with RadioSet():
+            yield RadioButton("0", disabled=True)
+            yield RadioButton("1")
+            yield RadioButton("2", disabled=True)
+            yield RadioButton("3", disabled=True)
+            yield RadioButton("4")
+            yield RadioButton("5")
+            yield RadioButton("6", disabled=True)
+            yield RadioButton("7")
+            yield RadioButton("8", disabled=True)
+
+    def on_radio_set_changed(self, radio_set: RadioSet.Changed) -> None:
+        self.selected.append(str(radio_set.pressed.label))
+
+
+async def test_keyboard_navigation_with_disabled_buttons():
+    """Regression test for https://github.com/Textualize/textual/issues/3839."""
+
+    app = RadioSetDisabledButtonsApp()
+    async with app.run_test() as pilot:
+        await pilot.press("enter")
+        for _ in range(5):
+            await pilot.press("down")
+            await pilot.press("enter")
+        for _ in range(5):
+            await pilot.press("up")
+            await pilot.press("enter")
+
+    assert app.selected == [
+        "1",
+        "4",
+        "5",
+        "7",
+        "1",
+        "4",
+        "1",
+        "7",
+        "5",
+        "4",
+        "1",
+    ]