MD Settings: update dialogs to focus without ink when using mouse

MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[Dan Beam, 2017-04-28 02:40:04]

Description was changed from

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
==========

to

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-04-28 06:05:09]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2017-04-28 06:05:28]

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

[commit-bot: I haz the power, 2017-04-28 06:05:39]

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-04-28 06:09:30]

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

[commit-bot: I haz the power, 2017-04-28 06:09:31]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Dan Beam, 2017-04-28 06:25:36]

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

[commit-bot: I haz the power, 2017-04-28 06:25:48]

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-04-28 07:32:49]

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

[commit-bot: I haz the power, 2017-04-28 07:32:50]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-04-28 17:36:46]

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/compiled_resources.gyp (right):

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/compiled_resources.gyp:24:
'../../../../ui/webui/resources/js/cr/ui/focus_without_ink.js',
The rest of this CL does not touch any file in resources/options/ ? Is this
necessary?

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.js
(right):

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:97:
settings.focusWithoutInk(this.$$('paper-button'));
Should this be cr.ui.focusWithoutInk (here and elsewhere, if any)?

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_without_ink.js (right):

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:9: function
focusWithoutInk(toFocus) {
Can you add a short sentence explaining the purpose of this function (here or at
the top of this file). The "ink" terminology is only familiar within the Polymer
context, but there is nothing here explicitly stating that.

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:10: // TODO(dbeam):
assert('noink' in toFocus);
Why not doing this as part of this CL?

[Dan Beam, 2017-04-29 02:56:31]

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/compiled_resources.gyp (right):

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/compiled_resources.gyp:24:
'../../../../ui/webui/resources/js/cr/ui/focus_without_ink.js',
On 2017/04/28 17:36:46, dpapad wrote:
> The rest of this CL does not touch any file in resources/options/ ? Is this
> necessary?

settings/ is depended on by options/ on ChromeOS

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.js
(right):

https://codereview.chromium.org/2842303004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:97:
settings.focusWithoutInk(this.$$('paper-button'));
On 2017/04/28 17:36:46, dpapad wrote:
> Should this be cr.ui.focusWithoutInk (here and elsewhere, if any)?

Done. (whoops, guess this file is neither tested nor compiled :/)

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_without_ink.js (right):

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:9: function
focusWithoutInk(toFocus) {
On 2017/04/28 17:36:46, dpapad wrote:
> Can you add a short sentence explaining the purpose of this function (here or
at
> the top of this file). The "ink" terminology is only familiar within the
Polymer
> context, but there is nothing here explicitly stating that.

Done.

https://codereview.chromium.org/2842303004/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:10: // TODO(dbeam):
assert('noink' in toFocus);
On 2017/04/28 17:36:46, dpapad wrote:
> Why not doing this as part of this CL?

if we don't merge, i'll add it now.  if we do merge, i'd leave it as a TODO and
do it right after committing.

[Dan Beam, 2017-04-29 02:56:39]

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

[commit-bot: I haz the power, 2017-04-29 02:56:47]

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-04-29 04:10:09]

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

[commit-bot: I haz the power, 2017-04-29 04:10:09]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-01 23:51:40]

so i diverged a little bit from what FocusOutlineManager does because it's not
able to get various focus changes through multiple layers of shadow DOM and I
think this code works in a more expected way without a public, external side
effective (the .focus-outline-visible class)

[Dan Beam, 2017-05-01 23:51:45]

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

