Implement input.FileDialogOpened

[jrandolf]

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]

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

[jgraham]

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]

@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]

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]

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]

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]

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]

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

[whimboo]

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.

[OrKoN]

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?

[mathiasbynens]

@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?

[jgraham]

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.

[OrKoN]

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)

[jgraham]

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.

[OrKoN]

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?

[jgraham]

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]

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.

[sadym-chromium]

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?

[OrKoN]

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.

[jgraham]

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]

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]

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]

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]

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.