Repository: Textualize/textual
Pull request: Pilot.click/hover methods restricted to the visible area
This fixes #3349.

The two methods now raise an `OutOfBounds` error if the click/hover offset target is outside of the visible screen and it returns a Boolean that determines whether the click/hover hit the correct widget or not.

Problem statement:
Restrict Pilot.click to visible area
I suspect that `Pilot.click` will allow clicking of widgets that may not be in the visible area of the screen.

Since we are simulating a real user operating the mouse, we should make that not work. Probably by throwing an exception.

It also looks like it is possible to supply an `offset` that would click outside of the visible area. This should probably also result in an exception.

Hints from issue discussion:
(Not sure why it shows my face in the first comment; that was written by Will.)
`Pilot.click` already has a note saying that the click _may_ not land on the widget if it is not visible or covered by other widget(s).
Same thing for `Pilot.hover`.

The app below shows an example of a click that doesn't land on the widget we want because the widget is scrolled out of view.

I think it makes sense to do something if the user tries to click/hover something outside of the screen.
Either raise an error or return a Boolean indicating whether the thing we tried to click/hover was on the screen.

<details>
<summary>Code showing a click that doesn't land on the target.</summary>

```py
import asyncio

from textual.app import App
from textual.widgets import Button, Label


class MyApp(App):
    clicked = False

    def compose(self):
        for idx in range(200):
            yield Label(str(idx))
        yield Button()

    def on_button_pressed(self):
        self.clicked = True


async def main():
    app = MyApp()
    async with app.run_test(size=(80, 24)) as pilot:
        await pilot.click(Button, offset=(1, 1))

    assert not app.clicked


if __name__ == "__main__":
    asyncio.run(main())
```

</details>
Regarding the methods `Pilot.click` and `Pilot.hover`, I propose that we raise an exception `OutOfBounds` if the widget that's selected is outside the visible screen or if the offset moves the click outside of the selected widget/screen.

Furthermore, I propose that both methods return a Boolean value indicating whether the action made it to the intended widget.

Commit message: Speed up tests a bit.

Turning off the animation will make the scrolling slightly snappier.

Patch to review:
diff --git a/src/textual/pilot.py b/src/textual/pilot.py
index c3c64d2e9a..75949343ac 100644
--- a/src/textual/pilot.py
+++ b/src/textual/pilot.py
@@ -16,6 +16,7 @@
 from ._wait import wait_for_idle
 from .app import App, ReturnType
 from .events import Click, MouseDown, MouseMove, MouseUp
+from .geometry import Offset
 from .widget import Widget
 
 
