fix(actions): click dispatches full pointer/mouse sequence, not a bare click event by R4m1r0qu41 · Pull Request #3431 · lightpanda-io/browser

R4m1r0qu41 · 2026-09-07T05:17:12Z

`actions.click()` previously dispatched a single untrusted-shaped `click` `MouseEvent` directly on the target node, skipping `pointerdown`, `mousedown`, `pointerup`, and `mouseup` entirely. Many real-world widgets — custom autocomplete/combobox components in particular — open or otherwise react on `mousedown`, not `click` alone, so this made them unreachable through the click tool even though the element was correctly focused and targeted.

`WebDriver.zig`'s own `click()` already implements the correct sequence (`pointerdown`, `mousedown`, `pointerup`, `mouseup`, `click`) for testdriver's click, with a comment explicitly contrasting it against a lone untrusted click event. This change ports that same sequence into `actions.click()` so the MCP/CDP "click" action produces the same event sequence a real user click would.

Adds a `mousedown` assertion to the existing MCP Actions test (`mcp_actions.html` + `tools.zig`) to catch a regression here; confirmed the new assertion fails against the old implementation and passes against this one. Full test suite (1339 tests) and `zig fmt --check` both pass.

Reproduced against a real, previously-untested production site (a Wix-built autocomplete branch-selector widget) where `click` could focus the input but never open its option list; a minimal local reproduction (a mousedown-only widget) confirms the fix.

…e click event actions.click() previously dispatched a single untrusted-shaped "click" MouseEvent directly on the target node, skipping pointerdown, mousedown, pointerup, and mouseup entirely. Many real-world widgets — custom autocomplete/combobox components in particular — open or otherwise react on mousedown, not click alone, so this made them unreachable through the click tool even though the element was correctly focused and targeted. WebDriver.zig's own click() already implements the correct sequence (pointerdown, mousedown, pointerup, mouseup, click) for testdriver's click, with a comment explicitly contrasting it against a lone untrusted click event. This change ports that same sequence into actions.click() so the MCP/CDP "click" action produces the same event sequence a real user click would. Adds a mousedown assertion to the existing MCP Actions test (mcp_actions.html + tools.zig) to catch a regression here; confirmed the new assertion fails against the old implementation and passes against this one. Full test suite (1339 tests) and zig fmt --check both pass. Reproduced against a real, previously-untested production site (a Wix-built autocomplete branch-selector widget) where click could focus the input but never open its option list; a minimal local reproduction (a mousedown-only widget) confirms the fix.

github-actions · 2026-09-07T05:17:24Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

R4m1r0qu41 · 2026-09-07T05:20:22Z

I have read the CLA Document and I hereby sign the CLA

arrufat

Thanks, this is the right direction: the MCP click should produce the same gesture a real user does, and WebDriver.click is the right reference. A few things need to change before merging though, mostly guards that the WebDriver version has and this port drops.

Blocking

Disabled controls now activate. WebDriver.click (and HtmlElement.click in element/Html.zig) start with:

if (element.is(Element.Html)) |html| {
    switch (html._type) {
        .button, .input, .textarea, .select, .option, .optgroup => if (element.isDisabled()) return,
        else => {},
    }
}

Without it, setChecked on a disabled checkbox toggles it and fires change, and a disabled button's mousedown handler runs. Please port the guard too.

A cancelled pointerdown must suppress mousedown/mouseup. Per Pointer Events, preventDefault() on pointerdown suppresses the compatibility mouse events (click still fires). Radix / Headless UI style triggers depend on this: they open on pointerdown, call preventDefault(), and close on a document-level mousedown. With this PR that combobox opens and immediately closes again. press() in the same file already shows the pattern: acquireRef() the event before dispatch, defer releaseRef, then check getDefaultPrevented() and skip the two mouse events when set.

mousedown has no focus default action. The CDP path (frame/user_input.zig, right after triggerMousePress dispatches mousedown) and WebDriver.actionSequence both call Frame.user_input.focusEditingHostForMouseDown. Here nothing does, so clicking a contenteditable editor or a div[tabindex=0] never focuses it and a following press goes to <body>.

