Merge pull request #482 from Textualize/bugfix-align-center-middle-not-working-correctly-with-scrollbars

[tests] Add an integration test for the vertical layout, fix the bug that messes up the layout when the children have `align: center top;`

pyproject.toml

@@ -46,6 +46,7 @@ includes = "src"
 
 [tool.pytest.ini_options]
 asyncio_mode = "auto"
+testpaths = ["tests"]
 
 [build-system]
 requires = ["poetry-core>=1.0.0"]

sandbox/color_names.py

@@ -0,0 +1,46 @@
+import rich.repr
+from rich.align import Align
+from rich.console import RenderableType
+from rich.panel import Panel
+from rich.pretty import Pretty
+
+from textual._color_constants import COLOR_NAME_TO_RGB
+from textual.app import App, ComposeResult
+from textual.widget import Widget
+from textual.widgets import Placeholder
+
+
+@rich.repr.auto(angular=False)
+class ColorDisplay(Widget, can_focus=True):
+    def render(self) -> RenderableType:
+        return Panel(
+            Align.center(
+                Pretty(self, no_wrap=True, overflow="ellipsis"), vertical="middle"
+            ),
+            title=self.name,
+            border_style="none",
+        )
+
+
+class ColorNames(App):
+    CSS = """
+    ColorDisplay {
+        height: 1;
+    }
+    """
+
+    def on_mount(self):
+        self.bind("q", "quit")
+
+    def compose(self) -> ComposeResult:
+        for color_name, color in COLOR_NAME_TO_RGB.items():
+            color_placeholder = ColorDisplay(name=color_name)
+            is_dark_color = sum(color) < 400
+            color_placeholder.styles.color = "white" if is_dark_color else "black"
+            color_placeholder.styles.background = color_name
+            yield color_placeholder
+
+
+if __name__ == "__main__":
+    color_name_app = ColorNames()
+    color_name_app.run()

sandbox/vertical_container.py

@@ -0,0 +1,94 @@
+from rich.console import RenderableType
+from rich.text import Text
+
+from textual.app import App, ComposeResult
+from textual.widget import Widget
+from textual.widgets import Placeholder
+
+root_container_style = "border: solid white;"
+initial_placeholders_count = 4
+
+
+class VerticalContainer(Widget):
+    CSS = """
+    VerticalContainer {
+        layout: vertical;
+        overflow: hidden auto;
+        background: darkblue;
+        ${root_container_style}
+    }
+
+    VerticalContainer Placeholder {
+        margin: 1 0;
+        height: 3;
+        border: solid lime;
+        align: center top;
+    }
+    """.replace(
+        "${root_container_style}", root_container_style
+    )
+
+
+class Introduction(Widget):
+    CSS = """
+    Introduction {
+        background: indigo;
+        color: white;
+        height: 3;
+        padding: 1 0;
+    }
+    """
+
+    def render(self) -> RenderableType:
+        return Text(
+            "Press '-' and '+' to add or remove placeholders.", justify="center"
+        )
+
+
+class MyTestApp(App):
+    def compose(self) -> ComposeResult:
+        # yield Introduction()
+
+        placeholders = [
+            Placeholder(id=f"placeholder_{i}", name=f"Placeholder #{i}")
+            for i in range(initial_placeholders_count)
+        ]
+
+        yield VerticalContainer(Introduction(), *placeholders, id="root")
+
+    def on_mount(self):
+        self.bind("q", "quit")
+        self.bind("t", "tree")
+        self.bind("-", "remove_placeholder")
+        self.bind("+", "add_placeholder")
+
+    def action_tree(self):
+        self.log(self.tree)
+
+    async def action_remove_placeholder(self):
+        placeholders = self.query("Placeholder")
+        placeholders_count = len(placeholders)
+        for i, placeholder in enumerate(placeholders):
+            if i == placeholders_count - 1:
+                await self.remove(placeholder)
+                placeholder.parent.children._nodes.remove(placeholder)
+        self.refresh(repaint=True, layout=True)
+        self.refresh_css()
+
+    async def action_add_placeholder(self):
+        placeholders = self.query("Placeholder")
+        placeholders_count = len(placeholders)
+        placeholder = Placeholder(
+            id=f"placeholder_{placeholders_count+1}",
+            name=f"Placeholder #{placeholders_count+1}",
+        )
+        root = self.query_one("#root")
+        root.mount(placeholder)
+        self.refresh(repaint=True, layout=True)
+        self.refresh_css()
+
+
+app = MyTestApp()
+
+if __name__ == "__main__":
+    app.run()

src/textual/_border.py

@@ -15,6 +15,7 @@ INNER = 1
 OUTER = 2
 
 BORDER_CHARS: dict[EdgeType, tuple[str, str, str]] = {
+    # TODO: in "browsers' CSS" `none` and `hidden` both set the border width to zero. Should we do the same?
     "": ("   ", "   ", "   "),
     "none": ("   ", "   ", "   "),
     "hidden": ("   ", "   ", "   "),

src/textual/_compositor.py

@@ -15,7 +15,7 @@ from __future__ import annotations
 
 from operator import attrgetter, itemgetter
 import sys
-from typing import cast, Callable, Iterator, Iterable, NamedTuple, TYPE_CHECKING
+from typing import cast, Iterator, Iterable, NamedTuple, TYPE_CHECKING
 
 import rich.repr
 from rich.console import Console, ConsoleOptions, RenderResult
@@ -23,15 +23,13 @@ from rich.control import Control
 from rich.segment import Segment, SegmentLines
 from rich.style import Style
 
-from . import errors, log
+from . import errors
 from .geometry import Region, Offset, Size
 
 
 from ._loop import loop_last
-from ._profile import timer
 from ._segment_tools import line_crop
 from ._types import Lines
-from .widget import Widget
 
 if sys.version_info >= (3, 10):
     from typing import TypeAlias
@@ -462,7 +460,6 @@ class Compositor:
         """Render a layout.
 
         Args:
-            console (Console): Console instance.
             clip (Optional[Region]): Region to clip to.
 
         Returns:

src/textual/app.py

@@ -20,6 +20,11 @@ from typing import (
     TYPE_CHECKING,
 )
 
+if sys.version_info >= (3, 8):
+    from typing import Literal
+else:
+    from typing_extensions import Literal  # pragma: no cover
+
 import rich
 import rich.repr
 from rich.console import Console, RenderableType
@@ -108,6 +113,10 @@ class App(Generic[ReturnType], DOMNode):
         driver_class: Type[Driver] | None = None,
         log_path: str | PurePath = "",
         log_verbosity: int = 1,
+        # TODO: make this Literal a proper type in Rich, so we re-use it?
+        log_color_system: Literal[
+            "auto", "standard", "256", "truecolor", "windows"
+        ] = "auto",
         title: str = "Textual Application",
         css_path: str | PurePath | None = None,
         watch_css: bool = False,
@@ -155,6 +164,7 @@ class App(Generic[ReturnType], DOMNode):
             self._log_file = open(log_path, "wt")
             self._log_console = Console(
                 file=self._log_file,
+                color_system=log_color_system,
                 markup=False,
                 emoji=False,
                 highlight=False,
@@ -494,6 +504,9 @@ class App(Generic[ReturnType], DOMNode):
 
         Returns:
             DOMNode: The first child of this node with the specified ID.
+
+        Raises:
+            NoMatchingNodesError: if no children could be found for this ID
         """
         return self.screen.get_child(id)
 

src/textual/dom.py

@@ -397,6 +397,9 @@ class DOMNode(MessagePump):
 
         Returns:
             DOMNode: The first child of this node with the ID.
+
+        Raises:
+            NoMatchingNodesError: if no children could be found for this ID
         """
         for child in self.children:
             if child.id == id:

src/textual/drivers/linux_driver.py

@@ -120,9 +120,7 @@ class LinuxDriver(Driver):
         self.console.show_cursor(False)
         self.console.file.write("\033[?1003h\n")
         self.console.file.flush()
-        self._key_thread = Thread(
-            target=self.run_input_thread, args=(asyncio.get_running_loop(),)
-        )
+        self._key_thread = Thread(target=self.run_input_thread, args=(loop,))
         send_size_event()
         self._key_thread.start()
 

src/textual/layouts/vertical.py

@@ -2,8 +2,6 @@ from __future__ import annotations
 
 from typing import cast, TYPE_CHECKING
 
-from .. import log
-
 from ..geometry import Offset, Region, Size
 from .._layout import Layout, WidgetPlacement
 
@@ -23,7 +21,7 @@ class VerticalLayout(Layout):
         placements: list[WidgetPlacement] = []
         add_placement = placements.append
 
-        y = max_width = max_height = 0
+        max_width = max_height = 0
         parent_size = parent.size
 
         box_models = [
@@ -40,14 +38,11 @@ class VerticalLayout(Layout):
 
         y = box_models[0].margin.top if box_models else 0
 
-        displayed_children = parent.displayed_children
-
+        displayed_children = cast("list[Widget]", parent.displayed_children)
         for widget, box_model, margin in zip(displayed_children, box_models, margins):
             content_width, content_height = box_model.size
-            offset_x = widget.styles.align_width(content_width, parent_size.width)
+            offset_x = widget.styles.align_width(content_width, size.width)
             region = Region(offset_x, y, content_width, content_height)
-            # TODO: it seems that `max_height` is not used?
-            max_height = max(max_height, content_height)
             add_placement(WidgetPlacement(region, widget, 0))
             y += region.height + margin
             max_height = y

src/textual/screen.py

@@ -20,8 +20,8 @@ class Screen(Widget):
     CSS = """
     
     Screen {
-        layout: dock;
-        docks: _default=top;
+        layout: vertical;
+        overflow-y: auto;
         background: $surface;
         color: $text-surface;
     }

src/textual/widget.py

@@ -28,6 +28,7 @@ from ._context import active_app
 from ._types import Lines
 from .dom import DOMNode
 from .geometry import clamp, Offset, Region, Size
+from .layouts.vertical import VerticalLayout
 from .message import Message
 from . import messages
 from ._layout import Layout
@@ -82,6 +83,7 @@ class Widget(DOMNode):
         self._virtual_size = Size(0, 0)
         self._container_size = Size(0, 0)
         self._layout_required = False
+        self._default_layout = VerticalLayout()
         self._animate: BoundAnimator | None = None
         self._reactive_watches: dict[str, Callable] = {}
         self.highlight_style: Style | None = None
@@ -553,7 +555,12 @@ class Widget(DOMNode):
 
     @property
     def layout(self) -> Layout | None:
-        return self.styles.layout
+        return self.styles.layout or (
+            # If we have children we _should_ return a layout, otherwise they won't be displayed:
+            self._default_layout
+            if self.children
+            else None
+        )
 
     @property
     def is_container(self) -> bool:

tests/test_integration_layout.py

@@ -0,0 +1,173 @@
+from __future__ import annotations
+import asyncio
+from typing import cast, List
+
+import pytest
+
+from tests.utilities.test_app import AppTest
+from textual.app import ComposeResult
+from textual.geometry import Size
+from textual.widget import Widget
+from textual.widgets import Placeholder
+
+# Let's allow ourselves some abbreviated names for those tests,
+# in order to make the test cases a bit easier to read :-)
+SCREEN_W = 100  # width of our Screens
+SCREEN_H = 8  # height of our Screens
+SCREEN_SIZE = Size(SCREEN_W, SCREEN_H)
+PLACEHOLDERS_DEFAULT_H = 3  # the default height for our Placeholder widgets
+
+
+@pytest.mark.asyncio
+@pytest.mark.integration_test  # this is a slow test, we may want to skip them in some contexts
+@pytest.mark.parametrize(
+    (
+        "screen_size",
+        "placeholders_count",
+        "root_container_style",
+        "placeholders_style",
+        "expected_root_widget_virtual_size",
+        "expected_placeholders_size",
+        "expected_placeholders_offset_x",
+    ),
+    (
+        [
+            SCREEN_SIZE,
+            1,
+            "border: ;",  # #root has no border
+            "",  # no specific placeholder style
+            # #root's virtual size=screen size
+            (SCREEN_W, SCREEN_H),
+            # placeholders width=same than screen :: height=default height
+            (SCREEN_W, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 0
+            0,
+        ],
+        [
+            # "none" borders still allocate a space for the (invisible) border
+            SCREEN_SIZE,
+            1,
+            "border: none;",  # #root has an invisible border
+            "",  # no specific placeholder style
+            # #root's virtual size is smaller because of its borders
+            (SCREEN_W - 2, SCREEN_H - 2),
+            # placeholders width=same than screen, minus 2 borders :: height=default height minus 2 borders
+            (SCREEN_W - 2, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 1 because of #root's border
+            1,
+        ],
+        [
+            SCREEN_SIZE,
+            1,
+            "border: solid white;",  # #root has a visible border
+            "",  # no specific placeholder style
+            # #root's virtual size is smaller because of its borders
+            (SCREEN_W - 2, SCREEN_H - 2),
+            # placeholders width=same than screen, minus 2 borders :: height=default height minus 2 borders
+            (SCREEN_W - 2, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 1 because of #root's border
+            1,
+        ],
+        [
+            SCREEN_SIZE,
+            4,
+            "border: solid white;",  # #root has a visible border
+            "",  # no specific placeholder style
+            # #root's virtual height should be as high as its stacked content
+            (SCREEN_W - 2 - 1, PLACEHOLDERS_DEFAULT_H * 4),
+            # placeholders width=same than screen, minus 2 borders, minus scrollbar :: height=default height minus 2 borders
+            (SCREEN_W - 2 - 1, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 1 because of #root's border
+            1,
+        ],
+        [
+            SCREEN_SIZE,
+            1,
+            "border: solid white;",  # #root has a visible border
+            "align: center top;",  # placeholders are centered horizontally
+            # #root's virtual size=screen size
+            (SCREEN_W, SCREEN_H),
+            # placeholders width=same than screen, minus 2 borders :: height=default height
+            (SCREEN_W - 2, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 1 because of #root's border
+            1,
+        ],
+        [
+            SCREEN_SIZE,
+            4,
+            "border: solid white;",  # #root has a visible border
+            "align: center top;",  # placeholders are centered horizontally
+            # #root's virtual height should be as high as its stacked content
+            (SCREEN_W - 2 - 1, PLACEHOLDERS_DEFAULT_H * 4),
+            # placeholders width=same than screen, minus 2 borders, minus scrollbar :: height=default height
+            (SCREEN_W - 2 - 1, PLACEHOLDERS_DEFAULT_H),
+            # placeholders should be at offset 1 because of #root's border
+            1,
+        ],
+    ),
+)
+async def test_composition_of_vertical_container_with_children(
+    screen_size: Size,
+    placeholders_count: int,
+    root_container_style: str,
+    placeholders_style: str,
+    expected_placeholders_size: tuple[int, int],
+    expected_root_widget_virtual_size: tuple[int, int],
+    expected_placeholders_offset_x: int,
+    event_loop: asyncio.AbstractEventLoop,
+):
+    class VerticalContainer(Widget):
+        CSS = (
+            """
+        VerticalContainer {
+            layout: vertical;
+            overflow: hidden auto;
+            ${root_container_style}
+        }
+        
+        VerticalContainer Placeholder {
+            height: ${placeholders_height};
+            ${placeholders_style}
+        }
+        """.replace(
+                "${root_container_style}", root_container_style
+            )
+            .replace("${placeholders_height}", str(PLACEHOLDERS_DEFAULT_H))
+            .replace("${placeholders_style}", placeholders_style)
+        )
+
+    class MyTestApp(AppTest):
+        def compose(self) -> ComposeResult:
+            placeholders = [
+                Placeholder(id=f"placeholder_{i}", name=f"Placeholder #{i}")
+                for i in range(placeholders_count)
+            ]
+
+            yield VerticalContainer(*placeholders, id="root")
+
+    app = MyTestApp(size=screen_size, test_name="compositor")
+
+    expected_screen_size = Size(*screen_size)
+
+    async with app.in_running_state():
+        app.log_tree()
+
+        # root widget checks:
+        root_widget = cast(Widget, app.get_child("root"))
+        assert root_widget.size == expected_screen_size
+        root_widget_region = app.screen.get_widget_region(root_widget)
+        assert root_widget_region == (
+            0,
+            0,
+            expected_screen_size.width,
+            expected_screen_size.height,
+        )
+
+        app_placeholders = cast(List[Widget], app.query("Placeholder"))
+        assert len(app_placeholders) == placeholders_count
+
+    # placeholder widgets checks:
+    for placeholder in app_placeholders:
+        assert placeholder.size == expected_placeholders_size
+        assert placeholder.styles.offset.x.value == 0.0
+        assert app.screen.get_offset(placeholder).x == expected_placeholders_offset_x

tests/utilities/test_app.py

@@ -0,0 +1,131 @@
+from __future__ import annotations
+
+import asyncio
+import contextlib
+import io
+from pathlib import Path
+from typing import AsyncContextManager
+
+from rich.console import Console, Capture
+from textual import events
+from textual.app import App, ReturnType, ComposeResult
+from textual.driver import Driver
+from textual.geometry import Size
+
+
+# N.B. These classes would better be named TestApp/TestConsole/TestDriver/etc,
+# but it makes pytest emit warning as it will try to collect them as classes containing test cases :-/
+
+
+class AppTest(App):
+    def __init__(
+        self,
+        *,
+        test_name: str,
+        size: Size,
+        log_verbosity: int = 2,
+    ):
+        # will log in "/tests/test.[test name].log":
+        log_path = Path(__file__).parent.parent / f"test.{test_name}.log"
+        super().__init__(
+            driver_class=DriverTest,
+            log_path=log_path,
+            log_verbosity=log_verbosity,
+            log_color_system="256",
+        )
+        self._size = size
+        self._console = ConsoleTest(width=size.width, height=size.height)
+        self._error_console = ConsoleTest(width=size.width, height=size.height)
+
+    def log_tree(self) -> None:
+        """Handy shortcut when testing stuff"""
+        self.log(self.tree)
+
+    def compose(self) -> ComposeResult:
+        raise NotImplementedError(
+            "Create a subclass of TestApp and override its `compose()` method, rather than using TestApp directly"
+        )
+
+    def in_running_state(
+        self,
+        *,
+        initialisation_timeout: float = 0.1,
+    ) -> AsyncContextManager[Capture]:
+        async def run_app() -> None:
+            await self.process_messages()
+
+        @contextlib.asynccontextmanager
+        async def get_running_state_context_manager():
+            run_task = asyncio.create_task(run_app())
+            timeout_before_yielding_task = asyncio.create_task(
+                asyncio.sleep(initialisation_timeout)
+            )
+            done, pending = await asyncio.wait(
+                (
+                    run_task,
+                    timeout_before_yielding_task,
+                ),
+                return_when=asyncio.FIRST_COMPLETED,
+            )
+            if run_task in done or run_task not in pending:
+                raise RuntimeError(
+                    "TestApp is no longer return after its initialization period"
+                )
+            with self.console.capture() as capture:
+                yield capture
+            assert not run_task.done()
+            await self.shutdown()
+
+        return get_running_state_context_manager()
+
+    def run(self):
+        raise NotImplementedError(
+            "Use `async with my_test_app.in_running_state()` rather than `my_test_app.run()`"
+        )
+
+    @property
+    def console(self) -> ConsoleTest:
+        return self._console
+
+    @console.setter
+    def console(self, console: Console) -> None:
+        """This is a no-op, the console is always a TestConsole"""
+        return
+
+    @property
+    def error_console(self) -> ConsoleTest:
+        return self._error_console
+
+    @error_console.setter
+    def error_console(self, console: Console) -> None:
+        """This is a no-op, the error console is always a TestConsole"""
+        return
+
+
+class ConsoleTest(Console):
+    def __init__(self, *, width: int, height: int):
+        file = io.StringIO()
+        super().__init__(
+            color_system="256",
+            file=file,
+            width=width,
+            height=height,
+            force_terminal=True,
+            legacy_windows=False,
+        )
+
+
+class DriverTest(Driver):
+    def start_application_mode(self) -> None:
+        size = Size(self.console.size.width, self.console.size.height)
+        event = events.Resize(self._target, size, size)
+        asyncio.run_coroutine_threadsafe(
+            self._target.post_message(event),
+            loop=asyncio.get_running_loop(),
+        )
+
+    def disable_input(self) -> None:
+        pass
+
+    def stop_application_mode(self) -> None:
+        pass