Make MD Settings side bar keyboard accessible.

Make MD Settings side bar keyboard accessible.

Screenshots in bug.

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

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

[hcarmona, 2017-01-26 00:11:55]

Description was changed from

==========
Make MD Settings side bar keyboard accessible.

BUG=633858
==========

to

==========
Make MD Settings side bar keyboard accessible.

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

[hcarmona, 2017-01-26 00:11:57]

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

[commit-bot: I haz the power, 2017-01-26 00:13:14]

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-01-26 00:17:25]

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

[commit-bot: I haz the power, 2017-01-26 00:17:26]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hcarmona, 2017-01-26 19:40:11]

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

[commit-bot: I haz the power, 2017-01-26 19:40: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, 2017-01-26 21:36:53]

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

[commit-bot: I haz the power, 2017-01-26 21:36:54]

Dry run: This issue passed the CQ dry run.

[hcarmona, 2017-01-26 22:16:52]

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

[hcarmona, 2017-01-26 22:17:19]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-26 22:17:21]

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

[hcarmona, 2017-01-26 22:17:27]

Patchset #1 (id:20001) has been deleted

[hcarmona, 2017-01-26 22:18:23]

Description was changed from

==========
Make MD Settings side bar keyboard accessible.

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

to

==========
Make MD Settings side bar keyboard accessible.

Screenshots in bug.

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

[hcarmona, 2017-01-26 22:18:23]

hcarmona@chromium.org changed reviewers:
+ dschuyler@chromium.org

[hcarmona, 2017-01-26 22:19:18]

PTAL

[commit-bot: I haz the power, 2017-01-26 23:29:52]

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

[commit-bot: I haz the power, 2017-01-26 23:29:53]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-27 00:25:28]

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
<paper-button data-path="/internet" on-tap="openPage_">
How much of a performance difference is there from changing these from div to
paper-button?

[hcarmona, 2017-01-27 22:54:51]

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
<paper-button data-path="/internet" on-tap="openPage_">
On 2017/01/27 00:25:28, dschuyler wrote:
> How much of a performance difference is there from changing these from div to
> paper-button?

Ok, ran Tommy's testing and change is negligible average load time went from
1173ms to 1181ms. (20 runs each)

This entire section is behind a dom-if, so 'no change' makes sense.

Are there other tests that I should run?

[Dan Beam, 2017-01-27 23:01:17]

On 2017/01/27 22:54:51, hcarmona wrote:
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_menu/settings_menu.html
(right):
> 
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
> <paper-button data-path="/internet" on-tap="openPage_">
> On 2017/01/27 00:25:28, dschuyler wrote:
> > How much of a performance difference is there from changing these from div
to
> > paper-button?
> 
> Ok, ran Tommy's testing and change is negligible average load time went from
> 1173ms to 1181ms. (20 runs each)
> 
> This entire section is behind a dom-if, so 'no change' makes sense.
> 
> Are there other tests that I should run?

how much work is done with that dom-if flips to true?  i.e. on your big beefy
google linux machine, how long is the thread jammed up (you can check with dev
tools)_when you click the hotdogs menu for the first time to show the sidenav?

[hcarmona, 2017-01-30 19:01:43]

Is 17ms -> 32ms bad?

This happens on a user clicking a button before an animation, so time change
isn't noticeable, but it is measurable.

If we update this to not use a button, we would still add a ripple to match the
history page's behavior.

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
<paper-button data-path="/internet" on-tap="openPage_">
On 2017/01/27 22:54:51, hcarmona wrote:
> On 2017/01/27 00:25:28, dschuyler wrote:
> > How much of a performance difference is there from changing these from div
to
> > paper-button?
> 
> Ok, ran Tommy's testing and change is negligible average load time went from
> 1173ms to 1181ms. (20 runs each)
> 
> This entire section is behind a dom-if, so 'no change' makes sense.
> 
> Are there other tests that I should run?

Tested on my beefy dev machine and without change stamping is ~17ms. With change
stamping is ~32ms. This is also consistent on my macbook.

[Dan Beam, 2017-01-30 19:11:18]

