In dialogs & dialog-like bubbles, make the escape button just close

In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079, 163931
Committed: https://crrev.com/0bf4ced694bba0aeb3ba518c7fe8435bc9018674
Cr-Commit-Position: refs/heads/master@{#392708}

[Evan Stade, 2016-05-06 18:12:30]

Description was changed from

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079
==========

to

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes
are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079
==========

[Evan Stade, 2016-05-06 18:12:30]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-05-06 18:12:39]

Description was changed from

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes
are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079
==========

to

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079
==========

[Evan Stade, 2016-05-06 18:13:06]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-06 18:13:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953943003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953943003/1

[sky, 2016-05-06 21:29:58]

LGTM

[Evan Stade, 2016-05-09 18:28:21]

Description was changed from

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079
==========

to

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079,163931
==========

[Evan Stade, 2016-05-09 18:28:44]

PTAL additional changes to
	chrome/browser/ui/views/ssl_client_certificate_selector*

[Evan Stade, 2016-05-09 18:28:57]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-09 18:29:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953943003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953943003/40001

[Evan Stade, 2016-05-10 16:56:44]

On 2016/05/09 18:28:44, Evan Stade wrote:
> PTAL additional changes to
> 	chrome/browser/ui/views/ssl_client_certificate_selector*

ping --- I'm getting nagged on the bug to fix this quickly so we can merge.

[Evan Stade, 2016-05-10 17:07:57]

estade@chromium.org changed reviewers:
+ msw@chromium.org

[Evan Stade, 2016-05-10 17:07:58]

sky is ooo, msw could you ptal
	chrome/browser/ui/views/ssl_client_certificate_selector*

[msw, 2016-05-10 18:23:32]

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (left):

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:146: if (key_code ==
ui::VKEY_ESCAPE) {
This is the one case that I use myself and appreciate to some degree... If I
mistakenly hit CTRL+D (trying to hit F or C with my fat fingers), being able to
quickly press Esc to undo that action is nice. Now I'll have to tab/mouse to
press the cancel button. WDYT about keeping this behavior (and cancelling
dialogs by default?), but ensuring that Esc doesn't block permissions?

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:59:
CertificateSelected(nullptr);
nit: maybe note that this should have no effect if an earlier accept/close
action caused certificate selection to proceed or abort.

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:86: // If the dialog
is closed by closing the containing tab, the request should
nit: s/the request should abort/abort the request/ for a one-liner.

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (left):

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:218: // If there's a cancel button, it
handles escape.
I like this behavior with the bookmark bubble, and I'm trying to think of other
cases where it might be nice to cancel (not just close) on Esc. It'd be really
nice to have a table listing dialogs where cancel isn't equivalent to close, and
listing the respective actions... can you find just a couple more examples?

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:161:
dialog()->CheckAndResetStates(false, false, nullptr);
Can you add a test case that Esc closes the dialog now?

[Evan Stade, 2016-05-10 18:46:55]

Regarding the behavior change, we had a meeting about this that included hwi,
ainslie, zkoch, and multiple
engineers. This was the conclusion (hence sky's approval).

I forwarded you an associated email thread but I'd rather not reopen the debate
as I feel like all sides of the argument were represented and considered
(including those who were against the change).

[msw, 2016-05-10 19:16:06]

On 2016/05/10 18:46:55, Evan Stade wrote:
> Regarding the behavior change, we had a meeting about this that included hwi,
> ainslie, zkoch, and multiple
> engineers. This was the conclusion (hence sky's approval).
> 
> I forwarded you an associated email thread but I'd rather not reopen the
debate
> as I feel like all sides of the argument were represented and considered
> (including those who were against the change).

Hmm, okay, I guess.

[Evan Stade, 2016-05-10 19:27:36]

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:59:
CertificateSelected(nullptr);
On 2016/05/10 18:23:32, msw wrote:
> nit: maybe note that this should have no effect if an earlier accept/close
> action caused certificate selection to proceed or abort.

Done.

https://codereview.chromium.org/1953943003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:86: // If the dialog
is closed by closing the containing tab, the request should
On 2016/05/10 18:23:32, msw wrote:
> nit: s/the request should abort/abort the request/ for a one-liner.

Done.

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/1953943003/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate_unittest.cc:161:
dialog()->CheckAndResetStates(false, false, nullptr);
On 2016/05/10 18:23:32, msw wrote:
> Can you add a test case that Esc closes the dialog now?

Done.

[msw, 2016-05-10 19:38:36]

lgtm

[Evan Stade, 2016-05-10 19:39:44]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-05-10 19:39:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1953943003/#ps80001
(title: "elaborate test")

[commit-bot: I haz the power, 2016-05-10 19:40:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953943003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953943003/80001

[commit-bot: I haz the power, 2016-05-10 20:53:17]

Description was changed from

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079,163931
==========

to

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079,163931
==========

[commit-bot: I haz the power, 2016-05-10 20:53:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-10 20:55:08]

Description was changed from

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079,163931
==========

to

==========
In dialogs & dialog-like bubbles, make the escape button just close
the prompt.

Previously, the escape button would activate the negative button, which
was relevant if that button did anything extra in addition to closing
the prompt.

This intentionally changes the permissions bubble (back to what it
was before) as well as the bookmark bubble (now escape doesn't
remove a new bookmark). Other bubbles may be affected and
those changes are *probably* desired.

UX contacts for this decision are hwi@ and ainslie@

BUG=609079,163931

Committed: https://crrev.com/0bf4ced694bba0aeb3ba518c7fe8435bc9018674
Cr-Commit-Position: refs/heads/master@{#392708}
==========

[commit-bot: I haz the power, 2016-05-10 20:55:10]

Patchset 5 (id:??) landed as
https://crrev.com/0bf4ced694bba0aeb3ba518c7fe8435bc9018674
Cr-Commit-Position: refs/heads/master@{#392708}