[MD settings] using h2 for sub-headers consistently.

[MD settings] using h2 for sub-headers consistently.

This CL switches several differently styled sub-headers to all use h2
tags. This is good because it makes them all consistent and h2 is a good
choice because screen readers may scan for h2 tags specifically to give
the user context about what is on the screen.

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

Committed: https://crrev.com/a56e76de0f7e38f81f5be072f98a44516124b8bd
Cr-Commit-Position: refs/heads/master@{#414621}

[dschuyler, 2016-08-24 18:31:27]

Description was changed from

==========
[MD settings] using h2 for sub-headers consistently.

This CL switches several differently styled sub-headers to all use h2
tags. This is good because it makes them all consistent and h2 is a good
choice because screen readers may scan for h2 tags specifically to give
the user context about what is on the screen.

BUG=638461
==========

to

==========
[MD settings] using h2 for sub-headers consistently.

This CL switches several differently styled sub-headers to all use h2
tags. This is good because it makes them all consistent and h2 is a good
choice because screen readers may scan for h2 tags specifically to give
the user context about what is on the screen.

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

[dschuyler, 2016-08-24 18:31:30]

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

[commit-bot: I haz the power, 2016-08-24 18:33:19]

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

[dschuyler, 2016-08-24 18:56:14]

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

[dschuyler, 2016-08-24 18:56:15]

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_subpage.html:44:
<h1>[[pageTitle]]</h1>
This is the top header (not a sub-header) so I 
changed it to h1 to avoid clashing with h2.
(It seems like it should be an h1 anyway).

[commit-bot: I haz the power, 2016-08-24 20:39:16]

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

[commit-bot: I haz the power, 2016-08-24 20:39:17]

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

[hcarmona, 2016-08-24 23:41:21]

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:75:
<h2>$i18n{creditCards}</h2>
Looks like this add some extra space between the lists.

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:108:
<h2>$i18n{passwordExceptionsHeading}</h2>
Extra space here also.

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:26:
<div class="settings-box block">
This is adding a dividing line between the header and this list.
Is this expected?

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:34:
<div class="settings-box block">
Same here for a dividing line between the lists.

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/settings_subpage.html
(right):

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/settings_subpage.html:44:
<h1>[[pageTitle]]</h1>
On 2016/08/24 18:56:15, dschuyler wrote:
> This is the top header (not a sub-header) so I 
> changed it to h1 to avoid clashing with h2.
> (It seems like it should be an h1 anyway).

Sounds good.

[dschuyler, 2016-08-25 01:06:42]

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

[commit-bot: I haz the power, 2016-08-25 01:07:51]

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

[dschuyler, 2016-08-25 01:08:35]

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

[dschuyler, 2016-08-25 01:08:48]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-25 01:09:32]

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

[dschuyler, 2016-08-25 01:21:46]

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

[commit-bot: I haz the power, 2016-08-25 01:22:32]

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

[dschuyler, 2016-08-25 01:24:32]

Patchset #2 (id:40001) has been deleted

[dschuyler, 2016-08-25 01:25:17]

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

[commit-bot: I haz the power, 2016-08-25 01:26:06]

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

[dschuyler, 2016-08-25 01:31:21]

Patchset #2 (id:60001) has been deleted

[dschuyler, 2016-08-25 01:34:44]

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

[commit-bot: I haz the power, 2016-08-25 01:36:01]

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

[dschuyler, 2016-08-25 02:00:54]

Thanks for looking it over. I was saving the layout changes for a follow up CL,
but I can include them now if that's easier.

Please take a look. If you'd like me to try breaking this up into separate CLs
again, just let me know.

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:75:
<h2>$i18n{creditCards}</h2>
On 2016/08/24 23:41:21, Hector Carmona wrote:
> Looks like this add some extra space between the lists.

I was separating the CLs. I have more changes to go after this.

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:108:
<h2>$i18n{passwordExceptionsHeading}</h2>
On 2016/08/24 23:41:21, Hector Carmona wrote:
> Extra space here also.

Acknowledged.

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

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:26:
<div class="settings-box block">
On 2016/08/24 23:41:21, Hector Carmona wrote:
> This is adding a dividing line between the header and this list.
> Is this expected?

Ah, I changed this in the next CL, but it should be in this CL

https://codereview.chromium.org/2273083002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/search_engines_page/search_engines_page.html:34:
<div class="settings-box block">
On 2016/08/24 23:41:21, Hector Carmona wrote:
> Same here for a dividing line between the lists.

I think a line makes sense here. I haven't found mocks for this though.

[commit-bot: I haz the power, 2016-08-25 02:42:20]

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

[commit-bot: I haz the power, 2016-08-25 02:42:21]

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