On 2017/01/30 19:01:43, hcarmona wrote:
> Is 17ms -> 32ms bad?
> 
> This happens on a user clicking a button before an animation, so time change
> isn't noticeable, but it is measurable.
> 
> If we update this to not use a button, we would still add a ripple to match
the
> history page's behavior.
> 
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_menu/settings_menu.html
(right):
> 
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
> <paper-button data-path="/internet" on-tap="openPage_">
> On 2017/01/27 22:54:51, hcarmona wrote:
> > On 2017/01/27 00:25:28, dschuyler wrote:
> > > How much of a performance difference is there from changing these from div
> to
> > > paper-button?
> > 
> > Ok, ran Tommy's testing and change is negligible average load time went from
> > 1173ms to 1181ms. (20 runs each)
> > 
> > This entire section is behind a dom-if, so 'no change' makes sense.
> > 
> > Are there other tests that I should run?
> 
> Tested on my beefy dev machine and without change stamping is ~17ms. With
change
> stamping is ~32ms. This is also consistent on my macbook.

that's not awesome

[dschuyler, 2017-01-30 19:24:18]

On 2017/01/30 19:01:43, hcarmona wrote:
> Is 17ms -> 32ms bad?
> 
> This happens on a user clicking a button before an animation, so time change
> isn't noticeable, but it is measurable.
> 
> If we update this to not use a button, we would still add a ripple to match
the
> history page's behavior.
> 
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_menu/settings_menu.html
(right):
> 
>
https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
> <paper-button data-path="/internet" on-tap="openPage_">
> On 2017/01/27 22:54:51, hcarmona wrote:
> > On 2017/01/27 00:25:28, dschuyler wrote:
> > > How much of a performance difference is there from changing these from div
> to
> > > paper-button?
> > 
> > Ok, ran Tommy's testing and change is negligible average load time went from
> > 1173ms to 1181ms. (20 runs each)
> > 
> > This entire section is behind a dom-if, so 'no change' makes sense.
> > 
> > Are there other tests that I should run?
> 
> Tested on my beefy dev machine and without change stamping is ~17ms. With
change
> stamping is ~32ms. This is also consistent on my macbook.

I think it's worth looking into other options. Do we know whether getting the
features and speed will cost extra hours, days or weeks of dev time. (I'm
confident it can be done, but I'm unsure of the cost).

[hcarmona, 2017-02-01 00:38:15]

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

[commit-bot: I haz the power, 2017-02-01 00:38:43]

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

[hcarmona, 2017-02-01 00:39:48]

action-link'd as per our discussion. WDYT?

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:98:
<paper-button data-path="/internet" on-tap="openPage_">
On 2017/01/30 19:01:43, hcarmona wrote:
> On 2017/01/27 22:54:51, hcarmona wrote:
> > On 2017/01/27 00:25:28, dschuyler wrote:
> > > How much of a performance difference is there from changing these from div
> to
> > > paper-button?
> > 
> > Ok, ran Tommy's testing and change is negligible average load time went from
> > 1173ms to 1181ms. (20 runs each)
> > 
> > This entire section is behind a dom-if, so 'no change' makes sense.
> > 
> > Are there other tests that I should run?
> 
> Tested on my beefy dev machine and without change stamping is ~17ms. With
change
> stamping is ~32ms. This is also consistent on my macbook.

Changed to action-link and removed ripple after our conversation. Stamp time is
now ~17 seconds.

[commit-bot: I haz the power, 2017-02-01 00:42:08]

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

[commit-bot: I haz the power, 2017-02-01 00:42:09]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dan Beam, 2017-02-01 00:43:23]

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

[Dan Beam, 2017-02-01 00:43:24]

wait, don't we want a ripple to match history?

[Dan Beam, 2017-02-01 00:47:50]

https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:112: <a
is="action-link" data-path="/internet" on-tap="openPage_">
if you do something like:

<button is="paper-icon-button-light" data-path="/internet" on-tap="openPage_">
  <iron-icon icon="settings:network-wifi"></iron-icon>
  <span>$i18n{internetPageTitle}</span>
</button>

(which is dynamically adding ripples and pretty light) what's the effect on load
time?

[hcarmona, 2017-02-01 18:48:38]

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

[commit-bot: I haz the power, 2017-02-01 18:49:10]

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

[hcarmona, 2017-02-01 18:50:05]

