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

Broken keyboard input implementation #124

Open
jaydenseric opened this issue Jan 1, 2025 · 8 comments · Fixed by #129
Open

Broken keyboard input implementation #124

jaydenseric opened this issue Jan 1, 2025 · 8 comments · Fixed by #129
Labels
bug Something isn't working

Comments

@jaydenseric
Copy link

If you do await page.keyboard.down("Alt"), then this keydown listener will fire:

document.addEventListener("keydown", (event) => {
  console.log("keydown:");
  console.log(event.key);
});

Incorrectly however, event.key will be "".

At this point, because there was not yet await page.keyboard.up("Alt"), when you do .click() on an element in the page, the click event property altKey should be true. But, incorrectly it's false.

This test passes when using Puppeteer:

https://github.com/jaydenseric/ruck/blob/aa068afa7f0630c4e3e8617b2936210c14815964/useOnClickRouteLink.test.mjs#L121-L126

Here is the source code being tested:

https://github.com/jaydenseric/ruck/blob/aa068afa7f0630c4e3e8617b2936210c14815964/useOnClickRouteLink.mjs#L31

But with Astral, the test incorrectly fails when it should pass.

Here is the relevant code in Astral vs Puppeteer:

astral/src/keyboard.ts

Lines 278 to 287 in 0a4ed05

/**
* Dispatches a `keydown` event.
*/
async down(key: KeyInput) {
await this.#celestial.Input.dispatchKeyEvent({
type: "keyDown",
modifiers: this.#modifiers,
text: key,
});
}

https://github.com/puppeteer/puppeteer/blob/83449567c7013ae1c87c54e42501aa74575640d0/packages/puppeteer-core/src/cdp/Input.ts#L55-L82

@lino-levan lino-levan added the bug Something isn't working label Jan 1, 2025
@jaydenseric
Copy link
Author

@lino-levan what is a realistic expectation for when this bug might be resolved?

@lino-levan
Copy link
Owner

I can try to prioritize working on this today. Planning to take some time off work to get Astral into much better shape, both in terms of fixing issues and documentation.

@jaydenseric
Copy link
Author

Unfortunately, my test suit that passes with Puppeteer still doesn't pass with Astral v0.5.2:

// Test holding the alt key when clicking the link prevents a
// client side navigation…

await page.keyboard.down("Alt");
await page.locator('a[href="/b"]').click();
await page.keyboard.up("Alt");

assertStrictEquals(await browserPageUrl(page), urlPageA);

The assertion fails that a navigation did occur.

Do you think it's a bug with Astral, or do I need to use different capitalisation with "Alt" or something Astral specific?

@lino-levan
Copy link
Owner

Is there some way for you to write a keyboard testcase that I can work on passing?

Here's an example of a testcase in the codebase
Deno.test("Keyboard - tab navigation", async () => {
  const browser = await launch();
  const page = await browser.newPage();

  await page.setContent(`
    <!DOCTYPE html>
    <html>
      <body>
        <input id="first" type="text" placeholder="First input">
        <input id="second" type="text" placeholder="Second input">
        <button id="button">Click me</button>
        <textarea id="textarea">Text area</textarea>
        <div id="focused"></div>
        <script>
          const focused = document.getElementById('focused');
          document.addEventListener('focusin', (e) => {
            if (e.target.id) {
              focused.textContent = (focused.textContent || '') + e.target.id + ',';
            }
          });
        </script>
      </body>
    </html>
  `);

  // Start with first input focused
  const firstInput = await page.$("input#first");
  assertExists(firstInput);
  await firstInput.click();

  // Press tab multiple times to move through elements
  await page.keyboard.press("Tab");
  await page.keyboard.press("Tab");
  await page.keyboard.press("Tab");

  // Check the focus order
  const focusOrder = await page.evaluate(() =>
    document.getElementById("focused")?.textContent || ""
  );
  assertEquals(focusOrder, "first,second,button,textarea,");

  await browser.close();
});

Would also be good for regression testing.

@lino-levan lino-levan reopened this Jan 20, 2025
@jaydenseric
Copy link
Author

I think the thing to test is that mouse events like click have event.altKey etc. working. Searching the codebase for dispatchMouseEvent:

https://github.com/search?q=repo%3Alino-levan%2Fastral%20dispatchMouseEvent&type=code

It seems none of the event details include the keyboard modifier properties like altKey, etc:

astral/src/mouse.ts

Lines 47 to 53 in 348dda3

await this.#celestial.Input.dispatchMouseEvent({
type: "mousePressed",
x: this.#x,
y: this.#y,
button: opts?.button ?? "left",
clickCount: opts?.clickCount ?? 1,
});

@lino-levan
Copy link
Owner

Right, didn't even know that was in there. Great callout.
Image

@jaydenseric
Copy link
Author

jaydenseric commented Jan 21, 2025

Yes:

https://chromedevtools.github.io/devtools-protocol/tot/Input/#method-dispatchMouseEvent

I wonder if touch events, and other input events, need attention too.

@lino-levan
Copy link
Owner

Very likely the case, this'll be fun to work through.

jaydenseric added a commit to jaydenseric/ruck that referenced this issue Jan 21, 2025
They are failing due to an upstream Astral issue:

lino-levan/astral#124 (comment)
@lino-levan lino-levan reopened this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants