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

Don't take destructive action on esc #17779

Merged
merged 1 commit into from Aug 15, 2018

Conversation

Projects
None yet
5 participants
@matthewwithanm
Copy link
Member

matthewwithanm commented Aug 3, 2018

Description of the Change

People instinctively hit escape to ignore alerts, so we shouldn't have it do something destructive.

Alternate Designs

We could also switch the button order but I kept it the same in most regards.

Possible Drawbacks

Maybe somebody memorized that hitting escape did the destructive thing? Seems unlikely.

Verification Process

Did the following in the console:

require('electron').remote.getCurrentWindow().emit('unresponsive')
require('electron').remote.getCurrentWindow().emit('crash')

Hit escape after each and the window wasn't destroyed. Repeated, clicking the destructive buttons and it was.

@matthewwithanm matthewwithanm requested a review from maxbrunsfeld Aug 3, 2018

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld left a comment

👍 👍 👍

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Aug 4, 2018

@Ben3eeE was it you who noticed some of these Escape issues earlier? Just wondering if you know if there's any open issues regarding this.

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Aug 4, 2018

@50Wliu Yeah I've mentioned this before. I don't remember if there is an issue for this.

This change will not work on Windows according to the api documentation. We need to change the label to be Cancel or No for it to work on Windows instead of using cancelId.

Adding a Cancel or No label will also make the dialog render buttons with the correct style on Windows. See atom/github#1452 for pictures.

From the electron api documentation:

cancelId Integer (optional) - The index of the button to be used to cancel the dialog, via the Esc key. By default this is assigned to the first button with "cancel" or "no" as the label. If no such labeled buttons exist and this option is not set, 0 will be used as the return value or callback response. This option is ignored on Windows.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Aug 5, 2018

It might actually work, according to electron/electron#13882.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Aug 14, 2018

Latest VSTS build for this PR is green despite what the check says (known VSTS bug where PR rebuilds don't update PR status). AppVeyor is clean too so I'm merging this. Thanks a bunch @matthewwithanm!

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Aug 14, 2018

Actually, I just read @Ben3eeE's comment while doing one last pass over everything and it looks like this change might not resolve the issue on Windows? Is it worth trying to change these button labels so that the behavior is also correct on Windows?

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Aug 15, 2018

@daviwil It seems that cancelId does work on Windows since the PR to update the electron docs was merged electron/electron#13882.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Aug 15, 2018

Great, thanks for verifying @Ben3eeE! Merging this then :)

@daviwil daviwil merged commit f922b7b into master Aug 15, 2018

3 of 4 checks passed

VSTS: Atom Pull Requests 1.31.0-dev+20180803.2 failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the fb-mdt-esc-shouldnt-destroy branch Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.