Repository: Textualize/textual
Pull request: Focus order
Updates focus order to work in left to right then top to bottom order.
Anchor point is top left corner of the widget region **including** the margin spacing of the widget.

Problem statement:
Better focus order
We have a fairly naive approach to the focus order in Textual.

Textual will go through widgets in the order they are defined within `children`.

What should probably occur is that the children are scanned in the order they appear on screen, i.e. left to right, then top to bottom.

We should probably also implement a switch to got from right to left for future rtl language support.

This can be implemented in `App.focus_chain`. I suspect we need to sort the `focusable_children` attribute in that property.

Hints from issue discussion:
I would be happy to help with this.

Commit message: Improve order of focus

Patch to review:
diff --git a/sandbox/darren/just_a_box.css b/sandbox/darren/just_a_box.css
index 765fd56517..6f4806f828 100644
--- a/sandbox/darren/just_a_box.css
+++ b/sandbox/darren/just_a_box.css
@@ -1,6 +1,61 @@
-#box {
-    height: 50%;
-    width: 50%;
-    align: center middle;
+Screen {
+    height: 100vh;
+    width: 100%;
+    background: red;
+}
+
+#horizontal {
+    width: 100%;
+}
+
+.box {
+    height: 5;
+    width: 5;
+    margin: 1 10;
+}
+
+#left_pane {
+    width: 1fr;
+    background: $background;
+}
+
+#middle_pane {
+    margin-top: 4;
+    width: 1fr;
+    background: #173f5f;
+}
+
+#middle_pane:focus {
+    outline: #8bc34a;
+}
+
+#right_pane {
+    width: 1fr;
+    background: #f6d55c;
+}
+
+.box:focus {
+    outline: #8bc34a;
+}
+
+#box1 {
     background: green;
 }
+
+#box2 {
+    offset-y: 3;
+    background: hotpink;
+}
+
+#box3 {
+    background: red;
+}
+
+
+#box4 {
+    background: blue;
+}
+
+#box5 {
+    background: darkviolet;
+}
diff --git a/sandbox/darren/just_a_box.py b/sandbox/darren/just_a_box.py
index 0750afc43e..8e6fc3ae79 100644
--- a/sandbox/darren/just_a_box.py
+++ b/sandbox/darren/just_a_box.py
@@ -4,10 +4,11 @@
 from rich.panel import Panel
 
 from textual.app import App, ComposeResult
+from textual.layout import Horizontal, Vertical
 from textual.widget import Widget
 
 
-class Box(Widget):
+class Box(Widget, can_focus=True):
     CSS = "#box {background: blue;}"
 
     def __init__(
@@ -20,8 +21,25 @@ def render(self) -> RenderableType:
 
 
 class JustABox(App):
+    dark = True
+
     def compose(self) -> ComposeResult:
-        yield Box(id="box")
+        yield Horizontal(
+            Vertical(
+                Box(id="box1", classes="box"),
+                Box(id="box2", classes="box"),
+                Box(id="box3", classes="box"),
+                id="left_pane",
+            ),
+            Box(id="middle_pane"),
+            Vertical(
+                Box(id="box", classes="box"),
+                Box(id="box4", classes="box"),
+                Box(id="box5", classes="box"),
+                id="right_pane",
+            ),
+            id="horizontal",
+        )
 
 
 if __name__ == "__main__":
diff --git a/src/textual/dom.py b/src/textual/dom.py
index ccccd19e28..c2e514bf51 100644
--- a/src/textual/dom.py
+++ b/src/textual/dom.py
@@ -26,6 +26,7 @@
     from .css.styles import StylesBase
     from .css.query import DOMQuery
     from .screen import Screen
+    from .widget import Widget
 
 
 class NoParent(Exception):
@@ -428,8 +429,11 @@ def displayed_children(self) -> list[DOMNode]:
     @property
     def focusable_children(self) -> list[DOMNode]:
         """Get the children which may be focused."""
-        # TODO: This may be the place to define order, other focus related rules
-        return [child for child in self.children if child.display and child.visible]
+        focusable = [
+            child for child in self.children if child.display and child.visible
+        ]
+        sorted_focusable = sorted(focusable, key=_focus_sort_key)
+        return sorted_focusable
 
     def get_pseudo_classes(self) -> Iterable[str]:
         """Get any pseudo classes applicable to this Node, e.g. hover, focus.
@@ -596,3 +600,8 @@ def has_pseudo_class(self, *class_names: str) -> bool:
 
     def refresh(self, *, repaint: bool = True, layout: bool = False) -> None:
         pass
+
+
+def _focus_sort_key(widget: Widget) -> tuple[int, int]:
+    x, y, _, _ = widget.region_with_margin
+    return y, x
diff --git a/src/textual/widget.py b/src/textual/widget.py
index 2555f1121d..5ab59525a3 100644
--- a/src/textual/widget.py
+++ b/src/textual/widget.py
@@ -494,6 +494,16 @@ def window_region(self) -> Region:
         window_region = self.region.at_offset(self.scroll_offset)
         return window_region
 
+    @property
+    def region_with_margin(self) -> Region:
+        """The widget region relative to its container (*including margin*), which may not be visible,
+        depending on the scroll offset.
+
+        Returns:
+            Region: The virtual region of the Widget, inclusive of its margin.
+        """
+        return self.virtual_region.grow(self.styles.margin)
+
     @property
     def scroll_offset(self) -> Offset:
         return Offset(int(self.scroll_x), int(self.scroll_y))
@@ -736,7 +746,7 @@ def scroll_to_widget(self, widget: Widget, *, animate: bool = True) -> bool:
         """
 
         # Grow the region by the margin so to keep the margin in view.
-        region = widget.virtual_region.grow(widget.styles.margin)
+        region = widget.region_with_margin
         scrolled = False
 
         while isinstance(widget.parent, Widget) and widget is not self:


Code review comments:
diff_hunk: @@ -596,3 +600,8 @@ def has_pseudo_class(self, *class_names: str) -> bool:
 
     def refresh(self, *, repaint: bool = True, layout: bool = False) -> None:
         pass
+
+
+def _focus_sort_key(widget: Widget) -> tuple[int, int]:
path: src/textual/dom.py
text: @user1:
I have a feeling we can optimize this with a combination of `attrgetter` and a new property.

It's not crazy to imagine a layout with 1000s of widgets.

I'm thinking that widget could grow a `_focus_sort_key` attribute. which return a tuple of (y, x), and we can get that with `attrgetter`


@author:
I've made this change, and also moved `focusable_children` down to `Widget`. It's only used in the context of `Widget` types elsewhere, and in calculating it requires knowing the `virtual_region`, which doesn't exist at the `DOMNode` level.

diff_hunk: @@ -494,6 +494,16 @@ def window_region(self) -> Region:
         window_region = self.region.at_offset(self.scroll_offset)
         return window_region
 
+    @property
+    def region_with_margin(self) -> Region:
path: src/textual/widget.py
text: @user1:
Maybe this should be named `virtual_region_with_margin`. I've used the undecorated term "region" to mean the region relative to the screen.

@author:
Updated