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.

Sign up for GitHub

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

Jump to bottom

Add file dialog events and methods #535

Closed
wants to merge 3 commits into from
Closed

Add file dialog events and methods #535

wants to merge 3 commits into from

Conversation

jrandolf-2
Copy link
Contributor

@jrandolf-2 jrandolf-2 commented Sep 11, 2023 •
edited by pr-preview bot
Loading

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/file-dialog branch 2 times, most recently from d9162e9 to 4d4f3bf Compare September 11, 2023 13:43
OrKoN
OrKoN reviewed Sep 11, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@OrKoN OrKoN requested review from sadym-chromium and removed request for whimboo and jgraham September 11, 2023 13:48
@OrKoN
Copy link
Contributor

OrKoN commented Sep 11, 2023

We are going to review internally first.

OrKoN
OrKoN reviewed Sep 11, 2023
View reviewed changes
index.bs
element: script.SharedReference,
files: [*text]
input.SetFilesParameters = input.FileDialogInfo .and {
files: [*text]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead we should provide the element reference in the interception event and allow setting files both with and without the dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a use-case for setting files when the user was never prompted for a dialog?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the use case would be to fill out the forms without testing/verifying if they really result in a dialog (that is an existing use case supported by Puppeteer).

OrKoN
OrKoN reviewed Sep 11, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
OrKoN
OrKoN reviewed Sep 11, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
OrKoN
OrKoN reviewed Sep 11, 2023
View reviewed changes
index.bs Outdated

</div>

#### The input.displayFileDialog Command #### {#command-input-displayFileDialog}
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the use case for displaying the dialog to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows introspection of file dialog prompt if a manual workflow. I believe this makes this feature complete, but we can omit this for the initial proposal.

OrKoN
OrKoN requested changes Sep 25, 2023
View reviewed changes
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.

Let's update the spec with the following:

  1. mention that the dialog is not blocking and it is automatically dismissed.
  2. add a shared reference to the element that is the target of the dialog.
  3. keep the shared reference as the parameter on setFiles

@jgraham
Copy link
Member

jgraham commented Sep 25, 2023

mention that the dialog is not blocking and it is automatically dismissed.

I'm actually not sure we agreed to that (maybe by default if you're subscribed to the events?) But we need some kind of per-session configuration that enables either auto-dissmiss or not depending on the use case.

@jrandolf-2
Copy link
Contributor Author

jrandolf-2 commented Sep 25, 2023 •
edited
Loading

mention that the dialog is not blocking and it is automatically dismissed.

I'm actually not sure we agreed to that (maybe by default if you're subscribed to the events?) But we need some kind of per-session configuration that enables either auto-dismiss or not depending on the use case.

I think @OrKoN meant that it is automatically dismissed when a session is subscribed to the event.

But we need some kind of per-session configuration that enables either auto-dismiss or not depending on the use case.

So if one session is subscribed to the interception event, other sessions cannot be not auto-dismiss for the same reason multiple users of the same browser cannot use the browser if any of the users is picking a file.

@jgraham jgraham mentioned this pull request Sep 25, 2023
Open
@jrandolf-2 jrandolf-2 closed this Oct 9, 2023
@whimboo
Copy link
Contributor

whimboo commented Oct 9, 2023

@jrandolf could you maybe explain why this PR got closed?

@jrandolf-2
Copy link
Contributor Author

@jrandolf could you maybe explain why this PR got closed?

I'll be separating this PR into smaller, more independent features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN requested changes

@sadym-chromium sadym-chromium Awaiting requested review from sadym-chromium

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrandolf-2 @OrKoN @jgraham @whimboo