Consider visible children inside invisible containers when computing focus chain

CHANGELOG.md

@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed auto height container with default grid-rows https://github.com/Textualize/textual/issues/1597
 - Fixed `page_up` and `page_down` bug in `DataTable` when `show_header = False` https://github.com/Textualize/textual/pull/3093
+- Fixed issue with visible children inside invisible container when moving focus https://github.com/Textualize/textual/issues/3053
 
 ## [0.33.0] - 2023-08-15
 
@@ -36,6 +37,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fixed `SelectionList.clear_options` https://github.com/Textualize/textual/pull/3075
 - `MouseMove` events bubble up from widgets. `App` and `Screen` receive `MouseMove` events even if there's no Widget under the cursor. https://github.com/Textualize/textual/issues/2905
 
+### Changed
+
+- Breaking change: `DOMNode.visible` now takes into account full DOM to report whether a node is visible or not.
+
+### Removed
+
+- Property `Widget.focusable_children` https://github.com/Textualize/textual/pull/3070
+
 ### Added
 
 - Added an interface for replacing prompt of an individual option in an `OptionList` https://github.com/Textualize/textual/issues/2603

src/textual/dom.py

@@ -612,13 +612,21 @@ def display(self, new_val: bool | str) -> None:
 
     @property
     def visible(self) -> bool:
-        """Is the visibility style set to a visible state?
+        """Is this widget visible in the DOM?
 
-        May be set to a boolean to make the node visible (`True`) or invisible (`False`), or to any valid value for the `visibility` rule.
+        If a widget hasn't had its visibility set explicitly, then it inherits it from its
+        DOM ancestors.
 
-        When a node is invisible, Textual will reserve space for it, but won't display anything there.
+        This may be set explicitly to override inherited values.
+        The valid values include the valid values for the `visibility` rule and the booleans
+        `True` or `False`, to set the widget to be visible or invisible, respectively.
+
+        When a node is invisible, Textual will reserve space for it, but won't display anything.
         """
-        return self.styles.visibility != "hidden"
+        own_value = self.styles.get_rule("visibility")
+        if own_value is not None:
+            return own_value != "hidden"
+        return self.parent.visible if self.parent else True
 
     @visible.setter
     def visible(self, new_value: bool | str) -> None:

src/textual/screen.py

@@ -6,6 +6,7 @@
 from __future__ import annotations
 
 from functools import partial
+from operator import attrgetter
 from typing import (
     TYPE_CHECKING,
     Awaitable,
@@ -293,18 +294,42 @@ def focus_chain(self) -> list[Widget]:
 
         widgets: list[Widget] = []
         add_widget = widgets.append
-        stack: list[Iterator[Widget]] = [iter(self.focusable_children)]
-        pop = stack.pop
-        push = stack.append
+        focus_sorter = attrgetter("_focus_sort_key")
+        # We traverse the DOM and keep track of where we are at with a node stack.
+        # Additionally, we manually keep track of the visibility of the DOM
+        # instead of relying on the property `.visible` to save on DOM traversals.
+        # node_stack: list[tuple[iterator over node children, node visibility]]
+        node_stack: list[tuple[Iterator[Widget], bool]] = [
+            (
+                iter(sorted(self.displayed_children, key=focus_sorter)),
+                self.visible,
+            )
+        ]
+        pop = node_stack.pop
+        push = node_stack.append
 
-        while stack:
-            node = next(stack[-1], None)
+        while node_stack:
+            children_iterator, parent_visibility = node_stack[-1]
+            node = next(children_iterator, None)
             if node is None:
                 pop()
             else:
+                if node.disabled:
+                    continue
+                node_styles_visibility = node.styles.get_rule("visibility")
+                node_is_visible = (
+                    node_styles_visibility != "hidden"
+                    if node_styles_visibility
+                    else parent_visibility  # Inherit visibility if the style is unset.
+                )
                 if node.is_container and node.can_focus_children:
-                    push(iter(node.focusable_children))
-                if node.focusable:
+                    sorted_displayed_children = sorted(
+                        node.displayed_children, key=focus_sorter
+                    )
+                    push((iter(sorted_displayed_children), node_is_visible))
+                # Same check as `if node.focusable`, but we cached inherited visibility
+                # and we also skipped disabled nodes altogether.
+                if node_is_visible and node.can_focus:
                     add_widget(node)
 
         return widgets

src/textual/widget.py

@@ -1507,17 +1507,7 @@ def _self_or_ancestors_disabled(self) -> bool:
     @property
     def focusable(self) -> bool:
         """Can this widget currently be focused?"""
-        return self.can_focus and not self._self_or_ancestors_disabled
-
-    @property
-    def focusable_children(self) -> list[Widget]:
-        """Get the children which may be focused.
-
-        Returns:
-            List of widgets that can receive focus.
-        """
-        focusable = [child for child in self._nodes if child.display and child.visible]
-        return sorted(focusable, key=attrgetter("_focus_sort_key"))
+        return self.can_focus and self.visible and not self._self_or_ancestors_disabled
 
     @property
     def _focus_sort_key(self) -> tuple[int, int]:

tests/test_focus.py

@@ -1,8 +1,10 @@
 import pytest
 
 from textual.app import App
+from textual.containers import Container
 from textual.screen import Screen
 from textual.widget import Widget
+from textual.widgets import Button
 
 
 class Focusable(Widget, can_focus=True):
@@ -201,3 +203,109 @@ def test_focus_next_and_previous_with_str_selector_without_self(screen: Screen):
     assert screen.focus_previous(".a").id == "foo"
     assert screen.focus_previous(".a").id == "foo"
     assert screen.focus_previous(".b").id == "baz"
+
+
+async def test_focus_does_not_move_to_invisible_widgets():
+    """Make sure invisible widgets don't get focused by accident.
+
+    This is kind of a regression test for https://github.com/Textualize/textual/issues/3053,
+    but not really.
+    """
+
+    class MyApp(App):
+        CSS = "#inv { visibility: hidden; }"
+
+        def compose(self):
+            yield Button("one", id="one")
+            yield Button("two", id="inv")
+            yield Button("three", id="three")
+
+    app = MyApp()
+    async with app.run_test():
+        assert app.focused.id == "one"
+        assert app.screen.focus_next().id == "three"
+
+
+async def test_focus_moves_to_visible_widgets_inside_invisible_containers():
+    """Regression test for https://github.com/Textualize/textual/issues/3053."""
+
+    class MyApp(App):
+        CSS = """
+        #inv { visibility: hidden; }
+        #three { visibility: visible; }
+        """
+
+        def compose(self):
+            yield Button(id="one")
+            with Container(id="inv"):
+                yield Button(id="three")
+
+    app = MyApp()
+    async with app.run_test():
+        assert app.focused.id == "one"
+        assert app.screen.focus_next().id == "three"
+
+
+async def test_focus_chain_handles_inherited_visibility():
+    """Regression test for https://github.com/Textualize/textual/issues/3053
+
+    This is more or less a test for the interactions between #3053 and #3071.
+    We want to make sure that the focus chain is computed correctly when going through
+    a DOM with containers with all sorts of visibilities set.
+    """
+
+    class W(Widget):
+        can_focus = True
+
+    w1 = W(id="one")
+    c2 = Container(id="two")
+    w3 = W(id="three")
+    c4 = Container(id="four")
+    w5 = W(id="five")
+    c6 = Container(id="six")
+    w7 = W(id="seven")
+    c8 = Container(id="eight")
+    w9 = W(id="nine")
+    w10 = W(id="ten")
+    w11 = W(id="eleven")
+    w12 = W(id="twelve")
+    w13 = W(id="thirteen")
+
+    class InheritedVisibilityApp(App[None]):
+        CSS = """
+        #four, #eight, #ten {
+            visibility: visible;
+        }
+
+        #six, #thirteen {
+            visibility: hidden;
+        }
+        """
+
+        def compose(self):
+            yield w1  # visible, inherited
+            with c2:  # visible, inherited
+                yield w3  # visible, inherited
+                with c4:  # visible, set
+                    yield w5  # visible, inherited
+                    with c6:  # hidden, set
+                        yield w7  # hidden, inherited
+                        with c8:  # visible, set
+                            yield w9  # visible, inherited
+                        yield w10  # visible, set
+                    yield w11  # visible, inherited
+                yield w12  # visible, inherited
+            yield w13  # invisible, set
+
+    app = InheritedVisibilityApp()
+    async with app.run_test():
+        focus_chain = app.screen.focus_chain
+        assert focus_chain == [
+            w1,
+            w3,
+            w5,
+            w9,
+            w10,
+            w11,
+            w12,
+        ]

