MD Settings: Add Learn More button to the Passwords subpage

MD Settings: Add Learn More button to the Passwords subpage

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

Review-Url: https://codereview.chromium.org/2691043005
Cr-Commit-Position: refs/heads/master@{#450866}
Committed: https://chromium.googlesource.com/chromium/src/+/74fa573ec3eab3ec73b1ca5b659393714a36a0a9

[tommycli, 2017-02-13 22:25:28]

Description was changed from

==========
MD Settings: Add Learn More button to the Passwords subpage

BUG=631195
==========

to

==========
MD Settings: Add Learn More button to the Passwords subpage

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

[tommycli, 2017-02-13 22:25:34]

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

[commit-bot: I haz the power, 2017-02-13 22:26:16]

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

[tommycli, 2017-02-13 22:26:31]

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

[tommycli, 2017-02-13 22:26:32]

hcarmona: PTAL passwords page change.

Screenshot here: http://imgur.com/a/Xpkjl

This CL adds the question mark icon.

[hcarmona, 2017-02-13 22:44:15]

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:73:
<button class="icon-help subpage-title-extra"
Should the button be a link since it's navigating to a URL?

<a href="$i18n{passwordManagerLearnMoreURL}"><iron-icon ...></a>

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:74:
is="paper-icon-button-light" on-tap="onPasswordsHelpTap_">
Please add aria-label for a11y

[tommycli, 2017-02-13 22:53:57]

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

[tommycli, 2017-02-13 22:54:05]

thanks!

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:73:
<button class="icon-help subpage-title-extra"
On 2017/02/13 22:44:14, hcarmona wrote:
> Should the button be a link since it's navigating to a URL?
> 
> <a href="$i18n{passwordManagerLearnMoreURL}"><iron-icon ...></a>

I think it should stay a button, since the other instance of help icon is also a
button:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac...

Additionally the back button on the same subpage bar is also a paper-icon. (this
is a paper-icon-button-light)

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:74:
is="paper-icon-button-light" on-tap="onPasswordsHelpTap_">
On 2017/02/13 22:44:14, hcarmona wrote:
> Please add aria-label for a11y

Done.

[commit-bot: I haz the power, 2017-02-13 22:54: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-13 23:06:12]

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:73:
<button class="icon-help subpage-title-extra"
On 2017/02/13 22:54:05, tommycli wrote:
> On 2017/02/13 22:44:14, hcarmona wrote:
> > Should the button be a link since it's navigating to a URL?
> > 
> > <a href="$i18n{passwordManagerLearnMoreURL}"><iron-icon ...></a>
> 
> I think it should stay a button, since the other instance of help icon is also
a
> button:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac...
> 
> Additionally the back button on the same subpage bar is also a paper-icon.
(this
> is a paper-icon-button-light)

Would it make sense for both help buttons to be links instead? This allows
copying of the link and middle click to work. It also simplifies the code by not
needing the |window.open(...)| in the JS.

I'm not super attached to the link idea if you feel a button would be better.

[tommycli, 2017-02-14 00:16:12]

hcarmona, thanks!

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:73:
<button class="icon-help subpage-title-extra"
On 2017/02/13 23:06:12, hcarmona wrote:
> On 2017/02/13 22:54:05, tommycli wrote:
> > On 2017/02/13 22:44:14, hcarmona wrote:
> > > Should the button be a link since it's navigating to a URL?
> > > 
> > > <a href="$i18n{passwordManagerLearnMoreURL}"><iron-icon ...></a>
> > 
> > I think it should stay a button, since the other instance of help icon is
also
> a
> > button:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac...
> > 
> > Additionally the back button on the same subpage bar is also a paper-icon.
> (this
> > is a paper-icon-button-light)
> 
> Would it make sense for both help buttons to be links instead? This allows
> copying of the link and middle click to work. It also simplifies the code by
not
> needing the |window.open(...)| in the JS.
> 
> I'm not super attached to the link idea if you feel a button would be better.

Done. Thanks! After an in-person discussion I understand you now and have
implemented your suggestions.

[Dan Beam, 2017-02-14 00:26:58]

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

[Dan Beam, 2017-02-14 00:26:59]

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
(right):

https://codereview.chromium.org/2691043005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:73:
<button class="icon-help subpage-title-extra"
On 2017/02/14 00:16:12, tommycli wrote:
> On 2017/02/13 23:06:12, hcarmona wrote:
> > On 2017/02/13 22:54:05, tommycli wrote:
> > > On 2017/02/13 22:44:14, hcarmona wrote:
> > > > Should the button be a link since it's navigating to a URL?
> > > > 
> > > > <a href="$i18n{passwordManagerLearnMoreURL}"><iron-icon ...></a>
> > > 
> > > I think it should stay a button, since the other instance of help icon is
> also
> > a
> > > button:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac...
> > > 
> > > Additionally the back button on the same subpage bar is also a paper-icon.
> > (this
> > > is a paper-icon-button-light)
> > 
> > Would it make sense for both help buttons to be links instead? This allows
> > copying of the link and middle click to work. It also simplifies the code by
> not
> > needing the |window.open(...)| in the JS.
> > 
> > I'm not super attached to the link idea if you feel a button would be
better.
> 
> Done. Thanks! After an in-person discussion I understand you now and have
> implemented your suggestions.

this drops ripples, though, right?

[Dan Beam, 2017-02-14 00:27:57]

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

https://codereview.chromium.org/2691043005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:75:
<button class="icon-help" is="paper-icon-button-light"></button>
ah, this has a ripple

are these both in the tab order?

[tommycli, 2017-02-14 00:44:23]

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

https://codereview.chromium.org/2691043005/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:75:
<button class="icon-help" is="paper-icon-button-light"></button>
On 2017/02/14 00:27:57, Dan Beam wrote:
> ah, this has a ripple
> 
> are these both in the tab order?

Ah yes... these are both in the tab order... which is not good.

I can make the link no longer in the tab order by adding taborder=-1, but then I
probably need to move the area-label to the button.

hcarmona@ suggested that this should be an <a><iron-icon /><paper-ripple /><a>
but that sounded odd to me because it seemed like we were basically
re-implementing the paper-icon-button from its constituents.

Another issue I had is that if we keep the link in the tab order instead of the
button, visually there is a red outline around the whole box rather than the
icon taking on the 'focused' look.

Thoughts?

[Dan Beam, 2017-02-14 19:44:03]

On 2017/02/14 00:44:23, tommycli wrote:
>
https://codereview.chromium.org/2691043005/diff/40001/chrome/browser/resource...
> File
>
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html
> (right):
> 
>
https://codereview.chromium.org/2691043005/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.html:75:
> <button class="icon-help" is="paper-icon-button-light"></button>
> On 2017/02/14 00:27:57, Dan Beam wrote:
> > ah, this has a ripple
> > 
> > are these both in the tab order?
> 
> Ah yes... these are both in the tab order... which is not good.
> 
> I can make the link no longer in the tab order by adding taborder=-1, but then
I
> probably need to move the area-label to the button.
> 
> hcarmona@ suggested that this should be an <a><iron-icon /><paper-ripple /><a>
> but that sounded odd to me because it seemed like we were basically
> re-implementing the paper-icon-button from its constituents.
> 
> Another issue I had is that if we keep the link in the tab order instead of
the
> button, visually there is a red outline around the whole box rather than the
> icon taking on the 'focused' look.
> 
> Thoughts?

talked in person

-> we can't have both things in the tab order
-> both <a> and <button> will always be *focusable*, even if not in the
"sequential tab order"
-> we should only use 1 or the other
-> other paper-button-icon-light -> <a href> have been deprioritized

[tommycli, 2017-02-15 01:05:59]

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

[tommycli, 2017-02-15 01:06:28]

Hey guys, per our in person discussion, I have made this an <a><iron-icon /></a>

PTAL,

Thanks!

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

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

[tommycli, 2017-02-15 01:07:06]

On 2017/02/15 01:06:28, tommycli wrote:
> Hey guys, per our in person discussion, I have made this an <a><iron-icon
/></a>
> 
> PTAL,
> 
> Thanks!

Addendum: it now appears only once in the tab-order, and in the correct order. I
did have to remove the shadow dom slot and put the link-icon directly within the
subpage element though.

[commit-bot: I haz the power, 2017-02-15 01:12:46]

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

[commit-bot: I haz the power, 2017-02-15 01:12:47]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hcarmona, 2017-02-15 18:31:12]