Test coverage. The only new assertion is window.mousedowned === true. Dropping both pointer events, or reordering click before mouseup, keeps the test green. Please record the whole sequence in mcp_actions.html, e.g.

window.seq = [];
for (const t of ['pointerdown','mousedown','pointerup','mouseup','click'])
  btn.addEventListener(t, e => window.seq.push([e.type, e.button, e.buttons, e.isTrusted].join(':')));

and assert the exact five entries in tools.zig.

Should fix

Hover step first. Both existing paths call Frame.user_input.updateHoverTarget(frame, el, .{ .with_pointer = true }) before pressing, so pointerover/mouseenter fire and the hover target is updated. Add it before the first pointerdown.
Dispatch click as a PointerEvent. The engine already does this for HTMLElement.click() and keyboard clicks (pointerType: "mouse", pointerId: 1). With a plain MouseEvent, click.pointerType is undefined while pointerdown.pointerType is "mouse" in the same gesture.
pressure. Chrome reports 0.5 on a pressed mouse pointerdown. .pressure = if (buttons != 0) 0.5 else 0.0 in dispatchPointer.

Nits

button: i32 is always 0; drop the parameter.
Both helpers duplicate the dispatch/catch block and log "click failed" for every event type. One dispatch(el, event, comptime typ, frame) wrapper logging "click " ++ typ ++ " failed" is enough.
.clientX = 0, .clientY = 0 restate the defaults.
The comment above click repeats the PR description. A single /// line saying why (widgets key off mousedown/pointerdown, not click) is enough, and the file is webapi/WebDriver.zig, not WebDriver.zig.

Longer term this is now the third copy of the same sequence (WebDriver.click, user_input.triggerMousePress/Release, and this one), and they already disagree on the points above. Not asking for it in this PR, but a shared pub fn in frame/user_input.zig that all three call would be the real fix, and would also give Puppeteer/Playwright clicks pointerdown, which they currently lack.

Round-1 review feedback on the pointerdown/mousedown/pointerup/mouseup/ click sequence added in 3e8ff14: - Guard disabled elements up front (el.isDisabled()), matching the checks WebDriver.click and HtmlElement.click already have — otherwise a disabled checkbox/button still activates. - A cancelled pointerdown now suppresses both mousedown and mouseup for the gesture (one check, not two independent ones — see below). - mousedown now calls Frame.user_input.focusEditingHostForMouseDown, matching the CDP path in frame/user_input.zig, so clicking a contenteditable host or tabindex target actually focuses it. - updateHoverTarget(..., .{ .with_pointer = true }) fires before the first pointerdown, matching the hover step both other click paths take. - click is now dispatched as a PointerEvent (pointerType "mouse", pointerId 1), matching HTMLElement.click(), with pressure 0.5 while a button is down. - Dropped the always-0 `button` param from both dispatch helpers, merged the duplicated dispatch/log code into one dispatch() helper, dropped the restated clientX/clientY = 0 defaults, trimmed the doc comment. Test coverage: the click assertion in tools.zig now checks the exact five-event sequence (type:button:buttons:pointerType:isTrusted) instead of a single boolean. Two new fixtures in mcp_actions.html close the gaps the weaker test couldn't see: a button whose pointerdown listener calls preventDefault() (asserts the sequence collapses to exactly pointerdown, pointerup, click — no mousedown/mouseup), and a disabled button (asserts mousedown never fires). The first draft of the suppression fix wrongly gated mouseup on pointerup's own preventDefault() instead of pointerdown's; the new preventDefault fixture catches that regression too, confirmed by reintroducing it and watching the assertion fail before restoring the correct version. Full suite (1339 tests) and zig fmt --check pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

focusEditingHostForMouseDown only walked contenteditable hosts, so a div[tabindex=0] stayed unfocused after click. Replace it with focusForMouseDown: still prefer the outermost editing host, then focus the nearest mouse-focusable element (including tabindex=-1). Call sites: actions.click, CDP triggerMousePress, WebDriver pointerDown. Tests cover the MCP selector path, agent Page.click on the HTML fixture, and a runtime-created child click (ancestor walk, non-focusable must not steal focus, tabindex=-1 is mouse-focusable).

R4m1r0qu41 · 2026-09-08T06:18:00Z

Thanks for the thorough review — addressed everything below.

Disabled elements: click() now guards on el.isDisabled() up front,
so a disabled button/checkbox/etc. no-ops instead of firing handlers.

Cancelled pointerdown: a single suppress_mouse flag derived from
pointerdown's preventDefault() now gates both the compatibility
mousedown and mouseup — so a Radix/Headless-UI-style trigger that opens
on pointerdown and closes on a document mousedown no longer
immediately closes itself.

Mousedown focus: mousedown now runs the same default action as the
CDP path — focuses the contenteditable host if there is one, otherwise the
nearest mouse-focusable element (any explicit tabindex, including -1,
counts). Caught in review that my first pass only handled the
contenteditable half; a div[tabindex=0] was still left unfocused. Fixed
by broadening that helper (focusForMouseDown) rather than special-casing
it in click() — it's shared with WebDriver.click and the CDP path.

Hover, click as PointerEvent, pressure, and the nits: all done —
updateHoverTarget before the first pointerdown, click dispatched as a
PointerEvent matching HTMLElement.click(), pressure set while a
button is down, unused param dropped, dispatch/log helpers merged, and the
stale file reference in the doc comment fixed.

Test coverage: the click assertion now pins the exact 5-event sequence
(type/button/buttons/pointerType/isTrusted), plus new fixtures for a
preventDefault()-on-pointerdown widget, a disabled button, and a
tabindex target (with a non-focusable sibling to confirm a plain click
can't steal focus onto something that shouldn't take it). Full suite
(1341 tests) and zig fmt --check clean.

On the "longer term" point — I went ahead and drafted that shared pub fn
in frame/user_input.zig so WebDriver.click, this PR's actions.click,
and the CDP mousePressed/mouseReleased path all go through the same
three functions instead of three copies. It's rebased onto this branch and
already passing (1342 tests, both build modes), and as a side effect it
also gives Puppeteer/Playwright clicks pointerdown/pointerup for the
first time, and picks up the tabindex-focus fix above for all three
callers instead of just this one. Happy to open it as a separate PR once
this one lands, or sooner if you'd rather look at it in parallel — your
call. A couple of things I deliberately left as open questions rather than
deciding myself (notably whether WebDriver.click's disabled guard should
widen to match actions.click's, and two CDP-path gaps that predate this
work) — I'll lay those out in that PR's description rather than here.

arrufat

Thanks, this round addresses everything from the first pass, and the event sequence now matches real browsers exactly. I drove mcp_actions.html through headless Chrome and Firefox with real (trusted) clicks; both agree on every point below, so the remaining items are places where the fixture asserts something a browser would fail.

Confirmed against Chrome + Firefox: the five-event sequence with button/buttons/pointerType/isTrusted, detail (0/1/0/1/1), pressure (0.5 while pressed), preventDefault() on pointerdown suppressing mousedown/mouseup and the focus change (both browsers leave focus where it was, so skipping focusForMouseDown in the suppressed branch is right), tabindex="-1" being mouse-focusable, and a click on a child focusing its tabindex ancestor.

Blocking

A mousedown outside any focusable element must blur the active element. In both browsers, clicking #focusTarget then #plain leaves document.activeElement === document.body; the fixture asserts focus stays on focusTarget (window.focusAfterPlain === 'focusTarget' in tools.zig, and the "plain click stole focus" check in Runtime.zig). That is the behaviour toolbars have to preventDefault() on mousedown to avoid, and dropdowns that close on blur rely on it. focusForMouseDown should end with:

    // A mousedown outside any focusable element moves focus to the body.
    const doc = target.asNode().ownerDocument(frame) orelse frame.document;
    if (doc._active_element) |active| {
        try active.blur(frame);
    }

