w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
336 stars 35 forks source link

Implement `input.FileDialogOpened` #568

Open jrandolf opened 9 months ago

jrandolf commented 9 months ago

This PR implements the event portion of file dialog flow.

Relevant HTML spec: https://github.com/whatwg/html/pull/9844

Issue: https://github.com/w3c/webdriver-bidi/issues/494 Input.setFiles PR: https://github.com/w3c/webdriver-bidi/pull/514


Preview | Diff

jgraham commented 8 months ago

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

jgraham commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

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

css-meeting-bot commented 6 months ago

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/w3c/webdriver-bidi/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/w3c/webdriver-bidi/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 commented 1 week ago

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/w3c/webdriver-bidi/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 commented 2 days ago
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.