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(screenshotter): only wait for for document.fonts.ready on locator frame / page main frame #31295

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Jun 13, 2024

Investigation

We use nonStallingEvaluateInExistingContext as of today, which does eval() inside (from our utilityScript) which breaks for some sites. It causes a hang, since the returned Promise of eval() hangs. We don't know as of today why this happens. Without wrapping it ini eval() it does not hang.

Workaround: Do a plain Runtime.evaluate instead.

workaround: Only wait on main frame.

Relates #28995 (keeping it open until they confirm that it helps)

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the waiting-for-fonts-screenshotter branch from f1f149c to 3fb6fee Compare June 13, 2024 22:25
@mxschmitt mxschmitt changed the title fix: speculative fix for hanging screenshotter when waiting for fonts.ready fix(screenshotter): only wait for for document.fonts.ready on locator frame / page main frame Jun 13, 2024
Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [installation tests] › playwright-component-testing.spec.ts:21:5 › pnpm: @playwright/experimental-ct-react should work
❌ [installation tests] › playwright-component-testing.spec.ts:21:5 › pnpm: @playwright/experimental-ct-react should work
❌ [installation tests] › playwright-component-testing.spec.ts:21:5 › pnpm: @playwright/experimental-ct-react should work

28344 passed, 636 skipped
✔️✔️✔️

Merge workflow run.

@mxschmitt mxschmitt merged commit a9200be into microsoft:main Jun 13, 2024
27 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants