Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict Pilot.click to visible area #3349

Closed
rodrigogiraoserrao opened this issue Sep 19, 2023 · 10 comments · Fixed by #3360
Closed

Restrict Pilot.click to visible area #3349

rodrigogiraoserrao opened this issue Sep 19, 2023 · 10 comments · Fixed by #3360
Assignees

Comments

@rodrigogiraoserrao
Copy link
Contributor

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.

@rodrigogiraoserrao
Copy link
Contributor Author

(Not sure why it shows my face in the first comment; that was written by Will.)

@rodrigogiraoserrao
Copy link
Contributor Author

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.

Code showing a click that doesn't land on the target.
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())

@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Sep 19, 2023
@rodrigogiraoserrao
Copy link
Contributor Author

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.

rodrigogiraoserrao added a commit that referenced this issue Sep 20, 2023
The methods click/hover will now raise an error if they target a region that is outside of the target widget or completely outside of the screen. Then, they return a Boolean that indicates whether the click/hover hit the intended widget or not.

Related issue: #3349.
@rodrigogiraoserrao
Copy link
Contributor Author

Given the review feedback received, the new proposal is as follows:

  • Pilot.click will raise OutOfBounds if the click falls outside of the visible screen area.
  • Pilot.click will have its signature changed to Pilot.click(offset: tuple[int, int] = (0,0), offset_relative_to: type[Widget] | str | None = None, ...).
  • Pilot.click returns the widget where the click landed.
  • The docstring of Pilot.click will be changed to make it clear that the semantics of Pilot.click is not “let's click a widget” but “let's click a position on the screen”, as the first sentence and the selector/offset descriptions seem to clash.
  • Pilot.hover will also be changed accordingly.

@willmcgugan
Copy link
Collaborator

What is the purpose of offset_relative_to ?

Pilot.click returns the widget where the click landed.

Can you give me an example of a test that uses this?

@rodrigogiraoserrao
Copy link
Contributor Author

  • offset_relative_to is the parameter selector, I just renamed it and moved it after offset, so that it is clearer that Pilot.click will click a position on the screen given by the offset, but that a selector can be provided as an anchor for the offset.

  • I'd guess the return value would be used mostly as a sanity check. Since we are testing apps we cannot see, being able to make sure that clicks/hovers ended up on a specific widget looks like a very convenient thing to me.

@willmcgugan
Copy link
Collaborator

  • offset_relative_to is the parameter selector, I just renamed it and moved it after offset, so that it is clearer that Pilot.click will click a position on the screen given by the offset, but that a selector can be provided as an anchor for the offset.

I don't like the change of name, as it doesn't suggest what it is. Making it a keyword arg also makes it more verbose to write tests. It's also more common to click a thing with no offset, so it seems to be in the wrong order now.

  • I'd guess the return value would be used mostly as a sanity check. Since we are testing apps we cannot see, being able to make sure that clicks/hovers ended up on a specific widget looks like a very convenient thing to me.

Yes, but can I have an example? I would like to see how a dev would benefit from this in a practical example.

@rodrigogiraoserrao
Copy link
Contributor Author

I don't like the change of name, as it doesn't suggest what it is.

It says exactly what it is: the widget to which the provided offset is relative to.

Making it a keyword arg also makes it more verbose to write tests. It's also more common to click a thing with no offset, so it seems to be in the wrong order now.

I'm happy to implement a Pilot.click_widget(selector) whose sole purpose is to click a specific widget, returning the widget where the click landed. (For the same reasons as above.)

Yes, but can I have an example? I would like to see how a dev would benefit from this in a practical example.

  • Testing that layers put widgets in front and then clicks don't make it to the hidden widgets:
from textual.app import App, ComposeResult
from textual.widgets import Static


class LayersExample(App):
    CSS = """
    Screen {
        layers: below above;
    }

    #box1 {
        layer: above;
    }

    #box2 {
        layer: below;
    }
    """

    def compose(self) -> ComposeResult:
        yield Static("box1 (layer = above)", id="box1")
        yield Static("box2 (layer = below)", id="box2")


async def test_clicks_target_first_layer():
    app = LayersExample()
    async with app.run_test() as pilot:
        box_above = app.query_one("#box1")
        assert (await pilot.click((1, 1), "#box2")) is box_above
  • Testing that the hover pseudo-class is applied correctly:
class ThisAppHasAButton(App):
    ...

async def test_hover_pseudo_class_applied_on_hover():
    app = ThisAppHasAButton()
    async with app.run_test() as pilot:
        button = app.query_one(Button)
        assert (await pilot.click_widget(Button)) is button  # sanity check
        assert "hover" in button.get_pseudo_classes()

@rodrigogiraoserrao
Copy link
Contributor Author

After some more talking, we'll just clear up the wording of docstrings and return a Boolean value indicating whether the click/hover happened to hit the selected widget.
We also only raise an OOB exception if we try to click outside of the visible area of the screen.

@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants