MD Settings: Implementing modal popup/action menus.

MD Settings: Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

Committed: https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4
Cr-Commit-Position: refs/heads/master@{#425534}

[dpapad, 2016-10-07 01:44:44]

Description was changed from

==========
MD Settings; Implementing modal popup/action menus, WIP.

BUG=639718
==========

to

==========
MD Settings; Implementing modal popup/action menus, WIP.

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

[dpapad, 2016-10-11 19:10:35]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-10-12 01:20:23]

Description was changed from

==========
MD Settings; Implementing modal popup/action menus, WIP.

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

to

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new cr-action-menu element implemented as a
native <dialog>. This solves a suite of problems that current action menu
implementation suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems

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

[dpapad, 2016-10-12 01:38:20]

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

[commit-bot: I haz the power, 2016-10-12 01:38:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-10-12 01:38:47]

Description was changed from

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new cr-action-menu element implemented as a
native <dialog>. This solves a suite of problems that current action menu
implementation suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems

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

to

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems

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

[commit-bot: I haz the power, 2016-10-12 02:08:57]

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

[commit-bot: I haz the power, 2016-10-12 02:08:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-10-12 23:32:18]

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

[commit-bot: I haz the power, 2016-10-12 23:32:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-10-13 00:20:22]

Description was changed from

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems

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

to

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

[dpapad, 2016-10-13 00:20:53]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-10-13 00:20:54]

Screenshots at http://imgur.com/a/Vv9qM. Green tests at patch 10.

[dpapad, 2016-10-13 00:23:17]

https://codereview.chromium.org/2402553002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:127: this.style.right
= right + 'px';
I don't know if this is a blink bug, but
 - when RTL, I can only set 'right'. 'left' is ignored.
 - when LTR, I can only set 'left'. 'right' is ignored.

I'll try to create a minimal repro, but this explains why I am setting right for
RTL, and left for LTR.

[Dan Beam, 2016-10-13 00:28:34]

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/cr_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:26: onWindowResize_: null,
nit: just do this after the ctor as like "boundResize_" imo

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:62: e.key !== 'ArrowLeft' &&
e.key !== 'ArrowRight') {
nit: just drop 'ArrowLeft' and 'ArrowRight' for now, IMO

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:68: if (nextButton !== null)
if (nextButton)

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:110: this.showModal();
why does showModal() happen before positioning?

https://codereview.chromium.org/2402553002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:127: this.style.right
= right + 'px';
On 2016/10/13 00:23:17, dpapad wrote:
> I don't know if this is a blink bug, but
>  - when RTL, I can only set 'right'. 'left' is ignored.
>  - when LTR, I can only set 'left'. 'right' is ignored.
> 
> I'll try to create a minimal repro, but this explains why I am setting right
for
> RTL, and left for LTR.

no, that's known

[dpapad, 2016-10-13 00:51:22]

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/cr_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:26: onWindowResize_: null,
On 2016/10/13 at 00:28:34, Dan Beam wrote:
> nit: just do this after the ctor as like "boundResize_" imo

By ctor do you mean "attached"?

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:62: e.key !== 'ArrowLeft' &&
e.key !== 'ArrowRight') {
On 2016/10/13 at 00:28:34, Dan Beam wrote:
> nit: just drop 'ArrowLeft' and 'ArrowRight' for now, IMO

Done.

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:68: if (nextButton !== null)
On 2016/10/13 at 00:28:33, Dan Beam wrote:
> if (nextButton)

Done.

https://codereview.chromium.org/2402553002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/cr_action_menu.js:110: this.showModal();
On 2016/10/13 at 00:28:34, Dan Beam wrote:
> why does showModal() happen before positioning?

Because this.offsetWidth, this.offsetHeight are not set before calling
setModal(). BTW, you are reviewing an older patch. This is not latest version.

[Dan Beam, 2016-10-13 05:03:01]

didn't get to the tests yet

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:18: <link
rel="import" href="/settings_action_menu.html">
alpha

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.html (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.html:10: box-shadow: 0
2px 6px var(--paper-grey-500);
is it possible to share this somehow with wherever you're copying from?

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:30:
this.setAttribute('tabindex', 0);
nit:

hostAttributes: {
  tabindex: 0,
},

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:48:
this.addEventListener('click', function(e) {
do you need to remove these events on detached as well?

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:88: this.focusedIndex_
= this.options_.length - 1;
var numOptions = this.options_.length;
this.focusedIndex_ = (numOptions + this.focusedIndex_ + step) % numOptions;

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:95: } else {
don't use else after return

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:100: return
getNextOptionRec_();
var enabled = Array.from(this.options_).filter(function(option) {
  return !(option.disabled || option.hidden);
});
// check out this logic:
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_profil...

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:105: * @param
{!ClientRect} rect
can this just take an element instead of a clientrect?

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:110:
this.focusedIndex_ = 0;
can we just start this at -1 and call getNextOption(1)?

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:122: if
(directionDelegate.isRtl()) {
maybe this?

if (new settings.DirectionDelegateImpl().isRtl()) {

[dpapad, 2016-10-13 18:18:01]

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:18: <link
rel="import" href="/settings_action_menu.html">
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> alpha

Done.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.html (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.html:10: box-shadow: 0
2px 6px var(--paper-grey-500);
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> is it possible to share this somehow with wherever you're copying from?

This is copied from
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....
When the migration to settings-action-menu is done, there will be no
iron-dropdown in the codebase, and there will be no copying (the CSS rule will
be deleted from settings_shared_css.html). The one here should be the only
occurrence. So I am not inclined to share it with a soon to be gone code.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:30:
this.setAttribute('tabindex', 0);
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> nit:
> 
> hostAttributes: {
>   tabindex: 0,
> },

Done.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:48:
this.addEventListener('click', function(e) {
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> do you need to remove these events on detached as well?

I don't think so. Firstly because ready() is only executed once, even if you
attach/detach multiple times AFAIU.
Secondly, even if the previous is not true (which I think it is), there is no
case where we are detaching a dialog and keeping it around to attach it later. 
We only attach it once, and call showModal/close as many times as necessary
(which does not trigger attached/ready again).

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:88: this.focusedIndex_
= this.options_.length - 1;
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> var numOptions = this.options_.length;
> this.focusedIndex_ = (numOptions + this.focusedIndex_ + step) % numOptions;

Done.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:95: } else {
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> don't use else after return

Done.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:100: return
getNextOptionRec_();
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> var enabled = Array.from(this.options_).filter(function(option) {
>   return !(option.disabled || option.hidden);
> });

Getting a filtered array of the enabled elements first would make it impossible
(without searching again) to retrieve the index of the next element in the
unfiltered this.options_ array, to assign it to this.focusedIndex_. I'd be happy
to pair program a non-recursive solution if you want, but as your suggestion
stands, I don't think it would work (or I am misunderstanding it).


> // check out this logic:
>
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_profil...

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:110:
this.focusedIndex_ = 0;
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> can we just start this at -1 and call getNextOption(1)?

Done.

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:122: if
(directionDelegate.isRtl()) {
On 2016/10/13 at 05:03:01, Dan Beam wrote:
> maybe this?
> 
> if (new settings.DirectionDelegateImpl().isRtl()) {

Done.

[dpapad, 2016-10-13 18:58:25]

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:100: return
getNextOptionRec_();
On 2016/10/13 at 18:18:01, dpapad wrote:
> On 2016/10/13 at 05:03:01, Dan Beam wrote:
> > var enabled = Array.from(this.options_).filter(function(option) {
> >   return !(option.disabled || option.hidden);
> > });
> 
> Getting a filtered array of the enabled elements first would make it
impossible (without searching again) to retrieve the index of the next element
in the unfiltered this.options_ array, to assign it to this.focusedIndex_. I'd
be happy to pair program a non-recursive solution if you want, but as your
suggestion stands, I don't think it would work (or I am misunderstanding it).
> 
> 
> > // check out this logic:
> >
https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_profil...

Implemented a non-recursive version in latest patch. Looks much simpler.

[Dan Beam, 2016-10-13 19:01:24]

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:105: * @param
{!ClientRect} rect
On 2016/10/13 05:03:01, Dan Beam wrote:
> can this just take an element instead of a clientrect?

ping

[Dan Beam, 2016-10-13 19:02:50]

https://codereview.chromium.org/2402553002/diff/270001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/270001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:106: showAtLocation:
function(anchorElement) {
ah, nevermind

can we just rename this to show() or show{For,With}Anchor() or something?

[dpapad, 2016-10-13 19:17:41]

https://codereview.chromium.org/2402553002/diff/270001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/270001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:106: showAtLocation:
function(anchorElement) {
On 2016/10/13 at 19:02:50, Dan Beam wrote:
> ah, nevermind
> 
> can we just rename this to show() or show{For,With}Anchor() or something?

Renamed to showAt().

[dpapad, 2016-10-13 21:09:25]

Patchset #17 (id:330001) has been deleted

[dpapad, 2016-10-13 21:11:24]

Added unit tests, which revealed a small bug!

[dpapad, 2016-10-13 21:13:44]

Description was changed from

==========
MD Settings; Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

to

==========
MD Settings: Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

[dpapad, 2016-10-13 21:21:32]

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

[commit-bot: I haz the power, 2016-10-13 21:21:57]

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, 2016-10-13 21:36:35]

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

[commit-bot: I haz the power, 2016-10-13 21:36:35]

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...)

[dpapad, 2016-10-13 22:21:18]

Fixed closure compilation. PTAL.

[dpapad, 2016-10-13 23:14:54]

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

[commit-bot: I haz the power, 2016-10-13 23:15:19]

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, 2016-10-14 00:06:34]

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

[commit-bot: I haz the power, 2016-10-14 00:06:34]

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

[dpapad, 2016-10-14 00:39:29]

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

[commit-bot: I haz the power, 2016-10-14 00:40:05]

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, 2016-10-14 00:51:18]

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

[commit-bot: I haz the power, 2016-10-14 00:51:19]

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...)

[Dan Beam, 2016-10-14 03:51:13]

these tests are awesome!

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:51:
this.addEventListener('click', function(e) {
can you move these both to:

listeners: {
  'click': 'onClick_',
  'keydown': 'onKeyDown_',
},

so we re-use more polymer magic (and they're attached() and detached()
correctly?)

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:51:
this.addEventListener('click', function(e) {
should this be equivalent to on-tap in some way?  does this work with touch?

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:58:
this.addEventListener('keydown', function(e) {
can we use iron-a11y-keys-behavior?

behaviors: [Polymer.IronA11yKeysBehavior],

keyBindings: {
  'down': 'onUpOrDown_',
  'tab': 'close',
  'up': 'onUpOrDown_',
},

onUpOrDown_: function(e) {
  var next = this.getNextOption_(this.keyboardEventMatchesKeys('up') ? -1 : 1);
  if (next)
    next.focus();
  e.preventDefault();
},

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:117: }.bind(this))();
why can't you just use NodeList#forEach?
https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach

options.forEach(function(option) {
  option.removeAttribute('autofocus');
});

if you can't (for some reason), this is cleaner:

}.call(this));

as is this:

(function(options) {
  for (var option of options)
    option.removeAttribute('autofocus');
})(this.options_);

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:119: if (nextOption
!== null)
if (nextOption)

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_action_menu_test.js (right):

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:8: var buttons =
null;
nit: @types if you can

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:14: <button
id='dots'>...</button>
nit: use " instead of '

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:23: buttons =
menu.querySelectorAll('button');
arguable nit: use .dropdown-item / items instead

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:35:
assertEquals(buttons[0], menu.querySelector('[autofocus]'));
do you want to check that this is the only [autofocus] thing as well (not just
the first?)

[dpapad, 2016-10-14 17:39:23]

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:51:
this.addEventListener('click', function(e) {
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> can you move these both to:
> 
> listeners: {
>   'click': 'onClick_',
>   'keydown': 'onKeyDown_',
> },
> 
> so we re-use more polymer magic (and they're attached() and detached()
correctly?)

Done.

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:58:
this.addEventListener('keydown', function(e) {
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> can we use iron-a11y-keys-behavior?
> 
> behaviors: [Polymer.IronA11yKeysBehavior],
> 
> keyBindings: {
>   'down': 'onUpOrDown_',
>   'tab': 'close',
>   'up': 'onUpOrDown_',
> },
> 
> onUpOrDown_: function(e) {
>   var next = this.getNextOption_(this.keyboardEventMatchesKeys('up') ? -1 :
1);
>   if (next)
>     next.focus();
>   e.preventDefault();
> },

What is the benefit in doing this? By reading the documentation and looking at
the class itself

- part of that class is simply dealing with browser differences.
  "The element takes care of browser differences with respect to Keyboard
events"
- It defines a lot of enums that we don't need,
- Deals with key combos, which we don't need.
- It is is 500 LOC vs 14 LOC currently needed here.

So, I prefer not to use it, because I don't see a benefit from using it.

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:117: }.bind(this))();
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> why can't you just use NodeList#forEach?
> https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach
> 
> options.forEach(function(option) {
>   option.removeAttribute('autofocus');
> });
> 
> if you can't (for some reason), this is cleaner:

Because compiler.....
(ERROR) Error in: settings_action_menu.js
##
/usr/local/google/home/dpapad/workspace/chromium1/src/chrome/browser/resources/settings/settings_action_menu.js:123:
ERROR - Property forEach never defined on NodeList<Element>
##     this.options_.forEach(function(option) {


> 
> }.call(this));
> 
> as is this:
> 
> (function(options) {
>   for (var option of options)
>     option.removeAttribute('autofocus');
> })(this.options_);

Done.

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:119: if (nextOption
!== null)
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> if (nextOption)

Done.

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_action_menu_test.js (right):

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:8: var buttons =
null;
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> nit: @types if you can

Done.

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:23: buttons =
menu.querySelectorAll('button');
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> arguable nit: use .dropdown-item / items instead

Done.

https://codereview.chromium.org/2402553002/diff/430001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:35:
assertEquals(buttons[0], menu.querySelector('[autofocus]'));
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> do you want to check that this is the only [autofocus] thing as well (not just
the first?)

Done, good idea.

[dpapad, 2016-10-14 17:49:09]

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/430001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:51:
this.addEventListener('click', function(e) {
On 2016/10/14 at 03:51:13, Dan Beam wrote:
> should this be equivalent to on-tap in some way?  does this work with touch?

Changed to tap, even though both seemed to work using just 'click' (using the
device emulator).

[dpapad, 2016-10-14 21:14:02]

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

[commit-bot: I haz the power, 2016-10-14 21:14:45]

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, 2016-10-14 22:03:57]

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

[commit-bot: I haz the power, 2016-10-14 22:03:58]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-10-14 23:31:06]

lgtm

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/compiled_resources2.gyp (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/compiled_resources2.gyp:78:
':direction_delegate',
just 'direction_delegate' (no ':' required)

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:451: 
^H

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.html (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.html:16: :host::backdrop
{
is :host required in this selector?

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:34: 'keydown':
'onKeyDown_',
nit: alpha

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:45: 
remove \n

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:113:
window.removeEventListener('resize', this.onWindowResize_);
nit: call this.detached() or make a removeResizeListener_ and call from both?

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:138: 
nit: do this here and only if you haven't already

this.onWindowResize_ = this.onWindowResize_ || function() {
  if (this.open)
    this.close();
}.bind(this);

i don't really care if this.onWindowResize_ is undefined in detached

https://codereview.chromium.org/2402553002/diff/490001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_action_menu_test.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:45:
assertFocus(items[0]);
nit: assertFocused*

[dpapad, 2016-10-15 01:02:09]

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/compiled_resources2.gyp (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/compiled_resources2.gyp:78:
':direction_delegate',
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> just 'direction_delegate' (no ':' required)

Done.

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:451: 
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> ^H

^Done

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.html (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.html:16: :host::backdrop
{
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> is :host required in this selector?

Yes. Tried simply ::backdrop and did not work.

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_action_menu.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:34: 'keydown':
'onKeyDown_',
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> nit: alpha

Meh, done.

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:45: 
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> remove \n

Done.

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:113:
window.removeEventListener('resize', this.onWindowResize_);
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> nit: call this.detached() or make a removeResizeListener_ and call from both?

I prefer not to call a lifecycle method manually, since it is supposed to be
called by the Polymer framework itself. Added a removeRisezeListener_() instead.

https://codereview.chromium.org/2402553002/diff/490001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_action_menu.js:138: 
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> nit: do this here and only if you haven't already
> 
> this.onWindowResize_ = this.onWindowResize_ || function() {
>   if (this.open)
>     this.close();
> }.bind(this);
> 
> i don't really care if this.onWindowResize_ is undefined in detached

Done.

https://codereview.chromium.org/2402553002/diff/490001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_action_menu_test.js (right):

https://codereview.chromium.org/2402553002/diff/490001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_action_menu_test.js:45:
assertFocus(items[0]);
On 2016/10/14 at 23:31:06, Dan Beam wrote:
> nit: assertFocused*

Done.

[dpapad, 2016-10-15 01:03:35]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-10-15 01:03:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2402553002/#ps510001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-15 01:03:54]

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, 2016-10-15 02:12:06]

Committed patchset #25 (id:510001)

[commit-bot: I haz the power, 2016-10-15 02:14:03]

Description was changed from

==========
MD Settings: Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

to

==========
MD Settings: Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

Committed: https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4
Cr-Commit-Position: refs/heads/master@{#425534}
==========

[commit-bot: I haz the power, 2016-10-15 02:14:05]

Patchset 25 (id:??) landed as
https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4
Cr-Commit-Position: refs/heads/master@{#425534}