I'm not sure we want to remove this behaviour, but I agree that the string is pretty bad. However, this behaviour has been present since at least ~2013, so it's not really a pri-1.

We should probably change the "Cancel" button to be "Don't open" or something like that, and the checkbox string to be "Remember my decision" so it's clear that it applies to both decisions. We could also make this more like a permission prompt, and make the "Cancel" string "Block".

emilyschechter / benwells / rolfe - thoughts?

benwells has just pointed out that previously (on Mac at least), the dialog didn't respond to Esc, whereas now it does seem to (I'm not really sure why). So the behaviour has degraded quite a bit there. I'll bump this back up to pri-1; it's a string change so we should decide on an improved string ASAP.

Improving this behavior is on Apple's radar, they nag us about it continuously. Let's try and get this regression and the larger issue of never being able to change the decision addressed soon if we can.

Sounds like this bug can focus on the Esc regression. +hwi might be able to help you determine what kind of modal dialog the External Protocol Request handler should be on desktop (if it changed modal types in the revamp that could be why Esc is now accessible.)

Secondarily sounds like "Cancel" is a historically a problematic button label/action. I can discuss this with emilyschechter@ when we review the revamped dialog performance next month. If it feels more urgent I can reprioritize, but looking at comment 1 it seems like this plan is OK for now.

