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

Prompt handler #681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Prompt handler #681

wants to merge 1 commit into from

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Mar 8, 2024

@jgraham
Copy link
Member Author

jgraham commented Mar 8, 2024

whatwg/html#10189 is the HTML side of this change. It also depends on landing w3c/webdriver#1791

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

@jgraham what is the current status of this and the WhatWG PRs? Both are still marked as draft. Thanks.

@@ -1550,7 +1553,7 @@ session.CapabilityRequest = {
? browserVersion: text,
? platformName: text,
? proxy: session.ProxyConfiguration,
? webSocketUrl: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

This removal is unrelated. Maybe we should do it in a separate PR? I'm mentioning it here because it's also still included in the returned capabilities, which doesn't seem to be needed as well.

@jgraham jgraham closed this Apr 9, 2024
@jgraham jgraham reopened this Apr 9, 2024
@jgraham jgraham marked this pull request as ready for review April 9, 2024 09:03
The key part of this is that with BiDi HTML calls into the spec when
it's going to show a prompt. So at that point we can see if there's a
handler defined, and if so return the handler to HTML.

All the actual logic to handle the dialogs ends up in the HTML spec.

From the BiDi point of view, the intended lifecycle is that you always
get the session.userPromptOpened and session.userPromptClosed events,
even if the prompt is automatically handled. But the
session.UserPromptOpened event gets a new `handler` property that
tells you whether the prompt will be automatically handled; if this is
set to "none" the automation has to send a session.handleUserPrompt
command (or wait for the prompt to be dismissed by the user).
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @jgraham. But I decided to wait a bit with further feedback until the general handling of the capability has finished. I've a couple of questions that I've added inline. Maybe you can have a look at these? Thanks.

@@ -1550,7 +1554,7 @@ session.CapabilityRequest = {
? browserVersion: text,
? platformName: text,
? proxy: session.ProxyConfiguration,
? webSocketUrl: bool,
? userPromptHandler: session.UserPromptHandler,
Copy link
Contributor

Choose a reason for hiding this comment

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

The capability is named unhandledPromptBehavior.

[=Remote end definition=] and [=local end definition=]

<pre class="cddl remote-cddl local-cddl">
session.UserPromptHandler = session.UserPromptHandlerType /
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also keep support for the string type capability given that accept and notify and dismiss and notify are also valid options, and those don't match with the UserPromptHandlerConfiguration type.

@@ -1741,6 +1767,7 @@ This is a [=static command=].
setWindowRect: bool,
userAgent: text,
? proxy: session.ProxyConfiguration,
? userPromptHandler: session.UserPromptHandler,
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs to be unhandledPromptBehavior similar to the above mentioned case.

message: text,
type: "alert" / "confirm" / "prompt" / "beforeunload",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there actually a reason why we didnt' add the type to the browsingContext.UserPromptClosedParameters as well? I think it would be good to know for the client which type of alert was closed.

@@ -4422,8 +4452,9 @@ closed</dfn> steps given |window|, |accepted| and optional |user text|

browsingContext.UserPromptOpenedParameters = {
context: browsingContext.BrowsingContext,
type: "alert" / "confirm" / "prompt" / "beforeunload",
handler: "accept" / "dismiss" / "ignore",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also inform the client about the notify flag?

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.

None yet

2 participants