tests/test_visible.py

@@ -0,0 +1,78 @@
+from textual.app import App, ComposeResult
+from textual.containers import VerticalScroll
+from textual.widget import Widget
+
+
+async def test_visibility_changes() -> None:
+    """Test changing visibility via code and CSS.
+
+    See https://github.com/Textualize/textual/issues/1355 as the motivation for these tests.
+    """
+
+    class VisibleTester(App[None]):
+        """An app for testing visibility changes."""
+
+        CSS = """
+        Widget {
+            height: 1fr;
+        }
+        .hidden {
+            visibility: hidden;
+        }
+        """
+
+        def compose(self) -> ComposeResult:
+            yield VerticalScroll(
+                Widget(id="keep"), Widget(id="hide-via-code"), Widget(id="hide-via-css")
+            )
+
+    async with VisibleTester().run_test() as pilot:
+        assert pilot.app.query_one("#keep").visible is True
+        assert pilot.app.query_one("#hide-via-code").visible is True
+        assert pilot.app.query_one("#hide-via-css").visible is True
+
+        pilot.app.query_one("#hide-via-code").styles.visibility = "hidden"
+        await pilot.pause(0)
+        assert pilot.app.query_one("#keep").visible is True
+        assert pilot.app.query_one("#hide-via-code").visible is False
+        assert pilot.app.query_one("#hide-via-css").visible is True
+
+        pilot.app.query_one("#hide-via-css").set_class(True, "hidden")
+        await pilot.pause(0)
+        assert pilot.app.query_one("#keep").visible is True
+        assert pilot.app.query_one("#hide-via-code").visible is False
+        assert pilot.app.query_one("#hide-via-css").visible is False
+
+
+async def test_visible_is_inherited() -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/3071"""
+
+    class InheritedVisibilityApp(App[None]):
+        CSS = """
+        #four {
+            visibility: visible;
+        }
+
+        #six {
+            visibility: hidden;
+        }
+        """
+
+        def compose(self):
+            yield Widget(id="one")
+            with VerticalScroll(id="two"):
+                yield Widget(id="three")
+                with VerticalScroll(id="four"):
+                    yield Widget(id="five")
+                    with VerticalScroll(id="six"):
+                        yield Widget(id="seven")
+
+    app = InheritedVisibilityApp()
+    async with app.run_test():
+        assert app.query_one("#one").visible
+        assert app.query_one("#two").visible
+        assert app.query_one("#three").visible
+        assert app.query_one("#four").visible
+        assert app.query_one("#five").visible
+        assert not app.query_one("#six").visible
+        assert not app.query_one("#seven").visible