Implement `input.FileDialogOpened` #568

jrandolf-2 · 2023-10-09T12:37:26Z

This PR implements the event portion of file dialog flow.

Relevant HTML spec: whatwg/html#9844

Issue: #494
Input.setFiles PR: #514

Preview | Diff

jgraham

So, does this require a patch in HTML at https://html.spec.whatwg.org/#the-input-element-as-a-file-upload-control ?

How are you imagining the return value would be used?

jgraham · 2023-10-27T14:23:04Z

Oh, sorry I missed that there is an HTML patch, I'll read that :)

jgraham · 2023-10-27T15:04:33Z

So generally a concern I have at the moment is that we don't have any mechanism to specify the behaviour when the dialog is intercepted. I slightly worry that the default here being different to the default for alerts is confusing (with alerts we actually show the alert, even if there's an event, and rely on the subscriber sending a command to continue. If possible I think we ought to do that by default in this case as well, but also make it configurable in some way).

jrandolf-2 · 2023-10-28T00:35:42Z

@jgraham The problem with that approach is the blocking behavior of file dialogs, so you cannot set multiple file choosers in parallel.

IIRC, there was a consensus in TPAC to not allow that blocking behavior for file dialogs. Perhaps @OrKoN can confirm.

OrKoN · 2023-10-30T08:46:21Z

So generally a concern I have at the moment is that we don't have any mechanism to specify the behaviour when the dialog is intercepted. I slightly worry that the default here being different to the default for alerts is confusing (with alerts we actually show the alert, even if there's an event, and rely on the subscriber sending a command to continue. If possible I think we ought to do that by default in this case as well, but also make it configurable in some way).
IIRC, there was a consensus in TPAC to not allow that blocking behavior for file dialogs.

I think we touched upon that at TPAC and the idea is that file dialogs might not actually block the execution on the page so it is possible for the user to continue doing things (in the background via scripts and trigger other dialogs) while the dialog is showing that might have unpredictable results. It is different from the alerts where the loop is actually blocked while waiting for the alert resolution. We also discussed that some user agents might not implement a dialog at all.
So, the current version would be preferred from the Puppeteer's perspective.

@jimevans @shs96c do you have insights into what would be preferred by Selenium? I feel like if the client opts into intercepting the dialogs programmatically, there might be no need to show it on screen. WDYT?

jgraham · 2023-10-30T09:37:09Z

Conceptually file dialogs are similar to alert-type prompts, so I'm keen to make sure we don't end up with different behaviour in each case.

With alerts we currently send the client an event, and you're required to respond with a command to dismiss the alert (and provide the return value for the types where that makes sense).

The proposal here is that with file prompts opting in to getting the event means that you don't get a prompt at all, but only get an event, and can provide the value asynchronously if required. Technically that does make some sense (an alert is fundamentally synchronous, whereas you can provide an input to a file dialog at any time), but it's not clear whether or not that difference is enough to justify having a totally different model compared to alerts.

I'm also not sure about just subscribing to an event having side effects on browser behaviour. It is true that with e.g. network request interception we don't block the request if it matches a pattern but there isn't any session with a subscription to the relevant event. So there is some precedent there. But it also seems meaningfully different that interception is itself a webdriver feature, and without someone subscribing to the event it would be possible for anyone (even a human interacting with the browser) to unblock the request.

lukewarlow · 2023-11-11T22:58:30Z

Why does this only intercept the show picker algorithm for file inputs? Seems like it would be useful to provide a way to intercept all the show picker algorithms?

jrandolf-2 · 2023-11-16T12:11:39Z

Why does this only intercept the show picker algorithm for file inputs? Seems like it would be useful to provide a way to intercept all the show picker algorithms?

For now, we are limiting the scope of this PR to file inputs.

OrKoN · 2023-11-21T14:05:49Z

I have updated the PR to include the capability processing to reflect the latest discussion at the working group. PTAL

whimboo · 2023-11-24T08:33:55Z

index.bs