and the fixture can record window.plainBlurred = document.activeElement === document.body instead. I ran the full suite with that change and the corrected assertions; nothing else moves, including the CDP path that now shares this helper.

An unparsable tabindex is not focusable. parseInteger(attr) orelse 0 makes <div tabindex="abc"> mouse-focusable; both browsers send focus to <body> for it. return Element.Html.parseInteger(attr) != null; is what you want.

Should fix

mouseFocusTabIndex returns ?i32 but nobody reads the value; fold it into isMouseFocusable(el) bool directly (the two fixes above make the body a plain return chain anyway).
The Runtime.zig test "selector click preserves pointer mouse semantics" re-asserts exactly what the tools.zig test does, over the same tools.zig → actions.click path. Drop it and keep "dynamic tabindex child click focuses ancestor", which covers new ground (runtime-created nodes, child → ancestor walk, tabindex=-1).

Nits

Both browsers still dispatch pointerdown/pointerup on a disabled button (no mouse events, no click, no focus). The early return is fine for the tool if you'd rather keep it simple; if you want to be exact, treat disabled like the suppressed branch and skip click too.
// Dispatches a trusted pointer event… (actions.zig) and // Find the outermost element… (user_input.zig) describe functions, so they should be /// doc comments like the others you added.

Addresses review on the mousedown default action. Blur when a mousedown lands outside any focusable element: focusForMouseDown now blurs the document's active element when the ancestor walk finds nothing mouse-focusable, so a plain click moves focus to the body. The MCP fixture had this backwards and asserted the old behavior; it now records document.activeElement === document.body. Treat an unparsable tabindex as if the attribute were absent (HTML 6.6.3) rather than as a terminal "not focusable" answer, so <button tabindex="abc"> keeps its native focusability. This mirrors HtmlElement.getTabIndex's parse-failure fallthrough for the same attribute instead of introducing a second parser with different semantics. Check the tabindex attribute before the HTML-only guard. An explicit, parseable tabindex is focusable on any element, and SVG links are focusable by the same href they activate on. This was only a gap until the blur above made it a regression: clicking such an element used to leave focus alone and would now have dropped it to the body, where Chrome focuses the element. Honor preventDefault() on mousedown. The default action ran unconditionally, so the toolbar idiom -- preventDefault() on mousedown to keep focus in a focused input -- did not work, and the new blur made that load-bearing. The mouse dispatch helpers in actions.zig, user_input.zig and WebDriver.zig now report whether the event was cancelled, and each call site gates focusForMouseDown on it. Non-mousedown call sites discard the result. Editing-host focus behavior is deliberately unchanged. Fold mouseFocusTabIndex into isMouseFocusable; its ?i32 return was never read. Drop the duplicate Runtime.zig test that drove the same actions.click path as the tools.zig fixture, and expand the remaining one, renamed to reflect what it now covers. Two of its assertions sample document.activeElement in a mouseup listener rather than after the click, because click activation behavior focuses those elements unconditionally and would mask what mousedown decided. Harden the disabled-control test to record the full five-event sequence instead of mousedown alone. Every assertion was checked to fail against the pre-fix code. zig build test 1340/1340, zig build test -Dwpt_extensions 1340/1340, zig fmt --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

R4m1r0qu41 · 2026-09-11T07:11:24Z

Thanks — both blocking items are fixed, plus two things found while testing them.

Blur on a mousedown outside any focusable element. Done as suggested. focusForMouseDown now blurs the document's active element when the ancestor walk finds nothing mouse-focusable. You were right that the fixture had it backwards — #plain now records document.activeElement === document.body, and tools.zig asserts plainBlurred === true.

Unparsable tabindex. Fixed, but I didn't take the one-liner literally and want to flag why. return parseInteger(attr) != null; makes the tabindex branch terminal, so <button tabindex="abc"> loses native focusability. Per HTML §6.6.3 an unparsable value is treated as if the attribute were absent, so the native path should still apply — and HtmlElement.getTabIndex already implements that exact fallthrough for the same attribute. I matched it rather than adding a second parser with different semantics:

if (el.getAttributeSafe(comptime .wrap("tabindex"))) |attr| {
    if (Element.Html.parseInteger(attr)) |_| {
        return true;
    }
}
// falls through to the native-tag switch

<div tabindex="abc"> still lands on else => false, so your #dynBad case holds.

Should-fix: mouseFocusTabIndex is folded into isMouseFocusable, and the duplicate Runtime.zig test is gone. Nits: both comments are now ///. I left the disabled-button pointer events as-is, since you said either way was fine.

The preventDefault()-on-mousedown escape hatch didn't work. The mousedown default action never checked defaultPrevented, so a focused input was still blurred by a click on a preventDefault-ing toolbar button — the exact idiom you mentioned in the review. That predates this PR for the focus case, but the new blur is what made it matter. The three dispatchMouse/dispatchMouseEventOn helpers now report cancellation, and each call site gates focusForMouseDown on it. Happy to split this into its own PR if you'd rather keep this one narrow.

An explicit tabindex on a non-HTML element was unreachable. isMouseFocusable returned false for every non-HTML element before checking tabindex, so <rect tabindex="0"> could never be focused. On its own that was only a gap — but the new blur turned it into a regression: such a click used to leave focus alone and would now drop it to the body, where Chrome focuses the element. The tabindex check moved above the HTML-only guard, and SVG <a href> is handled there too, reusing the existing svgAnchorHref since it has the same story.

On the tests: two assertions sample document.activeElement in a mouseup listener rather than after the click, because click activation behavior focuses those elements unconditionally and would otherwise mask what mousedown decided. Every assertion in this round was checked to fail against the pre-fix code. Editing-host focus behavior is deliberately unchanged by this PR.

zig build test 1340/1340, zig build test -Dwpt_extensions 1340/1340, zig fmt --check clean.

Separately, and not for this PR: while testing the blur I ran a set of trusted-input probes against Chrome 152 over CDP, and mouse focus inside editing hosts diverges from what we do in a few measured ways.

The parts we already match: a host takes focus over a <button>, [tabindex] div or <a href> inside it, and a nested host resolves to the outer one. The parts we don't: <input>, <select> and <textarea> inside a host focus themselves in Chrome, as does anything inside a contenteditable="false" island — we focus the host in all four cases.

Worth flagging that the obvious rule doesn't work, since it cost me a detour: isContentEditable is true for the <input> case and the <button> case alike, so it can't be the discriminator. The line seems to be whether the element's contents become editable text — which cuts across the form-control category, <button> behaving like a span.

All of that is pre-existing and untouched here. If this round lands, I'm happy to send the full probe table and a follow-up PR implementing it, with a regression per case. Two other pre-existing items would fit naturally in the same change: findClickActivationTarget checks hasClickActivationBehavior(target) before walking ancestors for a host, so a direct <a href> inside contenteditable still navigates where Chrome doesn't; and isEditingHost accepts any value but false, so contenteditable="bogus" is treated as a host where Chrome treats the invalid token as inherited and blurs to body. Only if you want them — I'd rather not widen this PR.

🤖 Generated with Claude Code

Extract EventManager.dispatchCancelable so the acquire-ref/dispatch/read- defaultPrevented pattern lives in one place instead of four copies across actions.zig, user_input.zig and WebDriver.zig. Share a single isNativelyFocusable predicate between the mouse-focus rules and moveFocus, which had the same tag switch verbatim, and fold the SVG-link check into isSvgLink. Return early from Element.focus when the element is already active, before the visibility walk, so a click no longer pays a CSS ancestor walk to re-focus a native control. Collapse the repeated MCP click-test blocks into a selector loop and give the runtime focus test active()/expectActive() helpers.

…ll-mouse-sequence # Conflicts: # src/browser/frame/user_input.zig # src/browser/webapi/Element.zig

