microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
67.18k stars 3.69k forks source link

fix(codegen): document.documentElement is null on early navigation #33627

Closed mxschmitt closed 1 week ago

mxschmitt commented 1 week ago

This PR fixes the following when opening Codegen on https://primevue.org/inputnumber/:

VM26:921 Recorder::installListners install error TypeError: Cannot read properties of null (reading 'contains')
    at Highlight.install (<anonymous>:4742:56)
    at Recorder.installListeners (eval at extend (inputnumber/:5876:40), <anonymous>:919:22)
    at new Recorder (eval at extend (inputnumber/:5876:40), <anonymous>:890:10)
    at new PollingRecorder (eval at extend (inputnumber/:5876:40), <anonymous>:1322:22)
    at InjectedScript.extend (<anonymous>:5882:12)
    at eval (eval at evaluate (inputnumber/:234:30), <anonymous>:5:29)
    at UtilityScript.evaluate (<anonymous>:236:17)
    at UtilityScript.<anonymous> (<anonymous>:1:44)

In TypeScript this type is not nullable. This is working as intended from their side: https://github.com/microsoft/TypeScript/issues/50078

Minimal Playwright repro script. This sometimes returns true and sometimes false.

import { chromium } from 'playwright';

(async () => {
  const check = async () => {
    const browser = await chromium.launch({ headless: false });
    const context = await browser.newContext();
    const page = await context.newPage();
    await page.goto('https://primevue.org/inputnumber/', {waitUntil: 'commit'});
    console.log(await page.evaluate(() => !!document.documentElement));

    await context.close();
    await browser.close();
  };
  while (true)
    await check();
})();

Contract-wise we have three users of Highlight::install:

  1. Recorder.installListeners
  2. Locator.highlight
  3. Locator.maskSelectors

The first one retries every couple of seconds. The other two we expect to be called after the navigation has happened IIUC.

I went over the rest of the code-base, there we usually check for null when interacting with document.documentElement.

github-actions[bot] commented 1 week ago

Test results for "tests 1"

1 failed :x: [webkit-page] › page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18

36894 passed, 650 skipped :heavy_check_mark::heavy_check_mark::heavy_check_mark:

Merge workflow run.

github-actions[bot] commented 1 week ago

Test results for "tests 1"

15 passed :heavy_check_mark::heavy_check_mark::heavy_check_mark:

Merge workflow run.

github-actions[bot] commented 1 week ago

Test results for "tests 1"

3 flaky :warning: [chromium-page] › page/page-event-request.spec.ts:138:3 › should report navigation requests and responses handled by service worker with routing @chromium-ubuntu-22.04-node18
:warning: [chromium-page] › page/page-event-request.spec.ts:110:3 › should report navigation requests and responses handled by service worker @chromium-ubuntu-22.04-node22
:warning: [chromium-library] › library/inspector/cli-codegen-java.spec.ts:49:5 › should print the correct context options when using a device @ubuntu-20.04-chromium-tip-of-tree

36892 passed, 650 skipped :heavy_check_mark::heavy_check_mark::heavy_check_mark:

Merge workflow run.