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

Modal dialog is too easy to bypass #122520

Closed
joaomoreno opened this issue Apr 28, 2021 · 9 comments
Closed

Modal dialog is too easy to bypass #122520

joaomoreno opened this issue Apr 28, 2021 · 9 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug dialogs Issues with native and custom dialogs verified Verification succeeded workspace-trust Trusted workspaces
Milestone

Comments

@joaomoreno
Copy link
Member

joaomoreno commented Apr 28, 2021

Testing #122252

  1. Have custom title bar

🐛

recording (11)

@joaomoreno
Copy link
Member Author

Are we (should we) be using the <dialog> HTML element?

@sbatten
Copy link
Member

sbatten commented Apr 28, 2021

@sbatten sbatten added bug Issue identified by VS Code Team member as probable bug dialogs Issues with native and custom dialogs labels Apr 28, 2021
@sbatten sbatten added this to the April 2021 milestone Apr 28, 2021
@bpasero
Copy link
Member

bpasero commented Apr 29, 2021

no https://caniuse.com/?search=dialog

Well, we could do browser detection and still use dialog where supported to get the best experience. I would also think that using dialog elements has some impact on accessibility / screen readers.

@joaomoreno
Copy link
Member Author

joaomoreno commented Apr 29, 2021

  1. Press Alt

recording (22)

@joaomoreno joaomoreno reopened this Apr 29, 2021
@sbatten
Copy link
Member

sbatten commented Apr 29, 2021

There's some trickiness with the alt listener because both event handlers use capture on the same element. I have a fix but I don't want to check it in this late in case there are conflicts with the other consumer (cc @isidorn)

@sbatten sbatten modified the milestones: April 2021, May 2021 Apr 29, 2021
@sbatten sbatten closed this as completed in 244eccd May 7, 2021
@joaomoreno
Copy link
Member Author

joaomoreno commented May 31, 2021

On Windows:

  1. Alt
  2. V
  3. Enter

recording (2)

@joaomoreno joaomoreno reopened this May 31, 2021
@lszomoru lszomoru added the workspace-trust Trusted workspaces label Jun 3, 2021
@sbatten sbatten modified the milestones: May 2021, June 2021 Jun 4, 2021
@sbatten
Copy link
Member

sbatten commented Jun 4, 2021

I'm not sure why I got lucky before with the timing, but the preventDefault just doesn't work for this now. You can bypass the dialog, but at least its not just clicking. Can try something better next week.

@sbatten
Copy link
Member

sbatten commented Jun 25, 2021

The menubar uses capture because it needs to function "natively" intercepting mnemonics everywhere. the dialog does the same and unfortunately is always going to add its event listener after, so it doesn't have a chance to intercept. I spent some time trying to come up with something but nothing I came up with didn't involve one component having to know about another. as I stated above, the initial "too easy" bypass was clicking which is no longer an option. since we are using a custom dialog, we will not get a truly blocking experience without some more major work that I don't see a need to prioritise.

@sbatten sbatten closed this as completed Jun 25, 2021
@sbatten
Copy link
Member

sbatten commented Jun 25, 2021

To Verify: Check that you cannot click the Custom Menu Bar on Windows. Native menu bars cannot be blocked by the custom dialog.

@rzhao271 rzhao271 added the verified Verification succeeded label Jun 30, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug dialogs Issues with native and custom dialogs verified Verification succeeded workspace-trust Trusted workspaces
Projects
None yet
Development

No branches or pull requests

6 participants
@joaomoreno @bpasero @lszomoru @sbatten @rzhao271 and others