[MD Bookmarks] Restore focus after closing a dialog or context menu.

[MD Bookmarks] Restore focus after closing a dialog or context menu.

This CL makes dialogs in the bookmark manager restore focus to the
previously focused element after dismissal.

BUG=730905
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2954273002
Cr-Commit-Position: refs/heads/master@{#483440}
Committed: https://chromium.googlesource.com/chromium/src/+/57b2e8fda5f0cfa3aacdb926cb3a845d5921704d

[calamity, 2017-06-26 04:46:42]

Description was changed from

==========
[MD Bookmarks] Restore focus after closing a dialog or context menu.

This CL makes dialogs in the bookmark manager restore focus to the
previously focused element after dismissal.

BUG=730905
==========

to

==========
[MD Bookmarks] Restore focus after closing a dialog or context menu.

This CL makes dialogs in the bookmark manager restore focus to the
previously focused element after dismissal.

BUG=730905
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-06-26 04:46:49]

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

[commit-bot: I haz the power, 2017-06-26 04:49:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 04:51:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 04:51:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[calamity, 2017-06-27 04:14:30]

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

[commit-bot: I haz the power, 2017-06-27 04:14:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 04:57:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 04:57:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2017-06-27 05:03:56]

Patchset #1 (id:1) has been deleted

[calamity, 2017-06-27 05:04:00]

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

[calamity, 2017-06-27 05:04:05]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-06-27 05:04:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 06:13:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 06:13:51]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-06-27 07:00:54]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[tsergeant, 2017-06-28 03:47:58]

looks pretty good overall! a nice, relatively small solution to a big problem

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dialog_focus_manager.js (right):

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:7: * Manages focus
restoration for modal dialogs.
I think it's worth going into a little more detail here. Maybe add an extra
sentence like

"After the final dialog in a stack is closed, restores focus to the element
which was focused when the first dialog was opened."

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:55: /** @private
*/
Nit: @return annotation

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:68:
getCloseListener_: function(dialog) {
This is nifty, I'm glad you figured out the async nastiness

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:88: },
Nit: this should be a ; not a ,

https://codereview.chromium.org/2954273002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right):

https://codereview.chromium.org/2954273002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:446:
dialogFocusManager = new bookmarks.DialogFocusManager();
You can also just do

bookmarks.DialogFocusManager.instance_ = null;

to reset the singleton without creating a new instance immediately.

[tsergeant, 2017-06-28 03:49:01]

https://codereview.chromium.org/2954273002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js (right):

https://codereview.chromium.org/2954273002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_focus_test.js:446:
dialogFocusManager = new bookmarks.DialogFocusManager();
On 2017/06/28 03:47:58, tsergeant wrote:
> You can also just do
> 
> bookmarks.DialogFocusManager.instance_ = null;
> 
> to reset the singleton without creating a new instance immediately.

Uh, ignore this comment, the suggestion is not what you're after.

[calamity, 2017-06-28 04:22:29]

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

[commit-bot: I haz the power, 2017-06-28 04:22:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-06-28 04:22:47]

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/dialog_focus_manager.js (right):

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:7: * Manages focus
restoration for modal dialogs.
On 2017/06/28 03:47:58, tsergeant wrote:
> I think it's worth going into a little more detail here. Maybe add an extra
> sentence like
> 
> "After the final dialog in a stack is closed, restores focus to the element
> which was focused when the first dialog was opened."

Done.

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:55: /** @private
*/
On 2017/06/28 03:47:58, tsergeant wrote:
> Nit: @return annotation

Done.

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:68:
getCloseListener_: function(dialog) {
On 2017/06/28 03:47:58, tsergeant wrote:
> This is nifty, I'm glad you figured out the async nastiness

Acknowledged.

https://codereview.chromium.org/2954273002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/dialog_focus_manager.js:88: },
On 2017/06/28 03:47:58, tsergeant wrote:
> Nit: this should be a ; not a ,

Done.

[tsergeant, 2017-06-28 04:49:18]

lgtm

https://codereview.chromium.org/2954273002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/app.html (right):

https://codereview.chromium.org/2954273002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/app.html:7: <link rel="import"
href="chrome://bookmarks/dialog_focus_manager.html">
Nit: Do you mind importing this in the places where it is used?

Primary benefit of doing that is that it makes the load order easier to reason
about if we decide to do lazy loading.

[calamity, 2017-06-29 06:19:34]

https://codereview.chromium.org/2954273002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/app.html (right):

https://codereview.chromium.org/2954273002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/app.html:7: <link rel="import"
href="chrome://bookmarks/dialog_focus_manager.html">
On 2017/06/28 04:49:18, tsergeant wrote:
> Nit: Do you mind importing this in the places where it is used?
> 
> Primary benefit of doing that is that it makes the load order easier to reason
> about if we decide to do lazy loading.

Done.

[calamity, 2017-06-29 06:19:41]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-06-29 06:19:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2954273002/#ps120001
(title: "remove dep")

[commit-bot: I haz the power, 2017-06-29 06:19:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 06:28:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 06:28:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[calamity, 2017-06-29 07:03:04]

Patchset #5 (id:120001) has been deleted

[calamity, 2017-06-29 07:03:51]

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

[commit-bot: I haz the power, 2017-06-29 07:03:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-06-29 07:04:23]

calamity@chromium.org changed reviewers:
+ thestig@chromium.org

[calamity, 2017-06-29 07:04:24]

+thestig@ for resource_ids

PTAL, thanks!

[commit-bot: I haz the power, 2017-06-29 08:07:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 08:07:55]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Lei Zhang, 2017-06-29 17:32:53]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2017-06-29 17:32:54]

lgtm

[Lei Zhang, 2017-06-29 17:32:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2954273002/#ps140001
(title: "fix deps")

[commit-bot: I haz the power, 2017-06-29 17:33:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 18:47:00]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498757573348070,
"parent_rev": "531f38bd194f9d8a0dd98d78a15f82947d742cec", "commit_rev":
"57b2e8fda5f0cfa3aacdb926cb3a845d5921704d"}

[commit-bot: I haz the power, 2017-06-29 18:47:14]

Description was changed from

==========
[MD Bookmarks] Restore focus after closing a dialog or context menu.

This CL makes dialogs in the bookmark manager restore focus to the
previously focused element after dismissal.

BUG=730905
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Restore focus after closing a dialog or context menu.

This CL makes dialogs in the bookmark manager restore focus to the
previously focused element after dismissal.

BUG=730905
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2954273002
Cr-Commit-Position: refs/heads/master@{#483440}
Committed:
https://chromium.googlesource.com/chromium/src/+/57b2e8fda5f0cfa3aacdb926cb3a...
==========

[commit-bot: I haz the power, 2017-06-29 18:47:16]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/57b2e8fda5f0cfa3aacdb926cb3a...