@@ -1400,6 +1400,7 @@ for a session.
 <pre class="cddl remote-cddl local-cddl">
 session.CapabilityRequest = {
  ? acceptInsecureCerts: bool,
+  ? interceptFileDialogs: bool,


By having such a capability I wonder how we best separate those which belong to different protocols. Right now some of the capabilities that we have are used in both classic and bidi, but others are classic only. Now we get bidi only capabilities. All that doesn't make it easy. I wonder if we should actually re-use unhandledPromptBehavior for bidi as well, or if we want to make the dialog handling dependent on subscribed events.

I think it's already separated because the BiDi spec defines these as additional capabilities. Or do you mean how to separate them in the CDDL definitions?

@whimboo @jgraham What do you think? The concept of “additional WebDriver capabilities” is defined in the Classic spec, but the WebDriver BiDi spec makes it clear that these additional WebDriver capabilities in particular are BiDi-only:

WebDriver BiDi defines additional WebDriver capabilities. The following tables enumerates the capabilities each implementation must support for WebDriver BiDi.

Do you have a suggestion of a better way to phrase this?

I don't think that BiDi-only capabilities are a problem in general.

Currently BiDi doesn't use unhandledPromptBehavior at all, but I assume that will change.

If I was designing this from scratch, I might have something like:

session.CapabilityRequest = { … promptBehavior: session.PromptBehavior } session.PromptBehavior = { ?alert: : session.PromptBehaviorOption, ?confirm: : session.PromptBehaviorOption, ?default: session.PromptBehaviorOption .default "ignore", ?file: : session.PromptBehaviorOption, ?httpAuth: : session.PromptBehaviorOption, ?unload: : session.PromptBehaviorOption, } session.PromptBehaviorOption = "accept" / "dismiss" / "ignore"

Then for each prompt type you could have different behaviour, depending on what you expect without needing a separate top-level capability. Classic's unhandledPromptBehavior would be more or less equivalent to {default: <behavior>}. For compatibility we could always use unhandledPromptBehavior as a fallback if nothing else is specified.

It does not look like unhandledPromptBehavior in classic includes file dialogs (https://html.spec.whatwg.org/#user-prompts)? So it would not be exactly equivalent to the default behavior. Should we change to this structure as part of this PR? I am not sure how would the accept and dismiss work as I understood it's rather difficult to dismiss file dialogs also in Firefox (based on a thread in Matrix)

Right, probably only "dismiss" and "ignore" work for all dialogs. "accept" works with dialogs which don't take a value or for which there's a reasonable default value. That doesn't include file or HTTP Auth.

I am also not sure if dismiss works for files because we don't actually want to dismiss the dialog (e.g., dispatch the cancel event on element) but rather control whether it is shown on screen or not. So do you suggest we introduce the general promptBehavior capability for this feature or does a standalone capability (current version) sound okay for this?

I think we should introduce something that we can extend to cover more dialogs, even if for now we only support it for file dialogs. So a definition like:

session.CapabilityRequest = { … promptBehavior: session.PromptBehavior } session.PromptBehavior = { ?file: : "suppress" / "dismiss" / "none", }

would be fine. "suppress" would emit the event but not display the dialog. "dismiss" would cause a cancel event to be fired, as if the Cancel button was pressed, and "none" would not affect the dialog (although an event would still be fired if a session was subscribed). I"d even be happy to drop "dismiss" for now, which I think should make it functionally equivalent to what you already have.

index.bs

jgraham · 2023-11-27T13:23:49Z

index.bs

@@ -1400,6 +1400,7 @@ for a session.
 <pre class="cddl remote-cddl local-cddl">
 session.CapabilityRequest = {
  ? acceptInsecureCerts: bool,
+  ? interceptFileDialogs: bool,


I don't think that BiDi-only capabilities are a problem in general.

Currently BiDi doesn't use unhandledPromptBehavior at all, but I assume that will change.

If I was designing this from scratch, I might have something like:

session.CapabilityRequest = { … promptBehavior: session.PromptBehavior } session.PromptBehavior = { ?alert: : session.PromptBehaviorOption, ?confirm: : session.PromptBehaviorOption, ?default: session.PromptBehaviorOption .default "ignore", ?file: : session.PromptBehaviorOption, ?httpAuth: : session.PromptBehaviorOption, ?unload: : session.PromptBehaviorOption, } session.PromptBehaviorOption = "accept" / "dismiss" / "ignore"

Then for each prompt type you could have different behaviour, depending on what you expect without needing a separate top-level capability. Classic's unhandledPromptBehavior would be more or less equivalent to {default: <behavior>}. For compatibility we could always use unhandledPromptBehavior as a fallback if nothing else is specified.

index.bs

sadym-chromium · 2023-12-08T11:12:27Z

index.bs

+            params: input.FileDialogInfo
+         )
+
+         input.FileDialogInfo = {


Should we provide an info if the file dialog was intercepted or not? And if so, should the value be different for different sessions depending on the intercept file dialogs capability?

The dialog is a shared resource so it will be intercepted if any of the active session intercepts it. I think a flag would be useful but there is no immediate use case for it.

sadym-chromium

LGTM. The isIntercepted flag in the event can be added later on.

index.bs

jgraham · 2024-01-10T14:36:29Z

index.bs

@@ -1400,6 +1400,7 @@ for a session.
 <pre class="cddl remote-cddl local-cddl">
 session.CapabilityRequest = {
  ? acceptInsecureCerts: bool,
+  ? interceptFileDialogs: bool,


I think we should introduce something that we can extend to cover more dialogs, even if for now we only support it for file dialogs. So a definition like:

session.CapabilityRequest = { … promptBehavior: session.PromptBehavior } session.PromptBehavior = { ?file: : "suppress" / "dismiss" / "none", }

would be fine. "suppress" would emit the event but not display the dialog. "dismiss" would cause a cancel event to be fired, as if the Cancel button was pressed, and "none" would not affect the dialog (although an event would still be fired if a session was subscribed). I"d even be happy to drop "dismiss" for now, which I think should make it functionally equivalent to what you already have.

jgraham · 2024-01-10T14:46:02Z

index.bs

+
+  1. [=Emit an event=] with |session| and |body|.
+
+1. Let |intercepting sessions| be the [=set of sessions for which file dialog interception is enabled=].


I don't think this should use "did any session suppress the dialog". Rather if any session actually requests a specific capability value here then any later session requesting a different capability value should fail during session creation. If sessions don't request a specific value then a later session requesting a specific value can affect the behaviour (maybe later we should have a session.CapabilityChanged event for when a new session affects existing settings, but that's out of scope for now).

css-meeting-bot · 2024-01-10T17:35:35Z

The Browser Testing and Tools Working Group just discussed File upload status.

The full IRC log of that discussion

<jgraham> Topic: File upload status
<jgraham> github: https://github.com//pull/568
<jgraham> jrandolf: I want to understand where we are with the file dialog opened event. Are there problems we should address there?
<jgraham> q+
<jgraham> ack next
<whimboo> q+
<jgraham> jgraham: The way we handle capabilities looks wrong. We should follow the behaviour of the first session to set a capability and if later sessions do something different then creating the session should fail.
<jgraham> jrandolf: That seems reasonable. Having unified behaviour for different prompts is hard because file dialogs come from the OS, which have different behaviour from alert type prompts. So I don't see a good reason to put them together.
<AutomatedTester> q+
<jgraham> 1+
<jgraham> q+
<jgraham> s/1+//
<jgraham> ach whimboo
<jgraham> ack whimboo
<jgraham> s/ach whimboo//
<jrandolf> q+
<jgraham> whimboo: Can we land the other PR without having to wait for the dialog event PR? This would help with the file upload feature.
<jgraham> jrandolf: Yes
<whimboo> https://github.com//pull/514
<jgraham> whimboo: I think that one is close and it would be good to land soon.
<jgraham> jrandolf: I'll do that after the meeting.
<jgraham> ack AutomatedTester
<jrandolf> q+
<jgraham> AutomatedTester: With regard to alerts vs OS-level dialogs, I don't think end users know about this difference. Is there enough of a difference that the difference will be meaingful.
<jgraham> ack jgrhaam
<jgraham> ack jgraham
<jgraham> jgraham: I don't think the distinction between OS level and implementation dialogs is meaningful e.g. alerts used to be OS-level and maybe file dialogs will move away from native in the future. I don't think all prompts can have the same handling, but I think it would be nice to have a more uniform protocol that avoids having one top-level capability for each different prompt type. But it's an
<jgraham> aesthetic concern, not a functional one.
<jgraham> ack jrandolf
<jgraham> jrandolf: The file dialog is a consequence of input, so it would make more sense to put it in performActions if we wanted to put this management somewhere. There are other types of picker like color pickers that are also a kind of dialog. We don't put those in the spec. We could maybe put those with the file dialog. Because it's an input it's not really related to alert et. al. even from an end user
<whimboo> lightning00blade: Sorry, but I had to leave. Coud you take the frame name agenda item given that you filed it? Thanks!
<jgraham> standpoint, because they're not created in the same way.
<jrandolf> q+
<jgraham> jgraham: a button with prompt and an input with a file dialog seem quite similar to me. But I don't think it's going to affect the functionality how we do this.
<jgraham> jrandolf: Could we separate capabilities from the PR, or do we want to land it together?
<jrandolf> q+
<jgraham> jgraham: I think the capabilites are useful because the default behaviour should be to not affect the normal appearance of the dialog. For most test automation cases you probably do want to suppress it, so it feels like the PR without the capabilites won't be useful.
<jgraham> jrandolf: I want to seperate out the discussion on the capabilities design from the evet itself, in which case we'd just have the normal behaviour to begin with.
<jgraham> jgraham: I think that's fine as long as the default is what would happen without WebDriver connected.
<jgraham> jrandolf: yes, the default would be what we have now.
<jgraham> q?
<jgraham> ack jrandolf
<jrandolf> q-

css-meeting-bot · 2024-07-10T16:49:03Z

The Browser Testing and Tools Working Group just discussed File dialog handling.

The full IRC log of that discussion

<AutomatedTester> topic: File dialog handling
<AutomatedTester> github: https://github.com//pull/568
<orkon> q+
<AutomatedTester> jgraham: This is more of aquestion. The user prompt handling is in place now we can work on this
<AutomatedTester> ack next
<AutomatedTester> orkon: Maksim will be working on this and will update the PR
<AutomatedTester> ... what we will want now is that if a dialog is opened that we fire an event back to the client
<AutomatedTester> ... and it is similar to the PR
<AutomatedTester> ... we don't have the ability to handle this at the moment because of the way that CDP works but we hope to have it updated for bidi
<AutomatedTester> q?
<jgraham> Using the userPromptOpened events
<AutomatedTester> jgraham: that sounds great! I was hoping for that

whimboo · 2024-07-15T10:45:10Z

Using the userPromptOpened events
jgraham: that sounds great! I was hoping for that

So I assume that we will have a custom type for file open dialogs then which are emitted as browsingContext.userPromptOpened events? We should then probably update the PR summary to reflect that. @sadym-chromium can you please check? Thanks.

css-meeting-bot · 2024-09-27T19:26:51Z

The Browser Testing and Tools Working Group just discussed Implement input.FileDialogOpened.

The full IRC log of that discussion

<AutomatedTester> topic: Implement input.FileDialogOpened
<AutomatedTester> github: https://github.com//pull/568
<AutomatedTester> sadym: I need to go review this and get back to the WG

jrandolf-2 requested a review from OrKoN October 9, 2023 12:37

jrandolf-2 force-pushed the jrandolf/file-dialog-opened branch 3 times, most recently from b8247dc to 3bd59bb Compare October 9, 2023 13:13

OrKoN approved these changes Oct 9, 2023

View reviewed changes

jrandolf-2 requested review from whimboo and jgraham October 9, 2023 13:43

jgraham reviewed Oct 27, 2023

View reviewed changes

OrKoN mentioned this pull request Oct 30, 2023

Add input.SetFiles #514

Merged

jrandolf-2 force-pushed the jrandolf/file-dialog-opened branch from 3bd59bb to e31b876 Compare November 8, 2023 11:34

jrandolf-2 requested a review from jgraham November 10, 2023 10:55

Implement input.FileDialogOpened

3f0bb48

OrKoN force-pushed the jrandolf/file-dialog-opened branch from e31b876 to 5421b69 Compare November 21, 2023 13:58

Add capability processing

4e766f5

OrKoN force-pushed the jrandolf/file-dialog-opened branch from 5421b69 to 4e766f5 Compare November 21, 2023 14:00

OrKoN self-requested a review November 21, 2023 14:12

whimboo reviewed Nov 24, 2023

View reviewed changes

OrKoN requested a review from sadym-chromium November 27, 2023 12:22

jgraham reviewed Nov 27, 2023

View reviewed changes

Added multiple

ef7baa7

OrKoN requested a review from gsnedders December 7, 2023 09:27

OrKoN approved these changes Dec 7, 2023

View reviewed changes

sadym-chromium reviewed Dec 8, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

sadym-chromium reviewed Dec 8, 2023

View reviewed changes

sadym-chromium approved these changes Dec 8, 2023

View reviewed changes

OrKoN reviewed Dec 8, 2023

View reviewed changes

index.bs Show resolved Hide resolved

OrKoN reviewed Dec 8, 2023

View reviewed changes

index.bs Show resolved Hide resolved

OrKoN force-pushed the jrandolf/file-dialog-opened branch from f24fb38 to ef7baa7 Compare December 8, 2023 13:39

OrKoN reviewed Dec 8, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Mention default value.

dc921f7

jrandolf-2 requested a review from jgraham December 13, 2023 17:03

jgraham requested changes Jan 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `input.FileDialogOpened` #568

Implement `input.FileDialogOpened` #568

jrandolf-2 commented Oct 9, 2023 •

edited by pr-preview bot

Loading

jgraham left a comment

jgraham commented Oct 27, 2023

jgraham commented Oct 27, 2023

jrandolf-2 commented Oct 28, 2023

OrKoN commented Oct 30, 2023

jgraham commented Oct 30, 2023 •

edited

Loading

lukewarlow commented Nov 11, 2023

jrandolf-2 commented Nov 16, 2023

OrKoN commented Nov 21, 2023

whimboo Nov 24, 2023

OrKoN Nov 24, 2023

mathiasbynens Nov 27, 2023

jgraham Nov 27, 2023

OrKoN Nov 27, 2023

jgraham Nov 27, 2023

OrKoN Nov 27, 2023

jgraham Jan 10, 2024

jgraham Nov 27, 2023

sadym-chromium Dec 8, 2023

OrKoN Dec 8, 2023

sadym-chromium left a comment

jgraham Jan 10, 2024

jgraham Jan 10, 2024

css-meeting-bot commented Jan 10, 2024

css-meeting-bot commented Jul 10, 2024

whimboo commented Jul 15, 2024

css-meeting-bot commented Sep 27, 2024


		1. [=Emit an event=] with \|session\| and \|body\|.

		1. Let \|intercepting sessions\| be the [=set of sessions for which file dialog interception is enabled=].

Implement input.FileDialogOpened #568

Are you sure you want to change the base?

Implement input.FileDialogOpened #568

Conversation

jrandolf-2 commented Oct 9, 2023 • edited by pr-preview bot Loading

jgraham left a comment

Choose a reason for hiding this comment

jgraham commented Oct 27, 2023

jgraham commented Oct 27, 2023

jrandolf-2 commented Oct 28, 2023

OrKoN commented Oct 30, 2023

jgraham commented Oct 30, 2023 • edited Loading

lukewarlow commented Nov 11, 2023

jrandolf-2 commented Nov 16, 2023

OrKoN commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadym-chromium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

css-meeting-bot commented Jan 10, 2024

css-meeting-bot commented Jul 10, 2024

whimboo commented Jul 15, 2024

css-meeting-bot commented Sep 27, 2024

Implement `input.FileDialogOpened` #568

Implement `input.FileDialogOpened` #568

jrandolf-2 commented Oct 9, 2023 •

edited by pr-preview bot

Loading

jgraham commented Oct 30, 2023 •

edited

Loading