Below is manual regression range:

Good build: 71.0.3560.0 (Revision: 593539)
Bad build: 71.0.3561.0 (Revision: 593801)

Using the 'per-revision' script providing the bisect result:

You are probably looking for a change made after 593618 (known good), but no later than 593619 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
  https://chromium.googlesource.com/chromium/src/+log/0b2c413c61036424fa66a8e27c6517ad10fb2d3f..ab21d472ec0c8e9fcdc6933e485b828455011c4c

Suspecting: https://chromium.googlesource.com/chromium/src/+/ab21d472ec0c8e9fcdc6933e485b828455011c4c

@sangwoo108: Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Kindly review the attached screen-cast for reference.

Thank you.

Deleted: Actual behavior.mp4 548 KB

	Actual behavior.mp4 548 KB View Download

Deleted: Expected behavior.mp4 528 KB

	Expected behavior.mp4 528 KB View Download

This actually has additional side effects as well. If the destinations modal is open and the search box is not focused, both the destinations dialog and the preview dialog close, which is incorrect. If the destinations dialog and provisional destination dialog are both open, escape closes both of them and the print preview dialog.

Issue 790992 already tracks what is stated in comment #2 I think.

790992 was fixed prior to this CL.

Just to confirm comment 5: Performed another bisect for the regression described in comment 2, which returned the following regression range: https://chromium.googlesource.com/chromium/src/+log/0b2c413c61036424fa66a8e27c6517ad10fb2d3f..4921ad7af6e7a032cf7f2e1de4fadf26e2229388

which includes the CL identified in comment 1. The other CLs in the regression range seem unrelated to the problem.

Scottchen@, What I recently made seems to cause this. I'm not sure how I can solve this without reverting the previous patch. Could you give me a guide?

I guess what happens here is that destination_dialog's onKeyDown_ is not called because we stopped propagation from cr_dialog. Maybe we can introduce ignoreEscapeKey to cr_dialog as we have ignoreEnterKey as well. What do you think?

What was the rationale for the original change?

It seems to be that the opposite approach is better. Make what https://chromium-review.googlesource.com/c/chromium/src/+/1230493 did opt-in, and preserve the previous behavior as the default behavior.

dpapad@ CommandManager handled shortcuts even when cr-dialog is open. And We thought cr-dialog is better to behave modally by default.

cc calamity@. Calamity, could you give me your opinion on comment 8 and 9? I'm on the fence. I think what dpapad@ suggested is safer but not sure if it's what you imagined in the beginning.

sangwoo108@, I think we should revert the CL and make it opt-in like dpapad@ suggested above. There seems to be other pages that assumes keydowns and keyboard commands will be propagated past cr-dialog. For example:

in chrome://settings add-language dialog, when users hit ctrl+f we are supposed to intercept that command and focus the dialog's search input, instead of bringing up the native browser search. This behaviors seems to be also broken now after your CL.

FYI, verified that the ctrl+F behavior for the add-language-dialog is also fixed by reverting this CL, so +1 on reverting for now.

Also, perhaps we should revisit briefly why do we need the new behavior at all, before we make it opt-in. Maybe there is a different bug causing the issue (Blink bug where dialog modal-ness is escaped maybe?)

Looking at the original bug and CL, here is an alternative proposed fix.

At [1] where we determine if the Ctrl+A command can be executed, we should update the check to take into account whether any modal dialog is shown. The latter can be determined either by querying the DOM, or listening for 'cr-dialog-open' (fired from [2]) and 'close' event. I don't think we need to modify cr-dialog itself (unless this becomes a larger issue in the future).


[1] https://cs.chromium.org/chromium/src/chrome/browser/resources/md_history/app.js?l=228
[2] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js?l=176

Can't the keydown listener in destination_dialog just be hooked onto the cr-dialog?

I think in general it's unintuitive for key events to flow past a modal, especially when our keyboard shortcut library (commands.js) registers handlers on body, so I'd prefer that cr-dialog helps handle this case, even if there are a couple of things that need fixing to make that API change happen.

There is no way to guarantee that an open modal dialog gets key events first (or sometimes at all). See example at https://jsfiddle.net/dnec5wz9/. User can always click on the backdrop and then hit a key, which fires the event only on the <body>.

@calamity, @sangwoo108: I think the best course of action for now is to revert the CL, and re-visit the solution. Since there are two separate regressions (print preview and settings add-language-dialog mentioned at #13).

Alternate fixes mentioned so far are:
1) Make the new cr-dialog behavior opt-in.
2) Address the bookmarks case only (see comment#14 suggestion, which could be expanded to all commands, not just "select all".

Revert CL sent to the CQ, see https://chromium-review.googlesource.com/c/chromium/src/+/1249809.

dpapad@ I agree. Thanks :)

Opt-in sgtm. Thanks!