8000 Don't take destructive action on esc by matthewwithanm · Pull Request #17779 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Don't take destructive action on esc #17779

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Don't take destructive action on esc #17779

merged 1 commit into from
Aug 15, 2018

Conversation

matthewwithanm
Copy link
Contributor

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.

maxbrunsfeld
maxbrunsfeld approved these changes Aug 3, 2018
View reviewed changes
Copy link
Contributor
@maxbrunsfeld maxbrunsfeld left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@winstliu
Copy link
Contributor
winstliu 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
Copy link
Contributor
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.

@winstliu
Copy link
Contributor
winstliu commented Aug 5, 2018

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

@daviwil
Copy link
Contributor
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
Copy link
Contributor
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
Copy link
Contributor
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
Copy link
Contributor
daviwil commented Aug 15, 2018

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

@daviwil daviwil merged commit f922b7b into master Aug 15, 2018
@daviwil daviwil deleted the fb-mdt-esc-shouldnt-destroy branch August 15, 2018 13:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maxbrunsfeld maxbrunsfeld maxbrunsfeld approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@matthewwithanm @winstliu @Ben3eeE @daviwil @maxbrunsfeld
0