1. ESC should dismiss a dialog and should not take any changes (this  crbug.com/609079#c10  might be relevant). The reasoning is that ESC could just mean for some people to exit the state. It'll be good to fix it for this bug. It's a common behavior for many dialogs on Chrome (e.g. Print) but not all. Harmony in 2017 plans to review inconsistency and aims to apply it to all dialogs. 
2. It'll be safe to tie/apply the checkbox status with clicking of either buttons on the dialog not with dismissal of the dialog. There're other ways to dismiss a dialog e.g. closing the tab and navigating away.
3. Changing the label "Cancel" to a specific string makes sense since it sounds like that the checkbox is meant to work with that button to remember the setting. 

Fixing the Esc regression on Win/Linux/ChromeOS (views) is easy.

Unfortunately, the dialog on Mac doesn't distinguish between dismissal via Esc and dismissal via the Cancel button. There doesn't seem to be any way to make this distinction outside of implementing a new custom Cocoa dialog.

Until MacViews arrives and we can use views dialogs on Mac, I don't think we'll be able to have Esc behave differently from the Cancel button on that platform. It's not a feasible option to implement a new Cocoa dialog at this time.

An interim solution for Mac will have to either ignore the checkbox entirely if the Cancel button is pushed, or remove the checkbox entirely.

This doesn't sound like a change that should be hard to implement on the Mac. What class needs to change to support this?

The dialog is currently using NSAlert on Mac, so it's an AppKit class. 

Personally I think it is an anti-pattern to make permanent settings changes when the user clicks Cancel. Cancel should have no effect.

In this case I feel fairly strongly about it as there is no way to undo the permanent settings changes we make.

I agree with Ben. This should probably have blocked M55, given the situation it puts users in. Every user who hits this cannot ever undo the damage. 

What are the next steps here? Can we get a fix quickly for a M55 refresh?

There is a stable refresh today. This shouldn't block that, but will likely require a subsequent refresh once we get a fix. This should not wait for M56. 

+rpop

Re: c#9, I was referring more to the Chrome code that implements the External Protocol Dialog on the Mac side so that I can take a look at it (figuring you know the class and thereby save myself some steps).

I am unable to get this dialog to appear using the set of steps in the problem description. Is there a better/alternative way to make this dialog visible? I want to take a look at it before recommending any particular change.

#15: I think it depends on what protocols your system has registered. Try this which has some more common ones: https://jsfiddle.net/r51n95vw/

If those don't work, change the array with ones for your system:

On Windows you can check by searching for "protocol" in Settings/Start.

On Mac, try one of these answers? http://superuser.com/questions/498943/directory-of-url-schemes-for-mac-apps

Also, I didn't realize this before, but it seems the permanent change is application-wide. It's not limited to the Chrome profile you're using.

I see completely different dialog when compared to Chrome 54.0.2840.98 to 55.0.2883.87 please find the attached screenshots for reference, But over all the behavior remained same i.e., after selecting "Remember my choice for all links of this type" or "Always open links for Screen Sharing" and hit "esc" or "donothing " or "Cancel" in 54.0.2840.98 or 55.0.2883.87 respectively next click doesn't bring up the dialog.. 

Deleted: 54.0.2840.98-1png.png 79.0 KB

	54.0.2840.98-1png.png 79.0 KB View Download

Deleted: 55.0.2883.87.png 55.5 KB

	55.0.2883.87.png 55.5 KB View Download

#c15: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/external_protocol_dialog.mm

#c16: That's  crbug.com/457254 

#c17: This is what I meant in #c1 when I said that this behaviour has existed for a very long time. The only change from <M54 -> M55 is that the strings in the dialog have become more misleading. However, dismissing the dialog with the checkbox checked would suppress the dialog forever for that scheme, and it has behaved this way for years. There was no functional change from <M54 -> M55.

As mentioned in #c2, the easy fix here is to change the strings to be less misleading. Again, this behaviour has existed for a very long time; the different strings make it worse. This code has been unloved for a while. :(

Per comment #18, 
easy fix for this is to change strings but it is too late for any string change for M55 as it is already in Stable.

Also this behaviour has existed for a very long time.So it would be best if this can wait until M56. 

+ anantha@ and kerz@

I'd propose the following:

(1) fix escape regression on Views
(2) work with shrike@ to fix escape on Mac
(3) change string, strawman:

Open <application name>?
[] Remember my decision
DON'T OPEN | OPEN 

Why has this become a Stable blocker so late in M55 cycle? Looks like this behavior is there from a long time? Am I missing any context?

While I agree that the new strings make this long term bad behavior even worse, neither emilyschechter@ nor I see this as blocking stable.

Let's fix in 56 - the behavior where we can, the strings per #20 where we can't. 

Can we also recover users who are in a never prompt, never open state, so they are reprompted the first time the protocol handler is invoked on 56?

dominickn@ - thank you for your explanation in c#18.

Re: c#19
> Also this behaviour has existed for a very long time.So it would be best if this can wait until M56.

Not exactly true - the wording of the buttons has changed, which makes the behavior incorrect (for the new wording). Also, I don't see where the Cancel / Don't Open button is tied to the Escape key on the Mac. I suspect that the Mac is recognizing that the button title is now Cancel and automatically setting the key equivalent of that button to Cancel. So that would be new behavior as well.

It looks like the right answer is to change the button titles, something along the lines of c#20 (or what it was previously). It *might* be possible to hack the dialog so that pressing Esc is not the same as clicking Cancel, but users do not reasonably expect that clicking Cancel with the "Always open links..." button checked will mean never open those links ever again.

Thanks for investigating shrike. The automatic recognition of "Cancel" explains why Esc has started triggering dismissal in M55 but not in M54 here - that was totally unexpected. 

When I proposed fixing this by updating the strings, I meant the checkbox string and the button string together. At least, that string change will put this back to where we used to be, without a confusing Cancel option, and we can address the persistence in profile rather than local state and Esc behaviour on Views from there. 

> When I proposed fixing this by updating the strings, I meant the checkbox string and the button string together. 

Agreed that we need to update both strings.

Revised M56 proposal from emilyschechter@ after consulting with srahim@ for UX writing tips:

Open <AppName>?
[ ] Remember my choice for <AppName>
[DON'T OPEN ]  [OPEN <AppName>]

ainslie@ still has concerns about having a checkbox but the plan is to measure usage and go from there

I like Shimi's suggestion on the thread to add the word "links" to the checkbox: 
[ ] Remember my choice for <AppName> links

And yes - I'm generally sad about sprinkling around always/never choices in UI - but it isn't a blocking sadness :) 

Thanks for the updates on the string. I have an in-flight CL that I will merge to M56 with the new strings.

I've confirmed that on Mac, changing the "Cancel" button to "Don't open" eliminates Esc as a means to close the dialog. It looks like AppKit is indeed detecting the presence of a button labelled with "Cancel" and wiring up Esc to trigger it.

I can see about adding a third button to the dialog for Mac only for cancellation, making it just dismiss the dialog with no other effect. That should be minor enough to merge to M56 too, but if it's not urgent it can wait till M57. The string change will essentially restore the pre-M55 behaviour in M56.

Generally the UX preference is not to have three buttons, so probably best to leave it for now. Soooo interesting how having "cancel" makes Esc suddenly be accessible.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb32611eaf94923d6cfb2d0965603c63de4c4178

commit eb32611eaf94923d6cfb2d0965603c63de4c4178
Author: dominickn <dominickn@chromium.org>
Date: Fri Dec 16 03:34:13 2016

Clarify the strings in the external protocol dialog.

Currently, clicking "Cancel" in an external protocol dialog with the
"Always open links for <app>" option checked will permanently silence
all protocol requests for that scheme. This result is unexpected given
the strings. This CL updates the strings to clarify the behaviour.
This is a stopgap fix for M56; future milestones will see further
changes to the behaviour of this dialog to improve its usability.

BUG= 671658 

Review-Url: https://codereview.chromium.org/2581623002
Cr-Commit-Position: refs/heads/master@{#439001}

[modify] https://crrev.com/eb32611eaf94923d6cfb2d0965603c63de4c4178/chrome/app/generated_resources.grd
[modify] https://crrev.com/eb32611eaf94923d6cfb2d0965603c63de4c4178/chrome/browser/ui/cocoa/external_protocol_dialog.mm
[modify] https://crrev.com/eb32611eaf94923d6cfb2d0965603c63de4c4178/chrome/browser/ui/external_protocol_dialog_delegate.cc

Requesting merge of #31 to M56. It's a string update and has been on Canary for a couple of days. We'll need to coordinate a post-branch translation for the updated strings.

Your change meets the bar and is auto-approved for M56 (branch: 2924)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a44cc09a52bb8fc8931a8763bb2cb6fc2b86fa7f

commit a44cc09a52bb8fc8931a8763bb2cb6fc2b86fa7f
Author: Dominick Ng <dominickn@chromium.org>
Date: Mon Dec 19 05:48:03 2016

Clarify the strings in the external protocol dialog.

Currently, clicking "Cancel" in an external protocol dialog with the
"Always open links for <app>" option checked will permanently silence
all protocol requests for that scheme. This result is unexpected given
the strings. This CL updates the strings to clarify the behaviour.
This is a stopgap fix for M56; future milestones will see further
changes to the behaviour of this dialog to improve its usability.

BUG= 671658 

Review-Url: https://codereview.chromium.org/2581623002
Cr-Commit-Position: refs/heads/master@{#439001}
(cherry picked from commit eb32611eaf94923d6cfb2d0965603c63de4c4178)

Review-Url: https://codereview.chromium.org/2585163002 .
Cr-Commit-Position: refs/branch-heads/2924@{#541}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/a44cc09a52bb8fc8931a8763bb2cb6fc2b86fa7f/chrome/app/generated_resources.grd
[modify] https://crrev.com/a44cc09a52bb8fc8931a8763bb2cb6fc2b86fa7f/chrome/browser/ui/cocoa/external_protocol_dialog.mm
[modify] https://crrev.com/a44cc09a52bb8fc8931a8763bb2cb6fc2b86fa7f/chrome/browser/ui/external_protocol_dialog_delegate.cc

O.k.thansyou

Tested the issue on windows-7, windows-10, Mac 10.12.2 and Ubuntu 14.04 using chrome version 56.0.2924.51 with below steps.
1.Opened Chrome and navigated to the url  https://jsfiddle.net/r51n95vw/
2.observed the pop up with Don't open and open<Application name> by clicking the links. 
3.When not checked the Remember my choice and clicked on Open/Don't open the next click on the respective links was able to see the Dialog prompt.
4.When checked the Remember my choice and clicked on Open/Don't open the next click on the respective links, the application choice was remembered for the scheme.

Note:
In Linux  pressing the esc the popup closed.In windows and Mac the esc not working.
In Linux the prompt dialog was not having the application name in the dialog and Button, where its seen as "open xdg-open".
Please find the attached screen cast for the same on mac.
could you please confirm is this the expected behavior?

Thanks.

Deleted: 671658.mov 15.7 MB

671658.mov 15.7 MB Download

Deleted: Issue 671658 (1).png 9.9 KB

	Issue 671658 (1).png 9.9 KB View Download

#36: the xdg-open on Linux is a known issue (see crbug.com/503219)

ramyasharma@ is taking over the eng work on this one.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc29407956cd9349e12be316b7f0007c9b2471ab

commit fc29407956cd9349e12be316b7f0007c9b2471ab
Author: dominickn <dominickn@chromium.org>
Date: Fri Jan 06 03:27:13 2017

Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

BUG= 671658 

Review-Url: https://codereview.chromium.org/2559783003
Cr-Commit-Position: refs/heads/master@{#441863}

[modify] https://crrev.com/fc29407956cd9349e12be316b7f0007c9b2471ab/chrome/browser/ui/views/external_protocol_dialog.cc
[modify] https://crrev.com/fc29407956cd9349e12be316b7f0007c9b2471ab/chrome/browser/ui/views/external_protocol_dialog.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507

commit 6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507
Author: dominickn <dominickn@chromium.org>
Date: Fri Jan 06 04:07:42 2017

Add a browser test for the views external protocol dialog.

This CL tests that the views external protocol dialog behaves correctly
when it is Accepted, Canceled, and Closed. In particular, the status of
the dialog's checkbox is verified; when the dialog is Closed without
a definite user choice of Accept or Cancel, it is ensured that the
checkbox is always reported as unchecked. This is to prevent dismissing
the dialog from accidentally persisting a permanent block on external
protocols.

The dialog is not used on ChromeOS, so the test is only run on Linux,
Windows, and MacViews.

BUG= 671658 

Review-Url: https://codereview.chromium.org/2610793002
Cr-Commit-Position: refs/heads/master@{#441869}

[modify] https://crrev.com/6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507/chrome/browser/ui/views/external_protocol_dialog.h
[add] https://crrev.com/6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc
[modify] https://crrev.com/6f0a6f4bcb0e0b8b06b79259ae66a3d9a3452507/chrome/test/BUILD.gn

Requesting merge of #40 to M56. This is a very small patch which stops users from accidentally using Esc or the Cancel X on Windows/Linux to permanently silence external protocol dialogs.

Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/124a59b333bd2fefa3b80501c877d0611bd8a7bf

commit 124a59b333bd2fefa3b80501c877d0611bd8a7bf
Author: Dominick Ng <dominickn@chromium.org>
Date: Sun Jan 08 22:56:18 2017

Don't treat Esc like Cancel in the external protocol dialog on Views.

By default, closing a dialog on Views using the Esc key behaves as if
the secondary button was pressed. In the external protocol handler, this
can create a state where the user inadvertently blocks external
protocol launching for a particular scheme with no way to allow them
again. This occurs when they tick the checkbox to always open links, but
then use Esc to close the dialog. Until UI to edit external protocol
settings is implemented, this permanently prevents any dialogs from
the page for that scheme.

This CL addresses part of the bug by ensuring that closing the external
protocol dialog without interacting with the buttons ignores the checkbox
state.

BUG= 671658 

Review-Url: https://codereview.chromium.org/2559783003
Cr-Commit-Position: refs/heads/master@{#441863}
(cherry picked from commit fc29407956cd9349e12be316b7f0007c9b2471ab)

Review-Url: https://codereview.chromium.org/2621513002 .
Cr-Commit-Position: refs/branch-heads/2924@{#697}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/124a59b333bd2fefa3b80501c877d0611bd8a7bf/chrome/browser/ui/views/external_protocol_dialog.cc
[modify] https://crrev.com/124a59b333bd2fefa3b80501c877d0611bd8a7bf/chrome/browser/ui/views/external_protocol_dialog.h

Tested the issue on Win-10, Ubuntu 14.04 and Mac 10.12.2 using chrome beta version #56.0.2924.59.

Steps followed to test this issue are as follows:
-------------
1. Navigated to URL: https://jsfiddle.net/maucr36f/
2. Clicked on the first link i.e Open calculator: link. 
3. Checked the option as "Remember my choice for <> links as in the attached screenshot(link.jpeg).
4. Pressed escape key.
5. Again clicked on the first link i.e Open calculator: link.
6. Observed that the dialogue box appeared with unchecked box. 

Note: Behavior is same in both Win and Linux OS. Whereas in the Mac none of the links at URL: https://jsfiddle.net/maucr36f/ opened.

Attaching screen casts for reference.

dominickn@ - Could you please verify the screencasts and please let us know if it is the expected behavior. Also please let us know how to test this issue on Mac OS.

Thanks...!!

Deleted: 671658.mp4 1.1 MB

	671658.mp4 1.1 MB View Download

Deleted: 671658_Linux.ogv 3.9 MB

	671658_Linux.ogv 3.9 MB View Download

Deleted: link.jpeg 15.1 KB

	link.jpeg 15.1 KB View Download

krajshree: thanks for the testing. The behaviour is as expected on Windows and Linux. For Mac OS X, try running a link to tel:+00000000 (that should try to open FaceTime). If that doesn't work, you have hit this bug, and you need to run Chrome with a fresh profile directory, e.g. 

/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --user-data-dir=/tmp/profile

Also note that the dialog will not respond to Escape on Mac, as the default NSAlert behaviour does not respond to Esc unless one of the buttons is "Cancel".

Removing myself. Hwi is the desktop contact but if needed you can also confer with maxwalker, contact for security/privacy ux.

The same alert also has another bug in it on Mac. If the page behind the alert redirects before "Open App" is clicked, the url is not handled properly. This is particularly bad since the new Google OAuth flow for native apps requires logins through the browser and the app callback urls trigger this alert. If the user does not interact within 3 seconds, the page behind the alert redirects to google.com, and clicking "Open App" does nothing. The alert should properly handle this case and refer to the original URL when opened.

c#48: that doesn't sound like a bug to me. If the page behind the alert has navigated away we should not trust anything it's left behind. For instance, a malicious site could pop up an external protocol dialog to do something, and then immediately navigate itself to some trusted site in the background of the alert.

Assigning back to dominickn@ as I am on long term leave starting this Thursday

This is fixed in  Issue 724919 . Now you can never end up in a state where you persist a block for a protocol launch.