MD Settings: change most checkboxes to toggles (leave dialogs alone)

MD Settings: change most checkboxes to toggles (leave dialogs alone)

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2617533003
Cr-Commit-Position: refs/heads/master@{#442947}
Committed: https://chromium.googlesource.com/chromium/src/+/0811dbe5577e389b35e8bc42ff9d19da756de89c

[Dan Beam, 2017-01-04 22:22:47]

Description was changed from

==========
MD Settings: change most <settings-checkbox> to <settings-toggle-button>

Ignore <settings-checkbox> in dialogs.

R=dschuyler@chromium.org
BUG=673953
==========

to

==========
MD Settings: change most <settings-checkbox> to <settings-toggle-button>

Ignore <settings-checkbox> in dialogs.

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

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

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

[commit-bot: I haz the power, 2017-01-05 02:11:29]

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-05 03:12:16]

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

[commit-bot: I haz the power, 2017-01-05 03:12:17]

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

[dschuyler, 2017-01-05 20:25:06]

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:44:
hidden="[[!prefs.settings.accessibility.value]]"></button>
How well does this work, having a button in a toggle? (and how does it look)?

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:28:
no-set-pref>
Can this unwrap?

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_button.html:31:
-webkit-margin-start: var(--checkbox-spacing);
optional since this is just moving exiting code, but the name --checkbox-spacing
seems odd to apply to a policy pref indicator. Maybe there should be a new var,
or maybe that var should be renamed.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_button.html:45:
pref.enforcement)]]">
This seems different from the checkbox to toggle comment in the CL description?
I'm wondering if the CL description could be amended to include a comment about
these changes.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.html
(right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.html:19: }
This change looks okay, but maybe comment on it in the CL description.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/stylus.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/stylus.html:21:
disabled="[[!prefs.settings.enable_stylus_tools.value]]">
It's okay if this disabled line needs to move, but I'm not seeing why the change
was made and wanted to ask about it.

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:25:
right-justified>
Will this work well in RTL?

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:66:
</settings-toggle-button>
Afaik, each toggle would be within a .settings-box div. I just checked with Alan
whom agreed that for now they should have a grey line to indicate the rows (aka,
putting them in a .settings-box). The screen shot I sent the other day shows how
they look with the grey lines (that's the screen shot I asked Alan about).

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:79: </div>
The div above could probably be converted changed to look more like the other
settings-toggle-button's on this page (with the label="" and such).

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
disabled="[[metricsReporting_.managed]]">
Could the label from line 93 be made a label="" here and then then templates
could be put in a div and slotted into the toggle button with the .more-actions
selector (similar to another change in this CL)?

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:24: <if expr="not
is_macosx">
After the div on line 30 is moved (mentioned in the next comment), this <if>
should go before line 23.
Then the "block" can be removed from <div class="settings-box block first"> on
line 23.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:30: <div
class="layout horizontal center">
This could be moved out of the div on line 23.
Then this div would change from 
<div class="layout horizontal center">
to <div class="settings-box continuation">
That will allow this div to get the proper height and layout from settings-box.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:51: <div
class="layout horizontal center inherit-min-height">
It looks like the "block" should be removed on line 43 and a </div><div
class="settings-box continuation"> added around line 50. That would allow lines
44 and 51 to simply be class="start" and remove the need for
.inherit-min-height.

[Dan Beam, 2017-01-06 02:45:30]

OK, this CL is now based on:

https://codereview.chromium.org/2616043002
https://codereview.chromium.org/2615093003
https://codereview.chromium.org/2611243002
https://codereview.chromium.org/2615613007
https://codereview.chromium.org/2614063004
https://codereview.chromium.org/2611183003

it should be a bit simpler, BUT I understand it'll be hard to keep these all
straight

let me know what you'd like to do.  this can't land until at least 3(?) of those
do.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:44:
hidden="[[!prefs.settings.accessibility.value]]"></button>
On 2017/01/05 20:25:05, dschuyler wrote:
> How well does this work, having a button in a toggle? (and how does it look)?

I added a specific <content selector=".more-actions"> to position this in
<settings-toggle-button>.  note that I'm also adding that class here.

http://i.imgur.com/XaBPZKM.png (the RELAUNCH button)

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/android_apps_page/android_apps_page.html:28:
no-set-pref>
On 2017/01/05 20:25:05, dschuyler wrote:
> Can this unwrap?
> 

yes, it could ;)

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_button.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_button.html:31:
-webkit-margin-start: var(--checkbox-spacing);
On 2017/01/05 20:25:05, dschuyler wrote:
> optional since this is just moving exiting code, but the name
--checkbox-spacing
> seems odd to apply to a policy pref indicator. Maybe there should be a new
var,
> or maybe that var should be renamed.

