-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
fix!: fix bounding box visibility conditions #8954
Conversation
2aeb178
to
4616713
Compare
@@ -609,7 +699,7 @@ describe('waittask specs', function () { | |||
it('should have an error message specifically for awaiting an element to be hidden', async () => { | |||
const {page} = getTestState(); | |||
|
|||
await page.setContent(`<div></div>`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrKoN It seems tests weren't covering intended behavior, just programmed behavior ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we land it together? All the refactors and features are on top of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've marked it as a breaking fix. It is still breaking, but it's a fix :P
5505b98
to
fb8cb27
Compare
9db3dc9
to
ef7425f
Compare
63371c5
to
36881d5
Compare
36881d5
to
e4501ff
Compare
We use
This change breaks a few thousand test cases for us and working around it is tedious. 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 |
@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. |
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 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. |
One use case I have is clicking e.g. a form submit button and then waiting with 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... |
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. |
@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.
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.
@pasieronen Please specify the implementation. This is not helpful in understanding what the problem space is.
@krisatverbidio Why not just use |
The Puppeteer tests for Zulip are also broken by this change. A few thoughts:
An element that’s outside the viewport but could be scrolled into view should count as user-interactive, because the
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. |
Back in the "jQuery era of Javascript", it was common to serve static HTML that contained many blocks with I agree with krisatverbidio that this is a major breaking change - perhaps |
@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 |
How about the case where the element is inside a scroll container that is not the document.querySelector('[href="/en-US/docs/Web/API/XMLDocument"]').getBoundingClientRect().top
// 9002
document.body.scrollHeight
// 3035
|
After some discussion, we've decided to remove the condition! |
This PR removes the viewport conditions in `waitForSelector`. See discussion: #8954 (comment)
This PR implements better conditions for bounding box visibility. We define a bounding box as visible if
none
, and visibility is set to an attribute showing the element.Fixes: #8953