-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 in_view function #3885
fix in_view function #3885
Conversation
|
✔️ Deploy Preview for kit-demo canceled. 🔨 Explore the source changes: 2f75426 🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/6208b699a1a38e00087b2e6a |
@bluwy @benmccann the tests using in_view are being flaky again. |
Yeah, I don't think the tests were de-flaked in the first place. |
I'm trying this now https://stackoverflow.com/a/68848306 |
@bluwy what do you think of that, it seems to pass consistently. |
Everywhere except Windows |
https://github.com/puppeteer/puppeteer/blob/08e9978ce662cf1c2c2708f7e6098cbea328cbe2/src/common/JSHandle.ts#L1133-L1151 uses it, but in the Playwright the Windows is still inconsistent. |
@bluwy @benmccann This works consistently even on Windows, it's the old deprecated way, but it works: in_view: async ({ page }, use) => {
/** @param {string} selector */
async function in_view(selector) {
return await page.$eval(selector, async (/** @type {Element} */ element) => {
// @ts-ignore
let top = element.offsetTop;
// @ts-ignore
let left = element.offsetLeft;
// @ts-ignore
const width = element.offsetWidth;
// @ts-ignore
const height = element.offsetHeight;
// @ts-ignore
while (element.offsetParent) {
// @ts-ignore
element = element.offsetParent;
// @ts-ignore
top += element.offsetTop;
// @ts-ignore
left += element.offsetLeft;
}
return top < window.pageYOffset + window.innerHeight &&
left < window.pageXOffset + window.innerWidth &&
top + height > window.pageYOffset &&
left + width > window.pageXOffset
? true
: { top, left, width, height };
});
}
use(in_view);
} Maybe you or I can do a PR for it and merge it. |
Thanks for digging into this @PH4NTOMiki. My first encounter with the scrolling shenanigans was also through that stackoverflow post, and the function had evolved over time. I'm not sure if using Re the old deprecated way, do you know why that works more reliably? I'm not 100% sure if the function is safe too. Also maybe related, perhaps we can setup the CI to emit the screenshots for failed tests. That might show us if the element is actually inside the view, or it's an error with |
It's already outputting screenshots and failure traces on the CI: kit/packages/kit/test/utils.js Line 194 in 0c6bc25
I'm not sure how to retrieve them though. I guess we'd have to setup something to upload them somewhere? |
There were a couple tests I couldn't get to fail on my machine. A user in Discord provided me with these traces awhile back to help us debug. Unfortunately, I haven't gotten to investigate yet. But maybe someone else will be able to 😁 |
I've done something like this before using |
Well it's more reliable than current ones, but still not 100% reliable, but it's the best we currently have. |
@bluwy I think it works better because it doesn't use boundingClientRect(boundingBox in playwright is similar). It uses element static properties. |
@PH4NTOMiki's #3873 (comment) reminded me of #3741 (comment). The
in_view
function now isn't correct and reportstrue
if the element is anywhere on the page, not stricly in the viewport as expected.With this PR,
in_view
only returns true if the element is actually in the viewport.Notes
According to playwright's docs:
Playwright's term for "is not visible" is confusing:
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpx changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0