Done.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_button.html:45:
pref.enforcement)]]">
On 2017/01/05 20:25:05, dschuyler wrote:
> This seems different from the checkbox to toggle comment in the CL
description?
> I'm wondering if the CL description could be amended to include a comment
about
> these changes.

split to a separate CL.

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/controlled_radio_button.html
(right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/controlled_radio_button.html:19: }
On 2017/01/05 20:25:05, dschuyler wrote:
> This change looks okay, but maybe comment on it in the CL description.

split off

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/stylus.html (right):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/stylus.html:21:
disabled="[[!prefs.settings.enable_stylus_tools.value]]">
On 2017/01/05 20:25:05, dschuyler wrote:
> It's okay if this disabled line needs to move, but I'm not seeing why the
change
> was made and wanted to ask about it.

so all <settings-toggle-button> are more consistently declared

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/downloads_page/downloads_page.html:25:
right-justified>
On 2017/01/05 20:25:05, dschuyler wrote:
> Will this work well in RTL?

yes, renamed in https://codereview.chromium.org/2615093003/

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (left):

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:48: }
this is being shrunk here: https://codereview.chromium.org/2614063004

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:66:
</settings-toggle-button>
On 2017/01/05 20:25:05, dschuyler wrote:
> Afaik, each toggle would be within a .settings-box div. I just checked with
Alan
> whom agreed that for now they should have a grey line to indicate the rows
(aka,
> putting them in a .settings-box). The screen shot I sent the other day shows
how
> they look with the grey lines (that's the screen shot I asked Alan about).

let's do that separately because i don't agree

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:79: </div>
On 2017/01/05 20:25:05, dschuyler wrote:
> The div above could probably be converted changed to look more like the other
> settings-toggle-button's on this page (with the label="" and such).

<paper-toggle-button> doesn't have label= property and the <content> slot is on
the wrong side (<paper-toggle-button> shows toggle on the left, label on the
right)

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:111:
disabled="[[metricsReporting_.managed]]">
On 2017/01/05 20:25:05, dschuyler wrote:
> Could the label from line 93 be made a label="" here and then then templates
> could be put in a div and slotted into the toggle button with the
.more-actions
> selector (similar to another change in this CL)?

paper-toggle-button != settings-toggle-button

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:24: <if expr="not
is_macosx">
On 2017/01/05 20:25:05, dschuyler wrote:
> After the div on line 30 is moved (mentioned in the next comment), this <if>
> should go before line 23.
> Then the "block" can be removed from <div class="settings-box block first"> on
> line 23.

see: https://codereview.chromium.org/2615613007

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:30: <div
class="layout horizontal center">
On 2017/01/05 20:25:05, dschuyler wrote:
> This could be moved out of the div on line 23.
> Then this div would change from 
> <div class="layout horizontal center">
> to <div class="settings-box continuation">
> That will allow this div to get the proper height and layout from
settings-box.

see: https://codereview.chromium.org/2615613007

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/system_page/system_page.html:51: <div
class="layout horizontal center inherit-min-height">
On 2017/01/05 20:25:05, dschuyler wrote:
> It looks like the "block" should be removed on line 43 and a </div><div
> class="settings-box continuation"> added around line 50. That would allow
lines
> 44 and 51 to simply be class="start" and remove the need for
> .inherit-min-height.

see: https://codereview.chromium.org/2615613007

[dschuyler, 2017-01-10 00:07:54]

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:66:
</settings-toggle-button>
On 2017/01/06 02:45:30, Dan Beam wrote:
> On 2017/01/05 20:25:05, dschuyler wrote:
> > Afaik, each toggle would be within a .settings-box div. I just checked with
> Alan
> > whom agreed that for now they should have a grey line to indicate the rows
> (aka,
> > putting them in a .settings-box). The screen shot I sent the other day shows
> how
> > they look with the grey lines (that's the screen shot I asked Alan about).
> 
> let's do that separately because i don't agree

There was a separate email thread about this, was Alan able to sway the choice
on row lines?

[Dan Beam, 2017-01-11 01:55:48]

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

https://codereview.chromium.org/2617533003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/privacy_page/privacy_page.html:66:
</settings-toggle-button>
On 2017/01/10 00:07:53, dschuyler wrote:
> On 2017/01/06 02:45:30, Dan Beam wrote:
> > On 2017/01/05 20:25:05, dschuyler wrote:
> > > Afaik, each toggle would be within a .settings-box div. I just checked
with
> > Alan
> > > whom agreed that for now they should have a grey line to indicate the rows
> > (aka,
> > > putting them in a .settings-box). The screen shot I sent the other day
shows
> > how
> > > they look with the grey lines (that's the screen shot I asked Alan about).
> > 
> > let's do that separately because i don't agree
> 
> There was a separate email thread about this, was Alan able to sway the choice
> on row lines?

yes, but we're doing this after

[Dan Beam, 2017-01-11 01:56:51]

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

[commit-bot: I haz the power, 2017-01-11 01:57:17]

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

[Dan Beam, 2017-01-11 01:57:18]

Description was changed from

==========
MD Settings: change most <settings-checkbox> to <settings-toggle-button>

Ignore <settings-checkbox> in dialogs.

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: change most checkboxes to toggles (leave dialogs alone)

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-01-11 02:02:46]

this CL (and review) is getting hairy

I've already started a followup to add line separators.  it's not as big as
this, but it's also sizable.

there are a few things here that have diminishing returns, and I could do either
before or after:

1) rename things like $i18n{blahCheckbox} -> $i18n{blahControl} or something
more generic

2) rename id="blahCheckbox" to id="blahControl"