[hcarmona, 2016-08-25 21:37:43]

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
(left):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:82:
class="vertical-list list-section list-with-header">
Good catch. I didn't realize "list-section" was unused.

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
(right):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:39:
<h2>$i18n{addresses}</h2>
Can we add the classes directly to the h2?
(here and elsewhere)

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:31:
settings-search-engine-entry[tabindex='0'] {
This has the unintended side-effect that the top row will have a separator when
a different row is selected.

This is because the tabindex changes based on the selected row.

Maybe you want settings-search-engine-entry:first-of-type

[dschuyler, 2016-08-25 22:24:22]

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

[dschuyler, 2016-08-25 22:25:06]

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
(left):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:82:
class="vertical-list list-section list-with-header">
On 2016/08/25 21:37:43, Hector Carmona wrote:
> Good catch. I didn't realize "list-section" was unused.

Acknowledged.

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
(right):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:39:
<h2>$i18n{addresses}</h2>
On 2016/08/25 21:37:43, Hector Carmona wrote:
> Can we add the classes directly to the h2?
> (here and elsewhere)

Not easily. I did give it a go and it complicates
the line height. settings-box is intended to wrap
something and not to style the thing itself. I've
been told that div is extremely inexpensive, so I
don't expect this is adding too much.

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:31:
settings-search-engine-entry[tabindex='0'] {
On 2016/08/25 21:37:43, Hector Carmona wrote:
> This has the unintended side-effect that the top row will have a separator
when
> a different row is selected.
> 
> This is because the tabindex changes based on the selected row.
> 
> Maybe you want settings-search-engine-entry:first-of-type

Done.

[commit-bot: I haz the power, 2016-08-25 22:25:13]

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

[hcarmona, 2016-08-25 22:27:59]

lgtm

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
(right):

https://codereview.chromium.org/2273083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:39:
<h2>$i18n{addresses}</h2>
On 2016/08/25 22:25:05, dschuyler wrote:
> On 2016/08/25 21:37:43, Hector Carmona wrote:
> > Can we add the classes directly to the h2?
> > (here and elsewhere)
> 
> Not easily. I did give it a go and it complicates
> the line height. settings-box is intended to wrap
> something and not to style the thing itself. I've
> been told that div is extremely inexpensive, so I
> don't expect this is adding too much.

Acknowledged.

[commit-bot: I haz the power, 2016-08-25 23:29:53]

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

[commit-bot: I haz the power, 2016-08-25 23:29:54]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-08-26 01:38:10]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-08-26 01:38:34]

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-08-26 01:43:47]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-08-26 01:46:38]

Description was changed from

==========
[MD settings] using h2 for sub-headers consistently.

This CL switches several differently styled sub-headers to all use h2
tags. This is good because it makes them all consistent and h2 is a good
choice because screen readers may scan for h2 tags specifically to give
the user context about what is on the screen.

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

to

==========
[MD settings] using h2 for sub-headers consistently.

This CL switches several differently styled sub-headers to all use h2
tags. This is good because it makes them all consistent and h2 is a good
choice because screen readers may scan for h2 tags specifically to give
the user context about what is on the screen.

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

Committed: https://crrev.com/a56e76de0f7e38f81f5be072f98a44516124b8bd
Cr-Commit-Position: refs/heads/master@{#414621}
==========

[commit-bot: I haz the power, 2016-08-26 01:46:40]

Patchset 4 (id:??) landed as
https://crrev.com/a56e76de0f7e38f81f5be072f98a44516124b8bd
Cr-Commit-Position: refs/heads/master@{#414621}

[Dan Beam, 2017-01-10 22:28:00]

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

[Dan Beam, 2017-01-10 22:28:01]

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html
(right):

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:50:
<div class="settings-box first two-line">
why is this two-line?

[dschuyler, 2017-01-10 22:54:40]

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html
(right):

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:50:
<div class="settings-box first two-line">
On 2017/01/10 22:28:01, Dan Beam wrote:
> why is this two-line?

Likely for the sub-label="". It may be nicer if this were better connected to
the contents. And/or it may be nice to have two-line as an attribute for
two-line$="[[hasSubLabel]]" or similar.

[Dan Beam, 2017-01-10 22:58:55]

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html
(right):

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:50:
<div class="settings-box first two-line">
On 2017/01/10 22:54:40, dschuyler wrote:
> On 2017/01/10 22:28:01, Dan Beam wrote:
> > why is this two-line?
> 
> Likely for the sub-label="". It may be nicer if this were better connected to
> the contents. And/or it may be nice to have two-line as an attribute for
> two-line$="[[hasSubLabel]]" or similar.

connected to the contents like this?
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro...

[dschuyler, 2017-01-10 23:05:20]

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html
(right):

https://codereview.chromium.org/2273083002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:50:
<div class="settings-box first two-line">
On 2017/01/10 22:58:55, Dan Beam wrote:
> On 2017/01/10 22:54:40, dschuyler wrote:
> > On 2017/01/10 22:28:01, Dan Beam wrote:
> > > why is this two-line?
> > 
> > Likely for the sub-label="". It may be nicer if this were better connected
to
> > the contents. And/or it may be nice to have two-line as an attribute for
> > two-line$="[[hasSubLabel]]" or similar.
> 
> connected to the contents like this?
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro...

Like that, yes.