Page.handleJavaScriptDialog previously responded -32000 "No dialog is
showing" regardless of whether a dialog was open, leaving CDP clients
no way to influence the JS-side return value of confirm() / prompt().
PR #2085 wired up the Page.javascriptDialogOpening event but explicitly
deferred the return-value override since true Chrome semantics require
suspending V8 mid-execution.
Add a pre-arm model that fits the auto-dismiss architecture without
runtime suspension: handleJavaScriptDialog stashes {accept, promptText}
on the BrowserContext; when the next JS dialog dispatches the
javascript_dialog_opening notification, the listener pops the stash and
fills it into the dispatch's response output param so Window.confirm /
prompt return the CDP client's choice. Without a pre-arm, headless
auto-dismiss values from PR #2085 are preserved (confirm->false,
prompt->null, alert->void).
Closes#2260
This is to pave the way for introducing a new "Page" container, which will take
over the page lifecycle currently burdening Session. The ultimate goal of that
is to allow the Session to have multiple pages (mostly for better transitions
between pages), which is hard to do now since the Session has so much state.
This rename was aggressive, e.g. currentPage() -> currentFrame() so that, when
the new Page container is added, you won't see "currentPage()" and wonder:
"Does 'currentPage' mean the new Page container, or the Frame (which
used to be called Page)".
Rename page.id -> page._loader_id and propagate the change throughout. This was
my attempt at pretending that page.id (and page._frame_id) weren't CDP-sepcific.
But they are, and it's a lot cleaner to treat them this way. Might seem
unnecessary, but without this, after page -> frame, you'd end up with:
frame.id
frame._frame_id
Which is weird? What is `frame.id` if it isn't the frame id and if that's the
case, what's frame_id? Now it'll be:
frame._loader_id
frame._frame_id
Which removes the ambiguity, makes the CDP code a bit more obvious, and doesn't
try to hide the fact that these are CDP things that, for now at least, pollute
the code a little.
@import("lightpanda") where needed.
Would also like to do this for String, Page, Session and js which all stand out
as types that are use across the codebase.
I know that a few devs are doing this in new work and I haven't heard anyone
voice an objection.
This introduces two slightly related changes.
My understanding is:
- frameId represents the page. Even if the page navigates, it's the same
frameId. We capture this in Page._frame_id. Nothing here changes.
- loaderId is essentially for a specific document of the page. If the page
navigates, it should be a different loaderId. We were using a distinct
loaderId per request. Not sure what problems that caused. But it was wrong.
This was achieved by exposing Page.id to CDP.
- requestId was mostly correct: unique per request. HOWEVER, for the original
document, apparently, requestId == loaderId. This change is particularly
important for various puppeteer and playwrightb behavior. This is a bit
hacked. CDP will look at the resource_type, if it's .document, it'll return
the loaderId, else it returns the requestId it always id.
window.alert(), confirm(), and prompt() now dispatch a
javascript_dialog_opening notification that the CDP layer
forwards as a Page.javascriptDialogOpening event. This enables
Puppeteer's page.on('dialog') to fire when JS dialogs open.
Also adds Page.handleJavaScriptDialog as a CDP method. Dialogs
still auto-dismiss in headless mode (alert is void, confirm
returns false, prompt returns null), so handleJavaScriptDialog
is an acknowledgement rather than a blocking gate.
Changes:
- Notification.zig: add JavascriptDialogOpening event type
- CDP.zig: register listener, forward to page domain
- page.zig: handleJavaScriptDialog handler + event emitter
- Window.zig: alert/confirm/prompt dispatch the notification
Fixes#2082
Ref #2043
These changes all better align with chrome's event ordering/timing.
There are two big changes. The first is that our internal page_navigated event,
which is kind of our heavy hitter, is sent once the header is received as
opposed to (much later) on document load. The main goal of this internal event
is to trigger the "Page.frameNavigated" CDP event which is meant to happen
once the URL is committed, which _is_ on header response.
To accommodate this earlier trigger, new explicit events for DOMContentLoaded
and load have be added.
This drastically changes the flow of events as things go from:
Start Page Navigation
Response Received
Start Frame Navigation
Response Received
End Frame Navigation
End Page Navigation
context clear + reset
DOMContentLoaded
Loaded
TO:
Start Page Navigation
Response Received
End Page Navigation
context clear + reset
Start Frame Navigation
Response Received
End Frame Navigation
DOMContentLoaded
Loaded
So not only does it remove the nesting, but it ensures that the context are
cleared and reset once the main page's navigation is locked in, and before any
frame is created.
This field was recently added and is used to generate correct frameIds in CDP
messages. They remain the same during a navigation event, so calling them
page.id might cause surprises since navigation events create new pages, but
retain the original id. Hence, frame_id is more accurate and hopefully less
surprising.
(This is a small cleanup prior to doing some iframe navigation work).
Our BrowsingContext currently supports 1 target. So we have a per-BC target_id.
Previously, our target had 1 "frame" - our page. So we often treated the
targetId as the frameId. But to work with frames, we need page-specific
frameIds and loaderIds.
This tries to clean up our ids (a little). frameIds are now ids derived from
a new incrementing page.id. This page.id has to be passed around (via http
Requests and through notifications) in order to properly generate messages with
a frameId.
Deciding what should be an lp.assert, vs an std.debug.assert, vs a debug-only
assert is a little arbitrary.
debug-only asserts, guarded with an `if (comptime IS_DEBUG)` obviously avoid the
check in release and thus have a performance advantage. We also use them at
library boundaries. If libcurl says it will always emit a header line with a
trailing \r\n, is that really a check we need to do in production? I don't think
so. First, that code path is checked _a lot_ in debug. Second, it feels a bit
like we're testing libcurl (in production!)..why? A debug-only assertion should
be good enough to catch any changes in libcurl.