[commit-bot: I haz the power, 2017-05-01 23:52:27]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-02 00:13:22]

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_without_ink.js (right):

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:8:
document.addEventListener('mousedown', function() {
As discussed in person, can we make this file re-entrant, such that duplicate
listeners are not registered if this file is imported more than once?

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:11:
document.addEventListener('touchstart', function() {
Maybe we can use 'pointerdown' instead of mousedown+touchstart? We already use
it at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:23: * @param {Element} toFocus
I think focus() only exists on HTMLElemnet, but not Element. Also, parameter is
not nullable so let's add a "!".

[commit-bot: I haz the power, 2017-05-02 00:47:53]

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

[commit-bot: I haz the power, 2017-05-02 00:47:55]

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

[Dan Beam, 2017-05-02 02:55:11]

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/focus_without_ink.js (right):

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:8:
document.addEventListener('mousedown', function() {
On 2017/05/02 00:13:22, dpapad wrote:
> As discussed in person, can we make this file re-entrant, such that duplicate
> listeners are not registered if this file is imported more than once?

Done.

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:11:
document.addEventListener('touchstart', function() {
On 2017/05/02 00:13:22, dpapad wrote:
> Maybe we can use 'pointerdown' instead of mousedown+touchstart? We already use
> it at
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....

Done.

https://codereview.chromium.org/2842303004/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/focus_without_ink.js:23: * @param {Element} toFocus
On 2017/05/02 00:13:22, dpapad wrote:
> I think focus() only exists on HTMLElemnet, but not Element. Also, parameter
is
> not nullable so let's add a "!".

Done. (I originally didn't do this intentionally to not add a bunch of
assert()s)

[Dan Beam, 2017-05-02 02:55:20]

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

[commit-bot: I haz the power, 2017-05-02 02:55:46]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-05-02 03:03:43]

LGTM

https://codereview.chromium.org/2842303004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js
(right):

https://codereview.chromium.org/2842303004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:96:
cr.ui.focusWithoutInk(assert(this.$$('#remove button')));
Nit (not for this CL): We can probably use getRequiredElement and
queryRequiredElement [1] in a follow up to remove assert() statements across our
code.

[1]
https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&...

[commit-bot: I haz the power, 2017-05-02 03:36:15]

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

[commit-bot: I haz the power, 2017-05-02 03:36:16]

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

[Dan Beam, 2017-05-02 21:03:08]

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

[commit-bot: I haz the power, 2017-05-02 21:03:39]

Dry run: CQ is trying da patch.

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

[Dan Beam, 2017-05-02 21:34:25]

https://codereview.chromium.org/2842303004/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js
(right):

https://codereview.chromium.org/2842303004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/android_apps_page/android_apps_subpage.js:96:
cr.ui.focusWithoutInk(assert(this.$$('#remove button')));
On 2017/05/02 03:03:43, dpapad wrote:
> Nit (not for this CL): We can probably use getRequiredElement and
> queryRequiredElement [1] in a follow up to remove assert() statements across
our
> code.
> 
> [1]
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=util.js&...

Acknowledged.

[Dan Beam, 2017-05-02 21:35:17]

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

[commit-bot: I haz the power, 2017-05-02 21:36:00]

Dry run: CQ is trying da patch.

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

[Dan Beam, 2017-05-02 22:54:50]

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

[commit-bot: I haz the power, 2017-05-02 22:55:11]

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-05-02 22:59:25]

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

[commit-bot: I haz the power, 2017-05-02 22:59:26]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Dan Beam, 2017-05-02 23:57:46]

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

[commit-bot: I haz the power, 2017-05-02 23:58:09]

Dry run: CQ is trying da patch.

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

[tsergeant, 2017-05-03 00:43:19]

Description was changed from

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-05-03 01:22:42]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2017-05-03 01:22:57]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-05-03 01:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2842303004/#ps200001
(title: "merge")

[commit-bot: I haz the power, 2017-05-03 01:23:51]

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-05-03 01:57:18]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493774577550200,
"parent_rev": "c9acca9df826b31c2b58b50565172428396afb30", "commit_rev":
"fff67725b349e1b90c1545720e91de6ad9479192"}

[commit-bot: I haz the power, 2017-05-03 01:57:29]

Description was changed from

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: update dialogs to focus without ink when using mouse

R=dpapad@chromium.org
BUG=716184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2017-05-03 01:57:31]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/fff67725b349e1b90c1545720e91...