…im comments Route the mouse-wheel, keypress and press paths through the new EventManager.dispatchCancelable instead of open-coding the acquire-ref / dispatch / read-defaultPrevented dance three more times. Use getAttributeInterned for href/tabindex in the focusability helpers to match their neighbors, and drop WHAT-restating comments.

arrufat · 2026-09-11T08:53:45Z

Thanks for the thorough back-and-forth on this, @R4m1r0qu41. The event sequence and the focus rules ended up in great shape, and I checked the fixture against Chrome and Firefox along the way.

The branch had drifted a fair way behind main, so I merged main in directly. That surfaced a couple of things I fixed on the branch:

The CI break was a stale rename: the new runtime test referenced CDPNode.Registry, which main renamed to NodeRegistry.
Two merge conflicts against work main landed independently. moveFocus now uses main's new Element.focusTabIndex(), and focus() keeps your already-active early return but goes through main's Document.setActiveElement() (which also flags a style change).

While I was in here I took the liberty of simplifying a bit:

folded the acquire-ref / dispatch / read-defaultPrevented dance into a single EventManager.dispatchCancelable, and routed the wheel / keypress / press paths through it too;
shared the native-focusable tag switch between the mouse and Tab paths;
trimmed a few narrative comments.

I deliberately left the bigger structural items for the follow-up you offered rather than widening this PR: the shared click-sequence function, and unifying isMouseFocusable with focusTabIndex (they still disagree on SVG and unparsable tabindex). I wrote those up so they don't get lost.

The agent-script tests only drive actions.click, so Input.dispatchMouseEvent's own gate had no coverage: deleting the `if (!suppressed)` in triggerMousePress left the whole suite green. Press on a preventDefault-ing element and assert focus is kept, then press a focusable one and assert focus moves, so the test pins the gate rather than an inert default action. Verified to fail when the gate is removed. The WebDriver path (performPointerSource) is still uncovered; WebDriver.zig has no unit tests and is exercised through WPT testdriver, so that one needs a harness rather than another test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

arrufat self-assigned this Sep 7, 2026

arrufat requested changes Sep 7, 2026

View reviewed changes

R4m1r0qu41 and others added 2 commits September 7, 2026 13:42

arrufat requested changes Sep 10, 2026

View reviewed changes

arrufat added 3 commits September 11, 2026 10:35

Merge remote-tracking branch 'origin/main' into fix/click-dispatch-fu…

0e04a2f

…ll-mouse-sequence # Conflicts: # src/browser/frame/user_input.zig # src/browser/webapi/Element.zig

arrufat approved these changes Sep 11, 2026

View reviewed changes

arrufat requested a review from karlseguin September 11, 2026 09:14

karlseguin merged commit d812361 into lightpanda-io:main Sep 12, 2026
25 checks passed

github-actions Bot locked and limited conversation to collaborators Sep 12, 2026

Navigation Menu

fix(actions): click dispatches full pointer/mouse sequence, not a bare click event - #3431

fix(actions): click dispatches full pointer/mouse sequence, not a bare click event#3431
karlseguin merged 8 commits into
lightpanda-io:mainfrom
R4m1r0qu41:fix/click-dispatch-full-mouse-sequence

R4m1r0qu41 commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

R4m1r0qu41 commented Sep 7, 2026

Uh oh!

arrufat left a comment

Uh oh!

R4m1r0qu41 commented Sep 8, 2026

Uh oh!

arrufat left a comment

Uh oh!

R4m1r0qu41 commented Sep 11, 2026

Uh oh!

arrufat commented Sep 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

R4m1r0qu41 commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

R4m1r0qu41 commented Sep 7, 2026

Uh oh!

arrufat left a comment

Choose a reason for hiding this comment

Blocking

Should fix

Nits

Uh oh!

R4m1r0qu41 commented Sep 8, 2026

Uh oh!

arrufat left a comment

Choose a reason for hiding this comment

Blocking

Should fix

Nits

Uh oh!

R4m1r0qu41 commented Sep 11, 2026

Uh oh!

arrufat commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Sep 7, 2026 •

edited

Loading

arrufat commented Sep 11, 2026 •

edited

Loading