Skip to content

chore(bidi): reimplement RawKeyboard.sendText()#41637

Open
hbenl wants to merge 1 commit into
microsoft:mainfrom
hbenl:reimplement-sendtext
Open

chore(bidi): reimplement RawKeyboard.sendText()#41637
hbenl wants to merge 1 commit into
microsoft:mainfrom
hbenl:reimplement-sendtext

Conversation

@hbenl

@hbenl hbenl commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Bidi doesn't have a protocol method corresponding to CDP's Input.insertText, so this was emulated using keyboard actions. However, this emits key* events that are not expected for Keyboard.insertText(), breaking several tests.
This implementation tries to emulate CDP's Input.insertText behavior more closely.

Fixes the following tests:

  • page/page-fill.spec.ts:
    • "should not double-fill in contenteditable with beforeinput handler in Firefox"
  • tests/page/page-keyboard.spec.ts:
    • "should send a character with insertText"
    • "insertText should only emit input event"

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@hbenl hbenl force-pushed the reimplement-sendtext branch from f59dcb9 to 1343e6d Compare July 6, 2026 13:54
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@yury-s yury-s left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good except for putting Bidi-specific script into a separate extension.

This implementation tries to emulate CDP's Input.insertText behavior more closely.

Are there any plans to have such method in Bidi protocol?

Comment thread packages/injected/src/injectedScript.ts Outdated
return 'needsinput'; // Still need to input the value.
}

insertText(text: string): Element | undefined {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is only used in Bidi, it should go in its own script/module. You can call browserContext.extendInjectedScript() to add extra logic.

@hbenl hbenl force-pushed the reimplement-sendtext branch from 1343e6d to e25d3f6 Compare July 7, 2026 10:21
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Test results for "MCP"

2 failed
❌ [firefox] › mcp/cli-killall.spec.ts:42 › kill-all kills filtered dashboard pid @mcp-ubuntu-latest-firefox
❌ [firefox] › mcp/annotate.spec.ts:230 › should capture annotations via show --annotate @mcp-macos-latest-firefox

7707 passed, 1235 skipped


Merge workflow run.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

6 flaky ⚠️ [chromium-library] › library/chromium/chromium.spec.ts:434 › should produce network events, routing, and annotations for Service Worker (advanced) `@frozen-time-library-chromium-linux`
⚠️ [chromium-library] › library/tracing.spec.ts:429 › should produce screencast frames fit `@frozen-time-library-chromium-linux`
⚠️ [chromium-library] › library/beforeunload.spec.ts:130 › should support dismissing the dialog multiple times `@chromium-ubuntu-22.04-arm-node20`
⚠️ [chromium-library] › library/chromium/oopif.spec.ts:150 › should take screenshot `@chromium-ubuntu-22.04-node24`
⚠️ [chromium-library] › library/video.spec.ts:680 › screencast › should capture full viewport on hidpi `@chromium-ubuntu-22.04-node22`
⚠️ [firefox-page] › page/page-emulate-media.spec.ts:144 › should keep reduced motion and color emulation after reload `@firefox-ubuntu-22.04-node20`

49498 passed, 1164 skipped


Merge workflow run.

@hbenl

hbenl commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

@yury-s I tried using browserContext.extendInjectedScript() but it breaks the test "should avoid side effects upon disconnect". Even without this PR, after adding

promises.push(this.extendInjectedScript(`module.exports = { default: () => class C {} };`));

to BidiBrowserContext.initialize() or to CRBrowserContext.initialize() (verifying that this is not a Firefox/Bidi specific issue), the test fails.

return 'needsinput'; // Still need to input the value.
}

bidiInsertText?: (text: string) => Element | undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we avoid adding this to the main script? packages/injected/src/recorder/pollingRecorder.ts adds a rich API and doesn't leak any of it into the main InjectedScript, let's employ the same approach here.

@yury-s

yury-s commented Jul 8, 2026

Copy link
Copy Markdown
Member

@yury-s I tried using browserContext.extendInjectedScript() but it breaks the test "should avoid side effects upon disconnect". Even without this PR, after adding

promises.push(this.extendInjectedScript(`module.exports = { default: () => class C {} };`));

to BidiBrowserContext.initialize() or to CRBrowserContext.initialize() (verifying that this is not a Firefox/Bidi specific issue), the test fails.

Hmm, doesn't fail for me when I run the test with this change (in chromium). Are you sure there are no other changes?

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.

2 participants