LGTM

[tommycli, 2017-02-15 18:32:29]

On 2017/02/15 18:31:12, hcarmona wrote:
> LGTM

thank you sire!

[tommycli, 2017-02-15 18:33:19]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-02-15 18:33:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2691043005/#ps80001
(title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src
into 406-md-settings-passwo…")

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

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 19:17:42]

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

[commit-bot: I haz the power, 2017-02-15 19:17:43]

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

[tommycli, 2017-02-15 19:40:03]

dbeam: PTAL again, I need a stamp from you before I can submit. Thanks!

[tommycli, 2017-02-15 23:37:13]

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

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

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-16 00:50:38]

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

[commit-bot: I haz the power, 2017-02-16 00:50:39]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-02-16 02:41:39]

lgtm

[tommycli, 2017-02-16 02:46:55]

The CQ bit was checked by tommycli@chromium.org

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

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 03:40:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487213215752700,
"parent_rev": "2f48975e263dc630cab5dfedd6930d0fc5250579", "commit_rev":
"74fa573ec3eab3ec73b1ca5b659393714a36a0a9"}

[commit-bot: I haz the power, 2017-02-16 03:41:50]

Description was changed from

==========
MD Settings: Add Learn More button to the Passwords subpage

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

to

==========
MD Settings: Add Learn More button to the Passwords subpage

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

Review-Url: https://codereview.chromium.org/2691043005
Cr-Commit-Position: refs/heads/master@{#450866}
Committed:
https://chromium.googlesource.com/chromium/src/+/74fa573ec3eab3ec73b1ca5b6593...
==========

[commit-bot: I haz the power, 2017-02-16 03:41:51]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/74fa573ec3eab3ec73b1ca5b6593...