Fix selector visible and hidden

[zeevrosental]

fix(waitForSelector): waiting for visible or hidden fail with multiple elements

function fails if multiple elements have same selector and not same visibility

[msftclas]

All CLA requirements met.

[dgozman]

Great PR, thank you for sending it out! Implementation looks good to me.

Could you please share more details on the specific scenario where this is useful? I am not sure that existing semantic is better or worse than the proposed one. Since this would be on the edge of a breaking change, we need stronger evidence to go this way.

[zeevrosental]

Hi @dgozman,
Glad you liked the PR :-)

I apologize in advance if my answer is quite long...

I think there might be a simpler example... but I will give one that is similar to the one I encountered:
Let’s say I have in my system a navigation bar with sub menus like in this page: https://www.w3schools.com/howto/howto_css_subnav.asp

And I have many many users, and each one has different options in the navigation bar.
In order to test each navigation bar without too much of a hassle, I create a function that uses Playwright to map the navigation bar dynamically so I could receive the whole navigation bar state as an object.

The subnav in the link would look something like this:

const subNav =["Home", {About:["Company","Team","Careers"]}, {Services:["Bring","Deliver","Package","Express"]}, etc...]

where all the primary buttons have a similar data-test-id (e.g. "Menu-button-Home"), and all the subMenu Buttons also have a similar data-test-id(e.g. "subMenu-option-Bring");

when I map the navbar dynamically I do not know the full data test, so I query something like this:

await page.waitForSelector('[data-test-id*="subMenu-option"]';

and then, after I know it is visible, I use an eval to get all the subMenu options.

the problem occurs when I move from "About", to "Services":

I wait for all the subMenus from the About option to turn hidden:

await page.waitForSelector('[data-test-id="subMenu-option-Company", {state:"hidden"}]';

which work fine, but afterwards when I do the same query I did before in order to reveal the new subMenu:

await page.waitForSelector('[data-test-id*="subMenu-option"]';

I receive a timeout.(this is the bug)

obviously if there is a visible element that answers to my selector, and I am waiting for a visible element, the function should not fail because there is a different hidden element that answers to the same selector.

my implementation checks that there is at least one element that answers to the requested condition, whereas the current implementation checks only if the first arbitrary element with this selector answers to the requested condition.

once again, sorry if it was quite long :-)

[dgozman]

Thank you for the details. It seems like your usecase is "wait until at least one element matching the selector is visible". I am still not sure that logic will work for everyone. Here are a few ideas:

• Use more specific selector, if possible:

await page.waitForSelector('[data-test-id*="subMenu-option"]:nth-child(5)', { state: 'visible' })

• Use waitForFunction and manually check visibility:

await page.waitForFunction(() => {
  const submenus = Array.from(document.querySelector('[data-test-id*="subMenu-option"]'));
  return submenus.map(e => e.getBoundingClientRect()).some(r => r.width > 0 && r.height > 0);
});

• Possible new API (does not exist today):

await page.waitForSelectors('[data-test-id*="subMenu-option"]',
  elements => elements.map(e => e.getBoundingClientRect()).some(r => r.width > 0 && r.height > 0);
});

• Different new API (does not exist today):

await page.waitForSelectors('[data-test-id*="subMenu-option"]',
  elements => elements.length > 0,
  { state: 'visible' }
);

Let me know what you think.

[richardwillars]

What might be a nice way of dealing with it is just giving the developer more information - if a method is called that expects one selector but find multiple elements you could just output a warning with a list of all the matched elements in the terminal. Debugged in a few seconds, job done.

[zeevrosental]

Hi @dgozman,

Thank you for your elaborate answer,

I have to admit I still think that there is a very major problem in the basic of the usage of the "visible" in playwright.

Hidden elements are a nuisance, they stray you away from the happy path of testing, and in the current implementation they remain quite a very big problem.

Basically, I think my main point is that usage of {state:”visible”} should be as an additional selector, because it is such a powerful one, and a much needed one in an automation framework.

let’s say I have 2 forms in my browser window, both of them have a "submit form" with data-test-id of "submit_form". If one of them is hidden, and I try to click a data-test-id that matches both of them, I will always want to click the visible element.. Always.

In a case like that, creating a more specific selector is time consuming, code consuming and creates a flakier test.

With my suggested implementation:
await page.click("submit_form");

With current implementation(using waitForFunction):

//waiting and receiving visible element from selector
const formButton = await page.waitForFunction(() => { const formSubmits = Array.from(document.querySelector('[data-test-id="submit_form"]'));

return formSubmits.find(e => e.getBoundingClientRect().width > 0 && e.getBoundingClientRect().height > 0); });

//clicking on returned element
await formButton.click;

Or even worse than that, let’s say I had a solid test running for 2 years with the same form. And just because an invisible form was added, my test will start failing because my system timed out waiting for a new invisible element.. It creates a much less reliable automation framework.

I understand that currently all the single element functions are implemented in the same fashion: they do document.querySelector and continue from there.. But I think it makes much more sense to use the visibility as an extension of the selector criteria, and not as something I check on one specific element.

I know that both of the answers are possible, but I have the distinct feeling that the way that I expect the system to behave is much more common and makes much more sense.

Anyway, in a way of looking for the middle ground, maybe it is possible to add another option? I think it should be something like “elementIndex” or “elementDOMIndex” , this will still enable the same old functionality with elementIndex :0 but will keep the happy path as the main path in the functions.. I think this will enable much more fluidity for most users.

Obviously I’m sure no one is keen on adding extra options, but I think this one is very much justified.
Anyway, if you are for it, I’ll be happy to work on the PR :-)
Have a great day

[dgozman]

Thank you for the detailed response.

We discussed this and decided to continue with out predictable model. It is easy to understand and consistent:

• Querying for a single selector always returns the first match.
• All the checks including visibility are applied after that.
• This is consistent with page.click('selector') that always uses the first one, and is able to wait for that selector to become visible before clicking.
• Whenever selector stops pointing to the desired element, it has to be changed. This ensures a more specific selector that survives future refactorings. For example, using the test-id=submit-button practice is safe.

I have drafted a proposal in #2370 to address some usecases mentioned in this PR - comments and different proposals are welcome!

Again, thank you for the fantastic PR!

[zeevrosental]

my pleasure..
the new proposal sounds great!,
keep up the great work :-) !