@@ -44,6 +45,10 @@ def _get_mouse_message_arguments(
     return message_arguments
 
 
+class OutOfBounds(Exception):
+    """Raised when the pilot mouse target is outside of the target widget or screen."""
+
+
 class WaitForScreenTimeout(Exception):
     """Exception raised if messages aren't being processed quickly enough.
 
@@ -83,7 +88,7 @@ async def click(
         shift: bool = False,
         meta: bool = False,
         control: bool = False,
-    ) -> None:
+    ) -> bool:
         """Simulate clicking with the mouse.
 
         Args:
@@ -96,6 +101,9 @@ async def click(
             shift: Click with the shift key held down.
             meta: Click with the meta key held down.
             control: Click with the control key held down.
+
+        Returns:
+            True if the click lands on the target, False otherwise.
         """
         app = self.app
         screen = app.screen
@@ -104,21 +112,41 @@ async def click(
         else:
             target_widget = screen
 
+        if not target_widget.size.contains(*offset):
+            raise OutOfBounds(
+                f"Target size is {target_widget.size}, click offset is {offset}."
+            )
+
         message_arguments = _get_mouse_message_arguments(
             target_widget, offset, button=1, shift=shift, meta=meta, control=control
         )
+
+        click_offset = Offset(message_arguments["x"], message_arguments["y"])
+        visible_screen_region = screen.region + screen.scroll_offset
+        if not visible_screen_region.contains(*click_offset):
+            raise OutOfBounds(
+                "Target offset is outside of currently-visible"
+                + f"screen region {visible_screen_region}."
+            )
+
+        # Figure out the widget under the click before we click because the app
+        # might react to the click and move things.
+        widget_at, _ = app.get_widget_at(*click_offset)
+
         app.post_message(MouseDown(**message_arguments))
-        await self.pause(0.1)
+        await self.pause()
         app.post_message(MouseUp(**message_arguments))
-        await self.pause(0.1)
+        await self.pause()
         app.post_message(Click(**message_arguments))
-        await self.pause(0.1)
+        await self.pause()
+
+        return selector is None or widget_at is target_widget
 
     async def hover(
         self,
         selector: type[Widget] | str | None | None = None,
         offset: tuple[int, int] = (0, 0),
-    ) -> None:
+    ) -> bool:
         """Simulate hovering with the mouse cursor.
 
         Args:
@@ -128,6 +156,9 @@ async def hover(
                 currently hidden or obscured by another widget, then the hover may
                 not land on it.
             offset: The offset to hover over within the selected widget.
+
+        Returns:
+            True if the hover lands on the target, False otherwise.
         """
         app = self.app
         screen = app.screen
@@ -136,13 +167,30 @@ async def hover(
         else:
             target_widget = screen
 
+        if not target_widget.size.contains(*offset):
+            raise OutOfBounds(
+                f"Target size is {target_widget.size}, click offset is {offset}."
+            )
+
         message_arguments = _get_mouse_message_arguments(
             target_widget, offset, button=0
         )
+
+        click_offset = Offset(message_arguments["x"], message_arguments["y"])
+        visible_screen_region = screen.region + screen.scroll_offset
+        if not visible_screen_region.contains(*click_offset):
+            raise OutOfBounds(
+                "Target offset is outside of currently-visible"
+                + f"screen region {visible_screen_region}."
+            )
+
         await self.pause()
         app.post_message(MouseMove(**message_arguments))
         await self.pause()
 
+        widget_at, _ = app.get_widget_at(*click_offset)
+        return selector is None or widget_at is target_widget
+
     async def _wait_for_screen(self, timeout: float = 30.0) -> bool:
         """Wait for the current screen and its children to have processed all pending events.
 
diff --git a/tests/test_pilot.py b/tests/test_pilot.py
index d631146c77..d8ffe252af 100644
--- a/tests/test_pilot.py
+++ b/tests/test_pilot.py
@@ -5,12 +5,43 @@
 from textual import events
 from textual.app import App, ComposeResult
 from textual.binding import Binding
-from textual.widgets import Label
+from textual.containers import Center, Middle
+from textual.pilot import OutOfBounds
+from textual.widgets import Button, Label
 
 KEY_CHARACTERS_TO_TEST = "akTW03" + punctuation
 """Test some "simple" characters (letters + digits) and all punctuation."""
 
 
+class CenteredButtonApp(App):
+    CSS = """  # Ensure the button is 16 x 3
+    Button {
+        min-width: 16;
+        max-width: 16;
+        width: 16;
+        min-height: 3;
+        max-height: 3;
+        height: 3;
+    }
+    """
+
+    def compose(self):
+        with Center():
+            with Middle():
+                yield Button()
+
+
+class ManyLabelsApp(App):
+    """Auxiliary app with a button following many labels."""
+
+    AUTO_FOCUS = None  # So that there's no auto-scrolling.
+
+    def compose(self):
+        for idx in range(100):
+            yield Label(f"label {idx}", id=f"label{idx}")
+        yield Button()
+
+
 async def test_pilot_press_ascii_chars():
     """Test that the pilot can press most ASCII characters as keys."""
     keys_pressed = []
@@ -52,3 +83,209 @@ def action_beep(self) -> None:
     with pytest.raises(ZeroDivisionError):
         async with FailingApp().run_test() as pilot:
             await pilot.press("b")
+
+
+@pytest.mark.parametrize(
+    ["method", "screen_size", "offset"],
+    [
+        #
+        ("click", (80, 24), (100, 12)),  # Right of screen.
+        ("click", (80, 24), (100, 36)),  # Bottom-right of screen.
+        ("click", (80, 24), (50, 36)),  # Under screen.
+        ("click", (80, 24), (-10, 36)),  # Bottom-left of screen.
+        ("click", (80, 24), (-10, 12)),  # Left of screen.
+        ("click", (80, 24), (-10, -2)),  # Top-left of screen.
+        ("click", (80, 24), (50, -2)),  # Above screen.
+        ("click", (80, 24), (100, -2)),  # Top-right of screen.
+        #
+        ("click", (5, 5), (7, 3)),  # Right of screen.
+        ("click", (5, 5), (7, 7)),  # Bottom-right of screen.
+        ("click", (5, 5), (3, 7)),  # Under screen.
+        ("click", (5, 5), (-1, 7)),  # Bottom-left of screen.
+        ("click", (5, 5), (-1, 3)),  # Left of screen.
+        ("click", (5, 5), (-1, -1)),  # Top-left of screen.
+        ("click", (5, 5), (3, -1)),  # Above screen.
+        ("click", (5, 5), (7, -1)),  # Top-right of screen.
+        #
+        ("hover", (80, 24), (100, 12)),  # Right of screen.
+        ("hover", (80, 24), (100, 36)),  # Bottom-right of screen.
+        ("hover", (80, 24), (50, 36)),  # Under screen.
+        ("hover", (80, 24), (-10, 36)),  # Bottom-left of screen.
+        ("hover", (80, 24), (-10, 12)),  # Left of screen.
+        ("hover", (80, 24), (-10, -2)),  # Top-left of screen.
+        ("hover", (80, 24), (50, -2)),  # Above screen.
+        ("hover", (80, 24), (100, -2)),  # Top-right of screen.
+        #
+        ("hover", (5, 5), (7, 3)),  # Right of screen.
+        ("hover", (5, 5), (7, 7)),  # Bottom-right of screen.
+        ("hover", (5, 5), (3, 7)),  # Under screen.
+        ("hover", (5, 5), (-1, 7)),  # Bottom-left of screen.
+        ("hover", (5, 5), (-1, 3)),  # Left of screen.
+        ("hover", (5, 5), (-1, -1)),  # Top-left of screen.
+        ("hover", (5, 5), (3, -1)),  # Above screen.
+        ("hover", (5, 5), (7, -1)),  # Top-right of screen.
+    ],
+)
+async def test_pilot_target_outside_screen_errors(method, screen_size, offset):
+    """Make sure that targeting a click/hover completely outside of the screen errors."""
+    app = App()
+    async with app.run_test(size=screen_size) as pilot:
+        pilot_method = getattr(pilot, method)
+        with pytest.raises(OutOfBounds):
+            await pilot_method(offset=offset)
+
+
+@pytest.mark.parametrize(
+    ["method", "offset"],
+    [
+        ("click", (20, 1)),  # Right of button.
+        ("click", (20, 5)),  # Bottom-right of button.
+        ("click", (10, 5)),  # Under button.
+        ("click", (-3, 5)),  # Bottom-left of button.
+        ("click", (-3, 2)),  # Left of button.
+        ("click", (-3, -2)),  # Top-left of button.
+        ("click", (10, -2)),  # Above button.
+        ("click", (20, -2)),  # Top-right of screen.
+        #
+        ("hover", (20, 1)),  # Right of button.
+        ("hover", (20, 5)),  # Bottom-right of button.
+        ("hover", (10, 5)),  # Under button.
+        ("hover", (-3, 5)),  # Bottom-left of button.
+        ("hover", (-3, 2)),  # Left of button.
+        ("hover", (-3, -2)),  # Top-left of button.
+        ("hover", (10, -2)),  # Above button.
+        ("hover", (20, -2)),  # Top-right of screen.
+    ],
+)
+async def test_pilot_target_outside_of_widget_but_inside_screen_errors(method, offset):
+    """This test makes sure that targeting a widget with a click that's outside of the
+    widget BUT inside the screen raises an `OutOfBounds` error.
+    """
+
+    app = CenteredButtonApp()
+    async with app.run_test(size=(80, 24)) as pilot:
+        pilot_method = getattr(pilot, method)
+        with pytest.raises(OutOfBounds):
+            await pilot_method(Button, offset=offset)
+
+
+@pytest.mark.parametrize(
+    ["method", "offset"],
+    [
+        ("click", (100, 12)),  # Right of screen.
+        ("click", (100, 36)),  # Bottom-right of screen.
+        ("click", (50, 36)),  # Under screen.
+        ("click", (-10, 36)),  # Bottom-left of screen.
+        ("click", (-10, 12)),  # Left of screen.
+        ("click", (-10, -2)),  # Top-left of screen.
+        ("click", (50, -2)),  # Above screen.
+        ("click", (100, -2)),  # Top-right of screen.
+        #
+        ("hover", (100, 12)),  # Right of screen.
+        ("hover", (100, 36)),  # Bottom-right of screen.
+        ("hover", (50, 36)),  # Under screen.
+        ("hover", (-10, 36)),  # Bottom-left of screen.
+        ("hover", (-10, 12)),  # Left of screen.
+        ("hover", (-10, -2)),  # Top-left of screen.
+        ("hover", (50, -2)),  # Above screen.
+        ("hover", (100, -2)),  # Top-right of screen.
+    ],
+)
+async def test_pilot_target_outside_of_widget_and_outside_screen_errors(method, offset):
+    """This test makes sure that targeting a widget with a click that's outside of the
+    widget AND outside the screen raises an `OutOfBounds` error.
+    """
+
+    app = CenteredButtonApp()
+    async with app.run_test(size=(80, 24)) as pilot:
+        pilot_method = getattr(pilot, method)
+        with pytest.raises(OutOfBounds):
+            await pilot_method(Button, offset=offset)
+
+
+@pytest.mark.parametrize(
+    ["method", "target"],
+    [
+        ("click", "#label0"),
+        ("click", "#label90"),
+        ("click", Button),
+        #
+        ("hover", "#label0"),
+        ("hover", "#label90"),
+        ("hover", Button),
+    ],
+)
+async def test_pilot_target_on_widget_that_is_not_visible_errors(method, target):
+    """Make sure that clicking a widget that is not scrolled into view raises an error."""
+    app = ManyLabelsApp()
+    async with app.run_test(size=(80, 5)) as pilot:
+        app.query_one("#label50").scroll_visible(animate=False)
+        await pilot.pause()
+
+        pilot_method = getattr(pilot, method)
+        with pytest.raises(OutOfBounds):
+            await pilot_method(target)
+
+
+@pytest.mark.parametrize("method", ["click", "hover"])
+async def test_pilot_target_widget_under_another_widget(method):
+    """The targeting method should return False when the targeted widget is covered."""
+
+    class ObscuredButton(App):
+        CSS = """
+        Label {
+            width: 30;
+            height: 5;
+        }
+        """
+
+        def compose(self):
+            yield Button()
+            yield Label()
+
+        def on_mount(self):
+            self.query_one(Label).styles.offset = (0, -3)
+
+    app = ObscuredButton()
+    async with app.run_test() as pilot:
+        await pilot.pause()
+        pilot_method = getattr(pilot, method)
+        assert (await pilot_method(Button)) is False
+
+
+@pytest.mark.parametrize("method", ["click", "hover"])
+async def test_pilot_target_visible_widget(method):
+    """The targeting method should return True when the targeted widget is hit."""
+
+    class ObscuredButton(App):
+        def compose(self):
+            yield Button()
+
+    app = ObscuredButton()
+    async with app.run_test() as pilot:
+        await pilot.pause()
+        pilot_method = getattr(pilot, method)
+        assert (await pilot_method(Button)) is True
+
+
+@pytest.mark.parametrize(
+    ["method", "target", "offset"],
+    [
+        ("click", "#label0", (0, 0)),
+        ("click", "#label3", (0, 0)),
+        ("click", "#label5", (2, 0)),
+        ("click", None, (10, 23)),
+        ("click", None, (70, 0)),
+        #
+        ("hover", "#label0", (0, 0)),
+        ("hover", "#label3", (0, 0)),
+        ("hover", "#label5", (2, 0)),
+        ("hover", None, (10, 23)),
+        ("hover", None, (70, 0)),
+    ],
+)
+async def test_pilot_target_screen_always_true(method, target, offset):
+    app = ManyLabelsApp()
+    async with app.run_test(size=(80, 24)) as pilot:
+        pilot_method = getattr(pilot, method)
+        assert (await pilot_method(target, offset=offset)) is True


Code review comments:
diff_hunk: @@ -104,21 +112,41 @@ async def click(
         else:
             target_widget = screen
 
+        if not target_widget.size.contains(*offset):
+            raise OutOfBounds(
+                f"Target size is {target_widget.size}, click offset is {offset}."
+            )
+
         message_arguments = _get_mouse_message_arguments(
             target_widget, offset, button=1, shift=shift, meta=meta, control=control
         )
+
+        click_offset = Offset(message_arguments["x"], message_arguments["y"])
+        visible_screen_region = screen.region + screen.scroll_offset
+        if not visible_screen_region.contains(*click_offset):
+            raise OutOfBounds(
+                "Target offset is outside of currently-visible"
+                + f"screen region {visible_screen_region}."
path: src/textual/pilot.py
text: @user1:
Don't think we need the region repr in there. Can't see it being helpful.

@author:
Removed.

diff_hunk: @@ -104,21 +112,41 @@ async def click(
         else:
             target_widget = screen
 
+        if not target_widget.size.contains(*offset):
path: src/textual/pilot.py
text: @user1:
I don't understand this. The offset should be added to the click coordinate. 

It is not an error to have an offset that puts the click position outside of the selected widget. It is only out of bounds if the click is outside of the screen.

@author:
Refactored after the back-and-forth in the original issue.

diff_hunk: @@ -104,21 +112,41 @@ async def click(
         else:
             target_widget = screen
 
+        if not target_widget.size.contains(*offset):
+            raise OutOfBounds(
+                f"Target size is {target_widget.size}, click offset is {offset}."
+            )
+
         message_arguments = _get_mouse_message_arguments(
             target_widget, offset, button=1, shift=shift, meta=meta, control=control
         )
+
+        click_offset = Offset(message_arguments["x"], message_arguments["y"])
+        visible_screen_region = screen.region + screen.scroll_offset
+        if not visible_screen_region.contains(*click_offset):
path: src/textual/pilot.py
text: @user1:
I think this works:

```suggestion
        if click_offset not in visible_screen_region:
```

@author:
Changed.