[MD Bookmarks] Refactor focus ring hiding.

[MD Bookmarks] Refactor focus ring hiding.

This CL refactors how mouse focus hides the focus ring by moving the
listener into the app and using keyboard events to restore the focus ring
instead of blur event. This reduces the number of event listeners and deals
well with window focus/blur.

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

Review-Url: https://codereview.chromium.org/2981503002
Cr-Commit-Position: refs/heads/master@{#486675}
Committed: https://chromium.googlesource.com/chromium/src/+/d79887d431cc6fb7e494f92ac763c2b50d7fb0e7

[calamity, 2017-07-11 07:33:07]

Description was changed from

==========
[MD Bookmarks] Refactor focus ring hiding.

This CL refactors how mouse focus hides the focus ring by moving the
listener into the app and using keyboard events to restore the focus ring
instead of blur event. This reduces the number of event listeners and deals
well with window focus/blur.

BUG=740834
==========

to

==========
[MD Bookmarks] Refactor focus ring hiding.

This CL refactors how mouse focus hides the focus ring by moving the
listener into the app and using keyboard events to restore the focus ring
instead of blur event. This reduces the number of event listeners and deals
well with window focus/blur.

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

[calamity, 2017-07-11 07:43:48]

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

[commit-bot: I haz the power, 2017-07-11 07:43:55]

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-07-11 08:03:27]

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

[commit-bot: I haz the power, 2017-07-11 08:03:28]

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

[calamity, 2017-07-12 05:48:19]

Patchset #1 (id:1) has been deleted

[calamity, 2017-07-12 05:48:29]

Patchset #1 (id:20001) has been deleted

[calamity, 2017-07-12 05:48:56]

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

[calamity, 2017-07-12 05:48:57]

Settled for the keydown handler since it makes life so much easier.

[tsergeant, 2017-07-12 07:32:01]

Great! I like this a lot better than the old version.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:76: on-focus="clearMouseFocus">
This listener doesn't exist anymore.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js (right):

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:11: * when it
gains focus from the mouse, allowing the outline to be hidden
Nit: This comment is a little misleading, since we don't actually listen to
focus anymore. How about:

...when the user interacts with it using the mouse, allowing the focus outline
to be hidden without affecting keyboard users.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:18:
this.boundsOnKeydown = this.onKeydown_.bind(this);
Nit: 'bound', not 'bounds'

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:28:
this.removeEventListener('contextmenu', this.boundOnMousedown_);
Is this necessary? right-clicking will still fire a mousedown event.

[calamity, 2017-07-13 08:03:06]

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.html (right):

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.html:76: on-focus="clearMouseFocus">
On 2017/07/12 07:32:00, tsergeant wrote:
> This listener doesn't exist anymore.

Done.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js (right):

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:11: * when it
gains focus from the mouse, allowing the outline to be hidden
On 2017/07/12 07:32:00, tsergeant wrote:
> Nit: This comment is a little misleading, since we don't actually listen to
> focus anymore. How about:
> 
> ...when the user interacts with it using the mouse, allowing the focus outline
> to be hidden without affecting keyboard users.

Done.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:18:
this.boundsOnKeydown = this.onKeydown_.bind(this);
On 2017/07/12 07:32:00, tsergeant wrote:
> Nit: 'bound', not 'bounds'

Write once, typo everywhere.

https://codereview.chromium.org/2981503002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/mouse_focus_behavior.js:28:
this.removeEventListener('contextmenu', this.boundOnMousedown_);
On 2017/07/12 07:32:00, tsergeant wrote:
> Is this necessary? right-clicking will still fire a mousedown event.

Removed.

[tsergeant, 2017-07-14 00:58:37]

lgtm!

[calamity, 2017-07-14 02:33:55]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-07-14 02:34:04]

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-07-14 04:43:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499999635010690,
"parent_rev": "1f619b96fcd162cada464ff0ab7883f7f3d21df2", "commit_rev":
"d79887d431cc6fb7e494f92ac763c2b50d7fb0e7"}

[commit-bot: I haz the power, 2017-07-14 04:44:12]

Description was changed from

==========
[MD Bookmarks] Refactor focus ring hiding.

This CL refactors how mouse focus hides the focus ring by moving the
listener into the app and using keyboard events to restore the focus ring
instead of blur event. This reduces the number of event listeners and deals
well with window focus/blur.

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

to

==========
[MD Bookmarks] Refactor focus ring hiding.

This CL refactors how mouse focus hides the focus ring by moving the
listener into the app and using keyboard events to restore the focus ring
instead of blur event. This reduces the number of event listeners and deals
well with window focus/blur.

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

Review-Url: https://codereview.chromium.org/2981503002
Cr-Commit-Position: refs/heads/master@{#486675}
Committed:
https://chromium.googlesource.com/chromium/src/+/d79887d431cc6fb7e494f92ac763...
==========

[commit-bot: I haz the power, 2017-07-14 04:44:14]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d79887d431cc6fb7e494f92ac763...