3) probably other stuff like little UI tweaks or comment fixes etc.

All of these things are worth doing separately from the bulk, IMO.

If we can't come to consensus on this review (or my plan above ^), I can split
this review up into per page or something and we'll both get to deal with the
bugs filed by QA/PM/UX on the inconsistencies ;).

Ultimately, mega reviews aren't efficient.  Except maybe when they take like 10s
to create, like with clang-format.

[dschuyler, 2017-01-11 02:34:47]

I agree that this CL is more like a step toward something and the follow-up CLs
will make things nice overall. I don't expect those CLs will be problematic. So
LGTM as a step toward something nicer.

https://codereview.chromium.org/2617533003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2617533003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:39: a good idea.
-->
Just a suggestion: would something like "check whether
paper-button[is=action-link] is needed here", convey the intent?

[Dan Beam, 2017-01-11 03:13:56]

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

[commit-bot: I haz the power, 2017-01-11 03:14:34]

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-11 04:01:11]

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

[commit-bot: I haz the power, 2017-01-11 04:01:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-01-11 04:59:36]

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

[commit-bot: I haz the power, 2017-01-11 04:59:53]

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

[Dan Beam, 2017-01-11 05:00:51]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2017-01-11 05:00:59]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2017-01-11 05:01:17]

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-11 06:21:14]

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

[commit-bot: I haz the power, 2017-01-11 06:21:16]

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-01-11 07:27:38]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-01-11 07:27:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2617533003/#ps140001
(title: "fix tests")

[commit-bot: I haz the power, 2017-01-11 07:28: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-01-11 08:57:34]

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

[commit-bot: I haz the power, 2017-01-11 08:57:35]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Dan Beam, 2017-01-11 17:06:24]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2017-01-11 17:06:51]

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

[commit-bot: I haz the power, 2017-01-11 17:45:37]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484154384609330,
"parent_rev": "a8f55a43d7da20364992515885fc25c39ca97e8d", "commit_rev":
"0811dbe5577e389b35e8bc42ff9d19da756de89c"}

[commit-bot: I haz the power, 2017-01-11 17:46:44]

Description was changed from

==========
MD Settings: change most checkboxes to toggles (leave dialogs alone)

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: change most checkboxes to toggles (leave dialogs alone)

R=dschuyler@chromium.org
BUG=673953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2617533003
Cr-Commit-Position: refs/heads/master@{#442947}
Committed:
https://chromium.googlesource.com/chromium/src/+/0811dbe5577e389b35e8bc42ff9d...
==========

[commit-bot: I haz the power, 2017-01-11 17:46:46]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0811dbe5577e389b35e8bc42ff9d...