Skip to content
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: load event with non-url subframe #13340

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Apr 6, 2022

fixes: #12182
fixes: #9028

@liuxingbaoyu liuxingbaoyu changed the title fix: load event with non-src subframe fix: load event with non-url subframe Apr 6, 2022
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice find, thank you for the PR! However, both linked issues are Firefox-specific, so I believe the proper fix would be in Firefox instrumentation that does not send any load/domcontentloaded events. Adding @aslushnikov for any ideas.

@liuxingbaoyu
Copy link
Contributor Author

Thanks for your review!

It seems that it's not just firefox that has the problem.

Although only changing firefox can solve these two issues, the three browsers have not been able to pass the test completely before.

Firefox, webkit, and chrome are all inconsistent for javascript|text/html frame loading events.

And it looks like not waiting for non-url frames has little effect on behavior, so I went with this solution.

@zhanglei4333
Copy link

Hello, can I download the latest package to solve this problem, or change my case according to your code

@liuxingbaoyu
Copy link
Contributor Author

This pr has not been merged yet.

You can cherry-pick and build it yourself.

Or modify the files in node_module.

@aslushnikov
Copy link
Collaborator

aslushnikov commented Apr 20, 2022

@liuxingbaoyu thank you for the PR and awesome tests.

So indeed each browser struggles differently. Here's the summary:

  • javascript-src iframe
    • chromium: ❌ never fires "networkidle" for iframe
    • webkit: ✅
    • firefox: ❌ never fires "load" and "networkidle" for iframe
  • data-src iframe
    • chromium: ✅
    • webkit: ❌ never fires "networkidle" for iframe
    • firefox: ❌ never fires "networkidle" for iframe
  • blob-src iframe
    • chromium: ❌ never fires "networkidle" and "load" for iframe
    • webkit: ✅
    • firefox: ❌ never fires "domcontentloaded", "load" and "networkidle" for iframe

Let me see if it's worth fixing upstream in browsers or on our side.

@zhanglei4333
Copy link

This pr has not been merged yet.

You can cherry-pick and build it yourself.

Or modify the files in node_module.
Excuse me, how can I modify node_ Module file, which package needs to be modified

@zhanglei4333
Copy link

This pr has not been merged yet.

You can cherry-pick and build it yourself.

Or modify the files in node_module.

My node_module There is no playwright-core package in the module file

@zhanglei4333
Copy link

After changing according to your idea, my original case can't run. Have you encountered this problem

@dgozman
Copy link
Contributor

dgozman commented Jun 6, 2022

Most of these issues have been fixed, thank you for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants