microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
67.17k stars 3.69k forks source link

[Feature]: Visible option for locators #31840

Open qk5 opened 4 months ago

qk5 commented 4 months ago

🚀 Feature Request

I have a locator: page.getByText('Waiting for response...', { exact: true });

And playwright points to multiple locators on screen, one visible locator and other invisible ones.

I only want the visible one, and my workaround is to use the following locator: page.locator('div:has-text("Waiting for response..."):visible');

Please note that: If I remove the part :visible, this locator will behave the same as my first example using getByText()

So, is it possible to add an additional option within getByText() and other getBy locator methods for visible locator?

Maybe something like: page.getByText('Waiting for response...', { visible: true });

Example

No response

Motivation

I believe having the visible option will give us more flexibility when creating any getBy locators.

qk5 commented 4 months ago

Update: Upon reading more on the playwright documentation, I found out the following also works:

page.getByText('Waiting for response...', { exact: true }).locator('visible=true');

But I still think if we can expose visible as an option, it will look cleaner. Please feel free to correct me if I am wrong.

Thank you.

Skn0tt commented 3 months ago

Hey @qk5! We've discussed this in our team sync yesterday. The fact that by default, getByText selects not just visible elements but also invisible ones is a design regret of ours. All the RTL-Style selectors (think getByText, getByAltText, getByRole) are modeled after human perception, and that means that it's surprising for it to include hidden elements. So if we could go back in time, we would change the defaults so that getByText only selects visible elements.

We can't though, and we'd prefer not to introduce breaking changes in such a fundamental part of our API. Even if we made this behaviour a config toggle, we'd be forking our userbase into two fundamentally different behaviours, which would lead to even more confusion. Really bad, especially when you consider that libraries have no way of detecting it.

Now, the visible option you propose is an interesting way out of this! Before we think about implementing it though, we'd like to understand how big this problem really is. I'll label it "P3: Collecting Feedback" and then we can collect some upvotes on the issue. Hopefully that gives us some perspective on the gravitas of this, and sleep a couple of nights about other solutions to the problem.

Skn0tt commented 3 months ago

Also, an interesting observation in API design: in getByText("...", { visible: true }), the visible option is trimodal! With the defaults we have, { visible: false } clearly means "only select invisible", { visible: true } means "only select visible", and { visible: undefined } means "do the default", which is selecting both visible and invisible elements.

dtinth commented 3 months ago

I want to chime in that a workaround for this is to use getByRole instead of getByText, as we can exploit the fact that invisible elements do not have a role:

when visible when hidden
image image

Maybe the documentation can address getByText’s quirk (of also selecting visible elements) to nudge people into using getByRole more.

miso-belica commented 1 month ago

We can't though, and we'd prefer not to introduce breaking changes in such a fundamental part of our API. Even if we made this behaviour a config toggle, we'd be forking our userbase into two fundamentally different behaviours, which would lead to even more confusion. Really bad, especially when you consider that libraries have no way of detecting it.

@Skn0tt For testing a react-native-web applications this is really important because the routing works by hiding the pages and stack them one on another. Then selecting base things like navigation can lead to multiple elements found in all tests. Adding a global option to ignore invisible/hidden elements would be really a great help here so we don't have to add .locator('visible=true') to every selector.

The package react-native-testing-library changed the default behavior to exclude elements hidden from accessibility by default so maybe you can inspire or talk to them about this. It was a big help for our tests.