https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_menu/settings_menu.html:112: <a
is="action-link" data-path="/internet" on-tap="openPage_">
On 2017/02/01 00:47:50, Dan Beam wrote:
> if you do something like:
> 
> <button is="paper-icon-button-light" data-path="/internet" on-tap="openPage_">
>   <iron-icon icon="settings:network-wifi"></iron-icon>
>   <span>$i18n{internetPageTitle}</span>
> </button>
> 
> (which is dynamically adding ripples and pretty light) what's the effect on
load
> time?

IIUC paper-icon-button-light is just for an icon, no text.

When Dave and I spoke he mentioned that history's side bar was always visible
but since settings side bar slides away we only needed a ripple on the advanced
button. However, now I see that history's side bar also slides away if your
window is small enough and has ripples.

Adding ripples to all these action-links increases stamp time to ~20ms

[Dan Beam, 2017-02-01 19:13:49]

On 2017/02/01 18:50:05, hcarmona wrote:
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_menu/settings_menu.html
(right):
> 
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_menu/settings_menu.html:112: <a
> is="action-link" data-path="/internet" on-tap="openPage_">
> On 2017/02/01 00:47:50, Dan Beam wrote:
> > if you do something like:
> > 
> > <button is="paper-icon-button-light" data-path="/internet"
on-tap="openPage_">
> >   <iron-icon icon="settings:network-wifi"></iron-icon>
> >   <span>$i18n{internetPageTitle}</span>
> > </button>
> > 
> > (which is dynamically adding ripples and pretty light) what's the effect on
> load
> > time?
> 
> IIUC paper-icon-button-light is just for an icon, no text.

<button is="paper-icon-button-light"> has a <content> slot with no select="", so
it's possible to be used with anything, but as we just found out together it's
geared toward icons (.circle class on the ripple).

we should potentially just use PaperRippleBehavior directly in the side bar.

all of these links also take users to URLs, so they should be <a href> (so users
can ctrl+click and copy the links, etc.).

> 
> When Dave and I spoke he mentioned that history's side bar was always visible
> but since settings side bar slides away we only needed a ripple on the
advanced
> button. However, now I see that history's side bar also slides away if your
> window is small enough and has ripples.

yes, history's side bar is adaptive and has ripples on all items.

> 
> Adding ripples to all these action-links increases stamp time to ~20ms

btw, multiply by 10 for mortal desktops (i.e. +200ms, which is easily visible by
the human eye)

[commit-bot: I haz the power, 2017-02-01 19:54:16]

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

[commit-bot: I haz the power, 2017-02-01 19:54:17]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-02-01 22:15:15]

On 2017/02/01 18:50:05, hcarmona wrote:
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_menu/settings_menu.html
(right):
> 
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_menu/settings_menu.html:112: <a
> is="action-link" data-path="/internet" on-tap="openPage_">
> On 2017/02/01 00:47:50, Dan Beam wrote:
> > if you do something like:
> > 
> > <button is="paper-icon-button-light" data-path="/internet"
on-tap="openPage_">
> >   <iron-icon icon="settings:network-wifi"></iron-icon>
> >   <span>$i18n{internetPageTitle}</span>
> > </button>
> > 
> > (which is dynamically adding ripples and pretty light) what's the effect on
> load
> > time?
> 
> IIUC paper-icon-button-light is just for an icon, no text.
> 
> When Dave and I spoke he mentioned that history's side bar was always visible
> but since settings side bar slides away we only needed a ripple on the
advanced
> button. However, now I see that history's side bar also slides away if your
> window is small enough and has ripples.
> 
> Adding ripples to all these action-links increases stamp time to ~20ms

We intentionally only had ripples on 'advanced', iirc it was to improve
performance.
FYI, here is a CL that shows two different ways of adding ripples (using a
ripple element vs using a ripple_() function to create ripples on the fly):
https://codereview.chromium.org/2341593002/

[hcarmona, 2017-02-02 19:39:16]

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

[commit-bot: I haz the power, 2017-02-02 19:39:39]

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

[hcarmona, 2017-02-02 19:55:14]

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

[hcarmona, 2017-02-02 19:55:27]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-02-02 19:55:38]

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

[hcarmona, 2017-02-02 20:02:03]

On 2017/02/01 22:15:15, dschuyler wrote:
> On 2017/02/01 18:50:05, hcarmona wrote:
> >
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> > File chrome/browser/resources/settings/settings_menu/settings_menu.html
> (right):
> > 
> >
>
https://codereview.chromium.org/2651293003/diff/60001/chrome/browser/resource...
> > chrome/browser/resources/settings/settings_menu/settings_menu.html:112: <a
> > is="action-link" data-path="/internet" on-tap="openPage_">
> > On 2017/02/01 00:47:50, Dan Beam wrote:
> > > if you do something like:
> > > 
> > > <button is="paper-icon-button-light" data-path="/internet"
> on-tap="openPage_">
> > >   <iron-icon icon="settings:network-wifi"></iron-icon>
> > >   <span>$i18n{internetPageTitle}</span>
> > > </button>
> > > 
> > > (which is dynamically adding ripples and pretty light) what's the effect
on
> > load
> > > time?
> > 
> > IIUC paper-icon-button-light is just for an icon, no text.
> > 
> > When Dave and I spoke he mentioned that history's side bar was always
visible
> > but since settings side bar slides away we only needed a ripple on the
> advanced
> > button. However, now I see that history's side bar also slides away if your
> > window is small enough and has ripples.
> > 
> > Adding ripples to all these action-links increases stamp time to ~20ms
> 
> We intentionally only had ripples on 'advanced', iirc it was to improve
> performance.
> FYI, here is a CL that shows two different ways of adding ripples (using a
> ripple element vs using a ripple_() function to create ripples on the fly):
> https://codereview.chromium.org/2341593002/

Thanks Dave for link on ways to ripple. For now, I've removed the ripple from
everywhere except the advanced button. I emailed bettes@ to see if he wants
ripple everywhere. That should be a separate CL if he wants it.

For now, the side nav now uses href on the links so that we can middle click,
copy URL, etc

[commit-bot: I haz the power, 2017-02-02 20:38:49]

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

[commit-bot: I haz the power, 2017-02-02 20:38:50]

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

[dschuyler, 2017-02-02 20:46:53]

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:90:
line-height: 40px;
Will this scale well with super large fonts?

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:107:
<span>$i18n{internetPageTitle}</span>
Can these spans be removed (here and below in this file)?

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:78:
event.currentTarget.baseURI.length - 1);
My guess is that line 77 is cutting off any query parameters there may be - is
that right? Hmm, how about a comment saying what this is doing?

[Dan Beam, 2017-02-06 17:28:12]

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:2: <link
rel="import" href="chrome://resources/html/action_link.html">
you haven't imported action_link_css.html

but we probably just switch to native <a href>

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:43:
a[is='action-link'].no-outline {
you shouldn't need this now that scottchen@ fixed this

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:105: <a
is="action-link" href="/internet" on-tap="openPage_">
wait why do we need both href AND is="action-link"?  <a href> has the a11y
benefits of being in the tab order already

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:107:
<span>$i18n{internetPageTitle}</span>
On 2017/02/02 20:46:53, dschuyler wrote:
> Can these spans be removed (here and below in this file)?

the span is targeted by css above

hcarmona: why do we need to make the action links display: flex;?

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:77: var path =
event.currentTarget.href.substr(
btw, there's a URL class

new URL(event.currentTarget.href).pathname

but in this case you can just call getAttribute('href')

https://jsfiddle.net/00reaL2o/

[Dan Beam, 2017-02-09 17:29:52]

what is the hold up on this CL?

[hcarmona, 2017-02-10 19:18:50]

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

[commit-bot: I haz the power, 2017-02-10 19:19:53]

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

[hcarmona, 2017-02-10 19:27:34]

Fairly big change form last patch, but overall less code :-)

This doesn't include any ripple work, I'll add that as a separate CL to match
history. This DOES include fixing the side nav so it's a11y-able.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:2: <link
rel="import" href="chrome://resources/html/action_link.html">
On 2017/02/06 17:28:12, Dan Beam wrote:
> you haven't imported action_link_css.html
> 
> but we probably just switch to native <a href>

Done.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:43:
a[is='action-link'].no-outline {
On 2017/02/06 17:28:12, Dan Beam wrote:
> you shouldn't need this now that scottchen@ fixed this

Done.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:90:
line-height: 40px;
On 2017/02/02 20:46:53, dschuyler wrote:
> Will this scale well with super large fonts? 

Updated, no more line-height.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:105: <a
is="action-link" href="/internet" on-tap="openPage_">
On 2017/02/06 17:28:12, Dan Beam wrote:
> wait why do we need both href AND is="action-link"?  <a href> has the a11y
> benefits of being in the tab order already

We don't. Updated.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:107:
<span>$i18n{internetPageTitle}</span>
On 2017/02/06 17:28:12, Dan Beam (slow) wrote:
> On 2017/02/02 20:46:53, dschuyler wrote:
> > Can these spans be removed (here and below in this file)?
> 
> the span is targeted by css above
> 
> hcarmona: why do we need to make the action links display: flex;?

Spans are not necessary, they were left overs from a previous version. Removed.

Flex on #advancedButton > span so that the expanded/collapsed icon is to the
right.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:77: var path =
event.currentTarget.href.substr(
On 2017/02/06 17:28:12, Dan Beam wrote:
> btw, there's a URL class
> 
> new URL(event.currentTarget.href).pathname
> 
> but in this case you can just call getAttribute('href')
> 
> https://jsfiddle.net/00reaL2o/

Sweet! Using URL.

https://codereview.chromium.org/2651293003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:78:
event.currentTarget.baseURI.length - 1);
On 2017/02/02 20:46:53, dschuyler wrote:
> My guess is that line 77 is cutting off any query parameters there may be - is
> that right? Hmm, how about a comment saying what this is doing?

Changed to use URL

[commit-bot: I haz the power, 2017-02-10 20:24:19]

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

[commit-bot: I haz the power, 2017-02-10 20:24:20]

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

[Dan Beam, 2017-02-10 23:45:39]

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:85:
on-iron-activate="onSelectorActivate_">
why do we want an iron-selector instead of just

listeners: {
  tap: 'onTap_',
},

onTap_: function(e) {
  var href = e.target.getAttribute('href');
  if (!href)
    return;

  e.preventDefault();
  var route = assert(settings.getRouteForPath(href));
  ...
},

and remove all the on-click="onLinkTap_" stuff?

[hcarmona, 2017-02-13 18:53:11]

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

[commit-bot: I haz the power, 2017-02-13 18:53:35]

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

[hcarmona, 2017-02-13 18:54:08]

Fixed failing tests

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:85:
on-iron-activate="onSelectorActivate_">
On 2017/02/10 23:45:39, Dan Beam wrote:
> why do we want an iron-selector instead of just
> 
> listeners: {
>   tap: 'onTap_',
> },
> 
> onTap_: function(e) {
>   var href = e.target.getAttribute('href');
>   if (!href)
>     return;
> 
>   e.preventDefault();
>   var route = assert(settings.getRouteForPath(href));
>   ...
> },
> 
> and remove all the on-click="onLinkTap_" stuff?

iron-selector helps with selection. Section links turn blue when they're active.

listeners: { tap... } won't get us what we need because the tap listener gets
added to the settings-menu instead of each link. So, we don't get the right
target.

[commit-bot: I haz the power, 2017-02-13 20:03:52]

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

[commit-bot: I haz the power, 2017-02-13 20:03:53]

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

[Dan Beam, 2017-02-13 20:04:07]

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:87: <a
href="/internet" on-click="onLinkTap_">
why did you change to on-click?

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:87: <a
href="/internet" on-click="onLinkTap_">
why can't we just put 1 event listener on #topMenu and filter the target?

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:32:
Array.prototype.forEach.call(this.root.querySelectorAll('a'),
doesn't NodeList already have a forEach?

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:34: var path =
new URL(link.href).pathname;
link.getAttribute('href') should not need to be made into a URL() because it
wont be resolved

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:46: onLinkTap_:
function(event) {
let's do this at a higher level, there's no reason for all of these link to have
individual handlers

[Dan Beam, 2017-02-13 20:05:59]

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:85:
on-iron-activate="onSelectorActivate_">
On 2017/02/13 18:54:08, hcarmona wrote:
> On 2017/02/10 23:45:39, Dan Beam wrote:
> > why do we want an iron-selector instead of just
> > 
> > listeners: {
> >   tap: 'onTap_',
> > },
> > 
> > onTap_: function(e) {
> >   var href = e.target.getAttribute('href');
> >   if (!href)
> >     return;
> > 
> >   e.preventDefault();
> >   var route = assert(settings.getRouteForPath(href));
> >   ...
> > },
> > 
> > and remove all the on-click="onLinkTap_" stuff?
> 
> iron-selector helps with selection. Section links turn blue when they're
active.
> 
> listeners: { tap... } won't get us what we need because the tap listener gets
> added to the settings-menu instead of each link. So, we don't get the right
> target.

no, you'll get the same target with a different currentTarget

i think either e.target or e.path will have clear signs which link an event
originated from

[hcarmona, 2017-02-13 23:36:06]

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

[commit-bot: I haz the power, 2017-02-13 23:37:00]

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

[hcarmona, 2017-02-13 23:42:34]

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:87: <a
href="/internet" on-click="onLinkTap_">
On 2017/02/13 20:04:06, Dan Beam wrote:
> why did you change to on-click?

on-click didn't focus the link, while on-tap did.

New handling doesn't have this issue.

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:87: <a
href="/internet" on-click="onLinkTap_">
On 2017/02/13 20:04:06, Dan Beam wrote:
> why can't we just put 1 event listener on #topMenu and filter the target?

This worked. Done.

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:32:
Array.prototype.forEach.call(this.root.querySelectorAll('a'),
On 2017/02/13 20:04:07, Dan Beam wrote:
> doesn't NodeList already have a forEach?

Closure didn't like forEach on the NodeList or a typecast from NodeList to Array
:-/

Am I missing an include?

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:34: var path =
new URL(link.href).pathname;
On 2017/02/13 20:04:07, Dan Beam wrote:
> link.getAttribute('href') should not need to be made into a URL() because it
> wont be resolved

Sweet! Updated.

https://codereview.chromium.org/2651293003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:46: onLinkTap_:
function(event) {
On 2017/02/13 20:04:07, Dan Beam wrote:
> let's do this at a higher level, there's no reason for all of these link to
have
> individual handlers

Done.

[commit-bot: I haz the power, 2017-02-14 01:25:39]

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

[commit-bot: I haz the power, 2017-02-14 01:25:40]

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-02-14 05:44:31]

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:89:
</iron-icon>$i18n{internetPageTitle}
why are you using this indentation?  how is it better than previously?

is there a functional difference in that there's whitespace?  if so, can we just
use flex (now that I understand the reason?)

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:37:
Array.prototype.forEach.call(this.root.querySelectorAll('a'),
add this https://gist.github.com/danbeam/e17176126d827a8f52f6457c92207cbe and it
should fix compilation

if it doesn't, change to NodeList.prototype.forEach

you might also have to remove some duplicates (if pending_compiler_externs.js is
actually included other places)

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:40:
this.$.topMenu.selected = this.$.subMenu.selected = link.href;
nit: why are you using .href here instead of getAttribute('href')?  because it
matches how <iron-selector> works?

either way, maybe make a

setSelectedUrl_: function(url) {
  this.$.topMenu.selected = this.$.subMenu.selected = url;
},

and call from both places?

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:51: if
(event.target.href)
nit: this is probably fine but event.target.hasAttribute('href') doesn't
actually run the normalization steps

event.target.{webkitM,m}atches('a[href]') is also fairly explanatory (but does
slightly more)

[hcarmona, 2017-02-15 02:10:05]

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

[hcarmona, 2017-02-15 02:10:49]

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:89:
</iron-icon>$i18n{internetPageTitle}
On 2017/02/14 05:44:30, Dan Beam wrote:
> why are you using this indentation?  how is it better than previously?
> 
> is there a functional difference in that there's whitespace?  if so, can we
just
> use flex (now that I understand the reason?)

A space was being added between the icon and the text. display: flex; Fixes it.
No more weird indent.

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:37:
Array.prototype.forEach.call(this.root.querySelectorAll('a'),
On 2017/02/14 05:44:30, Dan Beam wrote:
> add this https://gist.github.com/danbeam/e17176126d827a8f52f6457c92207cbe and
it
> should fix compilation
> 
> if it doesn't, change to NodeList.prototype.forEach
> 
> you might also have to remove some duplicates (if pending_compiler_externs.js
is
> actually included other places)

forEach -> for avoids closure weirdness.

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:40:
this.$.topMenu.selected = this.$.subMenu.selected = link.href;
On 2017/02/14 05:44:30, Dan Beam wrote:
> nit: why are you using .href here instead of getAttribute('href')?  because it
> matches how <iron-selector> works?
> 
> either way, maybe make a
> 
> setSelectedUrl_: function(url) {
>   this.$.topMenu.selected = this.$.subMenu.selected = url;
> },
> 
> and call from both places?

Done. Added a comment to explain why href is needed instead of
getAttribute('href')

https://codereview.chromium.org/2651293003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:51: if
(event.target.href)
On 2017/02/14 05:44:31, Dan Beam wrote:
> nit: this is probably fine but event.target.hasAttribute('href') doesn't
> actually run the normalization steps
> 
> event.target.{webkitM,m}atches('a[href]') is also fairly explanatory (but does
> slightly more)

Done.

[commit-bot: I haz the power, 2017-02-15 02:10: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-02-15 03:15:56]

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

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

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

[dschuyler, 2017-02-15 21:45:24]

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:77: route, /*
dynamicParams */ null, /* removeSearch */ true);
It looks like a space got mistakenly removed.

[Dan Beam, 2017-02-15 23:14:32]

lgtm w/nits

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:55: display:
flex;
duplicate display: flex;

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:97:
hidden="[[!pageVisibility.people]]">
unwrap

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:34: var
currentRoute = settings.getCurrentRoute();
nit: var currentPath = settings.getCurrentRoute().path;

[hcarmona, 2017-02-15 23:22:10]

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

[commit-bot: I haz the power, 2017-02-15 23:23:18]

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

[dschuyler, 2017-02-15 23:29:51]

LGTM with some suggestions.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:30:
font-size: 100%;
Does this have any effect? Maybe remove font-size: 100%;

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:185: </a>
This could unwrap, if you like.

[hcarmona, 2017-02-15 23:33:38]

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

[hcarmona, 2017-02-15 23:33:53]

Patchset #9 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-02-15 23:34:18]

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

[hcarmona, 2017-02-15 23:35:31]

Thanks for feedback! Fixed nits, committing.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.html (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:30:
font-size: 100%;
On 2017/02/15 23:29:51, dschuyler wrote:
> Does this have any effect? Maybe remove font-size: 100%;

Done.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:55: display:
flex;
On 2017/02/15 23:14:32, Dan Beam wrote:
> duplicate display: flex;

Done.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:97:
hidden="[[!pageVisibility.people]]">
On 2017/02/15 23:14:32, Dan Beam wrote:
> unwrap

Done.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.html:185: </a>
On 2017/02/15 23:29:51, dschuyler wrote:
> This could unwrap, if you like.

Done.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:34: var
currentRoute = settings.getCurrentRoute();
On 2017/02/15 23:14:32, Dan Beam wrote:
> nit: var currentPath = settings.getCurrentRoute().path;

Done.

https://codereview.chromium.org/2651293003/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:77: route, /*
dynamicParams */ null, /* removeSearch */ true);
On 2017/02/15 21:45:23, dschuyler wrote:
> It looks like a space got mistakenly removed.

Good eye. Fixed.

[hcarmona, 2017-02-15 23:35:46]

The CQ bit was unchecked by hcarmona@chromium.org

[hcarmona, 2017-02-15 23:35:47]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2017-02-15 23:35:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2651293003/#ps240001
(title: "nits + rebase")

[commit-bot: I haz the power, 2017-02-15 23:37: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-02-16 00:19:46]

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

[commit-bot: I haz the power, 2017-02-16 00:19:48]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Dan Beam, 2017-02-16 04:03:54]

The CQ bit was checked by dbeam@chromium.org

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

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-02-16 06:06:30]

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

[commit-bot: I haz the power, 2017-02-16 06:06:32]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[hcarmona, 2017-02-16 17:34:04]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2017-02-16 17:34:50]

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-02-16 17:42:18]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487266444212190,
"parent_rev": "953322582e01e05f831456fecea0d48af331f16f", "commit_rev":
"f26ccb9043c276fb76b875f90361ef5b58acc510"}

[commit-bot: I haz the power, 2017-02-16 17:42:56]

Description was changed from

==========
Make MD Settings side bar keyboard accessible.

Screenshots in bug.

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

to

==========
Make MD Settings side bar keyboard accessible.

Screenshots in bug.

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

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

[commit-bot: I haz the power, 2017-02-16 17:42:58]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/f26ccb9043c276fb76b875f90361...