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

User prompt handler capabilities #1791

Merged
merged 9 commits into from
Apr 3, 2024
Merged

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Feb 9, 2024

@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch 2 times, most recently from 2339bb9 to 678b6ab Compare February 9, 2024 13:53
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch from 678b6ab to ab13ac3 Compare February 20, 2024 11:32
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch from ab13ac3 to 8c63611 Compare March 5, 2024 11:31
@jgraham jgraham changed the base branch from master to apostrophy March 5, 2024 11:35
@jgraham jgraham marked this pull request as ready for review March 5, 2024 11:35
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch 2 times, most recently from 225c89e to c5ecc04 Compare March 5, 2024 12:09
Base automatically changed from apostrophy to master March 5, 2024 12:09
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch 2 times, most recently from 5cb81e1 to c3901cd Compare March 5, 2024 12:16
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
<dl class=switch>
<dt><a>dismiss state</a>
<dd><p><a>Dismiss</a> the <a>current user prompt</a>.
<li><p>If <var>type</var> is "<code>beforeUnload</code>", return a
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that we can move this up given that it doesn't depend on any user prompt handler setting.

Copy link
Member Author

@jgraham jgraham Mar 11, 2024

Choose a reason for hiding this comment

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

I don't follow? If type is beforeUnload and you have beforeUnload in handlers the previous step applies. That's important for BiDi where we don't totally override the beforeUnload handler, but still default to dismissing it if you didn't add it in capabilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good point. Would it make sense to add a note here which explains why it needs to stay here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to the note above.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch from c3901cd to 0bce1f7 Compare March 8, 2024 20:54
jgraham and others added 5 commits March 11, 2024 13:49
The idea is that for BiDi we'll accept objects like:

{
  "alert": "dismiss",
  "beforeUnload": "ignore"
}

to allow different handling per prompt type, whilst falling back to
the appropiate defaults to maintain the current behaviour for
implementations that don't support BiDi.
@jgraham jgraham force-pushed the user_prompt_handler_capabilities branch from d6b81f5 to c5dfabc Compare March 11, 2024 13:52
@jgraham jgraham requested a review from whimboo March 12, 2024 09:34
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.

Thanks @jgraham! That looks fine to me now.

@sadym-chromium and maybe @gsnedders could you review as well please?

@whimboo whimboo requested a review from gsnedders March 12, 2024 17:38
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed User prompt handlers.

The full IRC log of that discussion <AutomatedTester> topic: User prompt handlers
<AutomatedTester> github: https://github.com//pull/1791
<jgraham> I note that is a position on whether Invalid Argument is only for things that can be detected from the schema; if we reject files for not existing then it isn't. So if we want that property we should invent a new error.
<AutomatedTester> jgraham (IRC): after working through handlers for user prompts in classic which shoulnd't change how we do things
<AutomatedTester> ... and now have something up for bidi
<AutomatedTester> ... in bidi we want to be notified before unload where in classic it was automatically handled
<AutomatedTester> ... whimboo has looked at this and since it's a new feature it would be good if other vendors can have a look
<AutomatedTester> ... one of the future items is that we can have the file dialog is handled this way
<AutomatedTester> ... as well as the classic PR there are also draft PRs on HTML and WebDriver Bidi
<AutomatedTester_> q?

@OrKoN
Copy link
Contributor

OrKoN commented Mar 19, 2024

Could you please add a short summary about the implications of the change? it's not expected to affect the current WebDriver Classic behavior, right?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
The specifications allow one prompt per event loop, so instead of
assuming a single current user prompt, allow getting the user
prompt for the active browsing context's event loop.
@jgraham
Copy link
Member Author

jgraham commented Mar 25, 2024

This is specifically worded to not change classic behaviour at all, at least in a mandatory way.

The new feature in the PR is the ability to define per-prompt-type behaviour e.g.

userPromptHandler: {
  alert: "accept",
  confirm: "cancel",
  prompt: "ignore"
}

That's not terribly useful just for classic as is, but it's more useful for BiDi to be able to do things like:

userPromptHandler: {
  beforeUnload: "ignore",
  default: "accept"
}

Which would mean that you get an event and have to explicitly handle beforeUnload prompts, but other kinds of prompts are auto-accepted. It's also likely to be useful in the future for file-type prompts which are rather likely to have different behaviour from other prompt types.

Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

LGTM

@jgraham jgraham merged commit 43903d0 into master Apr 3, 2024
2 checks passed
@jgraham jgraham deleted the user_prompt_handler_capabilities branch April 3, 2024 11:52
github-actions bot added a commit that referenced this pull request Apr 3, 2024
SHA: 43903d0
Reason: push, by jgraham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to xjc90s/webdriver that referenced this pull request Apr 4, 2024
SHA: 43903d0
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to soloinovator/webdriver that referenced this pull request Apr 6, 2024
SHA: 43903d0
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@whimboo
Copy link
Contributor

whimboo commented Apr 10, 2024

userPromptHandler: {
alert: "accept",
confirm: "cancel",
prompt: "ignore"
}

It was actually not intended to add a new capability with the name userPromptHandler. Instead it should look like:

unhandledPromptBehavior: {
  alert: "accept",
  confirm: "cancel",
  prompt: "ignore"
}

With a future use of:

unhandledPromptBehavior: {
  beforeUnload: "ignore",
  default: "accept"
}

I created PR #1808 to get this fixed.

github-actions bot added a commit that referenced this pull request May 21, 2024
SHA: 48a221b
Reason: push, by whimboo

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to xjc90s/webdriver that referenced this pull request May 22, 2024
SHA: 48a221b
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to soloinovator/webdriver that referenced this pull request May 24, 2024
SHA: 48a221b
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants