fix!: fix bounding box visibility conditions

[jrandolf]

This PR implements better conditions for bounding box visibility. We define a bounding box as visible if

1. Some part of the element is viewable in the viewport.
2. It is not hidden, display != none, and visibility is set to an attribute showing the element.
3. The width and height are both non-zero.

Fixes: #8953

[jrandolf]

@OrKoN It seems tests weren't covering intended behavior, just programmed behavior ;)

[OrKoN]

I actually wonder if it's going to be a breaking change since people likely rely on this? Anyway I think we should fix it but perhaps see if we mark it as a breaking change and land after all the refactoring and features.

[jrandolf]

Could we land it together? All the refactors and features are on top of this.

[OrKoN]

Let's mark it as non breaking then as it's technically a fix. Let's include the definition of what a visible bounding box is in the commit description.

[jrandolf]

I've marked it as a breaking fix. It is still breaking, but it's a fix :P

[heikkipora]

We use Page.waitForSelector({visible: true}) quite heavily even for simple cases like this:

1. click a button
2. wait for a new element to appear in DOM and be visible (i.e. not explicitly hidden) - just not necessarily inside the viewport

This change breaks a few thousand test cases for us and working around it is tedious.
The the referenced bug (#8953) seems to have nothing to do with the viewport - so I'm more than a bit curious why that (viewport rules) has been changed as well.

Suggestion: maybe this new behavior could be moved behind a parameter/argument instead to reduce the impact on existing codebases - or remove the checks about inclusion in viewport altogether.

Additionally, Puppeteer's API docs still state that Visible: A boolean wait for element to be present in DOM and to be visible, i.e. to not have display: none or visibility: hidden CSS properties. Defaults to false.

[jrandolf]

@heikkipora We want visibility to be defined as user-interactive since that is the main use case (which in hindsight should probably be renamed to interactive). I wouldn't mind implementing a flag that can support your use-case, but do you mind providing more information? (I'm thinking it could be used for animations)

In any case, waitForSelector then waitForFunction is always an option.

[joakimgunst]

We have the same issue as @heikkipora. We have many test cases where we perform an action, and then wait for an element to be visible in the page, but not necessarily in the viewport. Using waitForSelector without visible: true works, but doesn't fail when the element has display: none, visibility: hidden or an empty bounding box, all of which are useful conditions to check.

For me it is not intuitive to consider an element that is otherwise visible but not in the viewport not visible. Other libraries like Playwright and Cypress also do not include this in their definitons of visibility.

[pasieronen]

One use case I have is clicking e.g. a form submit button and then waiting with page.waitForSelector('.form-successfully-sent', {visible: true}) before continuing. Depending on how the site is implemented, the .form-successfully-sent element could exist in the DOM even before the form is submitted (with display: none).

But once the element is visible, the test doesn't really care if it's inside the viewport. Often it probably is, but if waitForSelector requires this, then the code breaks if e.g. texts earlier in the page get longer...

[krisatverbidio]

We also have the same issue as @heikkipora and @joakimgunst . We use visibility to determine if it is in the dom, even if it is off page. This is a major breaking change without a workaround.

[jrandolf]

it is not intuitive to consider an element that is otherwise visible but not in the viewport not visible.

@joakimgunst As a user, this is very intuitive. In fact, we should technically be stricter and say opacity should be included, but we don't because a user can interact with transparent elements. Visibility in our case is defined as user Interactive.

Other libraries like Playwright and Cypress also do not include this in their definitons of visibility.

Playwright is a fork of Puppeteer and Cypress uses Puppeteer... it's not strange they do not include it since we were the originators of their definitions.

Depending on how the site is implemented, the .form-successfully-sent element could exist in the DOM even before the form is submitted (with display: none).

@pasieronen Please specify the implementation. This is not helpful in understanding what the problem space is.

We use visibility to determine if it is in the dom, even if it is off page. This is a major breaking change without a workaround.

@krisatverbidio Why not just use waitForSelector without visibility?

[andersk]

The Puppeteer tests for Zulip are also broken by this change. A few thoughts:

We want visibility to be defined as user-interactive

An element that’s outside the viewport but could be scrolled into view should count as user-interactive, because the .click() method will scroll it into view.

Why not just use waitForSelector without visibility?

In addition to the obvious purpose of waiting for an element to be marked visible in the DOM, we also use it as a workaround for other problems like #6830.

[pasieronen]

Depending on how the site is implemented, the .form-successfully-sent element could exist in the DOM even before the form is submitted (with display: none).

@pasieronen Please specify the implementation. This is not helpful in understanding what the problem space is.

Back in the "jQuery era of Javascript", it was common to serve static HTML that contained many blocks with display: none and then show those when needed with $(...).show(). Here's a fairly typical example from that era, where both #success_message and #error_message are in the DOM: http://reusableforms.com/d/o5/html5-contact-form-send-email

I agree with krisatverbidio that this is a major breaking change - perhaps waitForSelectorshould have an additional flag like inViewport: true to control this behavior?

[jrandolf]

An element that’s outside the viewport but could be scrolled into view should count as user-interactive, because the .click() method will scroll it into view.

@andersk This was definitely not intentional. I've pushed out #9087 to make it required that the element can be at least scrolled to by checking scroll bounds instead of viewport. Would this settle the problem? Ping @joakimgunst @heikkipora @pasieronen @krisatverbidio

[joakimgunst]

How about the case where the element is inside a scroll container that is not the body element? For example, on this page the XMLDocument link in the sidebar would not be considered visible, even though the user can scroll to it.

document.querySelector('[href="/en-US/docs/Web/API/XMLDocument"]').getBoundingClientRect().top
// 9002

document.body.scrollHeight
// 3035

.click() seems to be using scrollIntoViewIfNeeded() or scrollIntoView(), which both would scroll the sidebar container so that the XMLDocument link is visible.

[jrandolf]

After some discussion, we've decided to remove the condition!