chore(server): pass action point/box as onBeforeInputAction args#40695
Open
dgozman wants to merge 5 commits intomicrosoft:mainfrom
Open
chore(server): pass action point/box as onBeforeInputAction args#40695dgozman wants to merge 5 commits intomicrosoft:mainfrom
dgozman wants to merge 5 commits intomicrosoft:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pavelfeldman
reviewed
May 7, 2026
| } as const; | ||
|
|
||
| export interface InputActionObserver { | ||
| onBeforeInputAction(progress: Progress, target: Page | ElementHandle, point?: types.Point, box?: types.Rect): Promise<void>; |
Member
There was a problem hiding this comment.
I like our attribution, instrumentation, etc on the server side as a server-side implementation detail. You mostly touching the server here. The only thing that falls off it potentially, is tracing, but for that we need to pass tracing sink into the context.
Replace `Instrumentation.onBeforeInputAction` with a dedicated `InputActionObserver` registered on `BrowserContext`. The observer receives `(progress, target)` where target is `Page | ElementHandle`. `Tracing`, `Debugger`, and `Screencast` register themselves on the context and remove themselves on close/dispose.
Pass the optional bounding box as a third argument to `performOnBeforeInputAction`. Pointer actions in `dom.ts` already have the box from clickable-point computation; for non-pointer actions on an `ElementHandle`, `Screencast` computes the box itself when it needs one. This also lets `Screencast` stop being an `InstrumentationListener` (its only use of `onBeforeCall` was flipping the now-removed `annotate` flag) and inlines the small `_beforeNonPointerAction` helper at its 5 call sites.
Pass the action point as a fourth argument to `performOnBeforeInputAction(progress, target, point?, box?)`. Mouse, touchscreen, and pointer-action call sites supply it directly; `Screencast` and `Tracing` consume it from the argument instead of reading `metadata.point`. The trace's after-action event no longer carries a point (the input event still does, which is what trace viewer reads). `Recorder` now registers as an `InputActionObserver` and caches points by callId so the overlay can keep drawing the action-point dot.
Debugger registered itself as an InputActionObserver in its constructor, so it became the first observer (BrowserContext creates Debugger before any other observer registers). Its pause is awaited inline, which prevented later observers from running while paused — most importantly, Recorder never got to record the action point in `_actionPoints`, so the `x-pw-action-point` overlay never appeared. The `should highlight pointer, only in main frame` test waits on that overlay and hung forever, keeping its worker alive until the 90-min globalTimeout. Drop Debugger from the observer set and invoke it explicitly at the end of `performOnBeforeInputAction`, after all observers have recorded their state.
dgozman
force-pushed
the
chore-input-action-observer
branch
from
5964b14 to
70ce687
Compare
dgozman
changed the title
Drop the InputActionObserver interface on BrowserContext and put
onBeforeInputAction back on Instrumentation, with the new
(sdkObject, metadata, point?, box?) signature. Tracing, Screencast,
Recorder, and Debugger are once again InstrumentationListeners.
To preserve the run-last semantics for the debugger pause (so observers
like Recorder record action points before a pause blocks), addListener
takes an optional `{ order: 'last' }` and Debugger registers with it.
dgozman
force-pushed
the
chore-input-action-observer
branch
from
70ce687 to
fd7eb0b
Compare
Contributor
Test results for "MCP"6 failed 7025 passed, 1068 skipped Merge workflow run. |
Contributor
Test results for "tests 1"3 flaky41693 passed, 850 skipped Merge workflow run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Instrumentation.onBeforeInputActionnow receives(sdkObject, metadata, point?, box?)instead of readingpoint/boxoffCallMetadata.Tracing,Screencast, andRecorderconsume the arguments directly.annotate,box, andpointfromCallMetadata.Screencastcomputes aboundingBoxitself for non-pointer actions;Recordercaches points bycallId.{ order: 'last' }toInstrumentation.addListenerso theDebuggerpause runs after other listeners — otherwise it blocks the recorder from capturing the action point while paused.