Fix style issues with MD accessibility page

Fix style issues with MD accessibility page

Fix subheading style by using h2 instead of h3 (it has the correct
style by default now).

Add 16px space before settings-dropdown-menu inside of a list-item.

Update text link style based on bug 649427 (should this be in
settings_shared_css.html or is this fine for now?)

BUG=649421, 649431, 649427
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[dmazzoni, 2016-09-27 18:19:28]

Description was changed from

==========
Fix style issues with MD accessibility page

Fix subheading style by using h2 instead of h3 (it has the correct
style by default now).

Add 16px space before settings-dropdown-menu inside of a list-item.

Update text link style based on bug 649427 (should this be in
settings_shared_css.html or is this fine for now?)

BUG=649421, 649431, 649427
==========

to

==========
Fix style issues with MD accessibility page

Fix subheading style by using h2 instead of h3 (it has the correct
style by default now).

Add 16px space before settings-dropdown-menu inside of a list-item.

Update text link style based on bug 649427 (should this be in
settings_shared_css.html or is this fine for now?)

BUG=649421, 649431, 649427
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-09-27 20:38:46]

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

[dschuyler, 2016-09-27 22:41:28]

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:1: <link
rel="import" href="chrome://resources/html/action_link.html">
WDYT about making a Polymer-ish control 
for action-link? I just wondered if that
would be a good place to put the styling
I mention in the next note.

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:10: <link
rel="import" type="css" href="chrome://resources/css/action_link.css">
Ah, this method importing css is deprecated.
(I know there is still code doing it, but we're
trying to not add more).
Could we make any needed changes in 
settings_shared_css.html and avoid doing an
import here?

The new way to access css is either with a
<style include=""> like line 12 below. Or,
by inlining it where it's needed (like line
13 below).

The way to choose which to do is based on
whether the css is a one-off or is used 
in several settings pages.

[Dan Beam, 2016-09-27 22:50:00]

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:36: <a
href="$i18nRaw{a11yLearnMoreUrl}" target="_blank" is="action-link">
Dominic: I added <a is="action-link"> for when something looks like a link but
needs to handle an action (i.e. in JS).

why are you using it here when this <a>... is already a link?  is there some
a11y benefit?  is it just for styling?

[Dan Beam, 2016-09-27 22:51:17]

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:10: <link
rel="import" type="css" href="chrome://resources/css/action_link.css">
On 2016/09/27 22:41:27, dschuyler wrote:
> Ah, this method importing css is deprecated.
> (I know there is still code doing it, but we're
> trying to not add more).
> Could we make any needed changes in 
> settings_shared_css.html and avoid doing an
> import here?
> 
> The new way to access css is either with a
> <style include=""> like line 12 below. Or,
> by inlining it where it's needed (like line
> 13 below).
> 
> The way to choose which to do is based on
> whether the css is a one-off or is used 
> in several settings pages.

for future reference: action_link.css is used by old webui via <link
rel="stylesheet"> so it gets a free pass from this rule

also, Polymer has not yet decided when removing type="css" will happen because
many people (including those in Google) prefer to keep their css in an external
.css file instead of in an .html file like we are

[dmazzoni, 2016-09-27 22:53:38]

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:36: <a
href="$i18nRaw{a11yLearnMoreUrl}" target="_blank" is="action-link">
On 2016/09/27 22:50:00, Dan Beam wrote:
> Dominic: I added <a is="action-link"> for when something looks like a link but
> needs to handle an action (i.e. in JS).
> 
> why are you using it here when this <a>... is already a link?  is there some
> a11y benefit?  is it just for styling?

I asked Hector and Dave and they said that this was the preferred way to
get the same visual style.

Should we have a css class? Or should there just be a style rule in
settings_shared_css.html that applies to all links?

[Dan Beam, 2016-09-27 22:56:02]

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:36: <a
href="$i18nRaw{a11yLearnMoreUrl}" target="_blank" is="action-link">
On 2016/09/27 22:53:38, dmazzoni wrote:
> On 2016/09/27 22:50:00, Dan Beam wrote:
> > Dominic: I added <a is="action-link"> for when something looks like a link
but
> > needs to handle an action (i.e. in JS).
> > 
> > why are you using it here when this <a>... is already a link?  is there some
> > a11y benefit?  is it just for styling?
> 
> I asked Hector and Dave and they said that this was the preferred way to
> get the same visual style.
> 
> Should we have a css class? Or should there just be a style rule in
> settings_shared_css.html that applies to all links?

I think we probably just need to update this:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

to include a:link or a[href] or maybe a (I guess)

[dmazzoni, 2016-09-28 19:46:17]

Updated to use a style rule for a[href] rather than
action-link, PTAL.

[dmazzoni, 2016-09-28 19:46:28]

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

[commit-bot: I haz the power, 2016-09-28 19:46:57]

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

[Dan Beam, 2016-09-28 20:28:54]

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

[Dan Beam, 2016-09-28 20:28:55]

lgtm w/nits

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:28:
margin-left: 16px;
-webkit-margin-start: 16px;

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

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:120: 
can we combine the text-decoration rule with this one?

:-webkit-any(a[href], [is='action-link']):hover {
  text-decoration: none;
}

[commit-bot: I haz the power, 2016-09-28 21:13:04]

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

[commit-bot: I haz the power, 2016-09-28 21:13:05]

Dry run: 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_...)

[dschuyler, 2016-09-28 21:34:51]

My nits would be about the same as Dans. LGTM

[dmazzoni, 2016-09-28 22:11:43]

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/a11y_page/manage_a11y_page.html (right):

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/a11y_page/manage_a11y_page.html:28:
margin-left: 16px;
On 2016/09/28 20:28:55, Dan Beam wrote:
> -webkit-margin-start: 16px;

Done. I need to learn how RTL flips everything
around like that. I also need to learn why there
isn't an unprefixed standard for -webkit-margin-start
yet.

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

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:120: 
On 2016/09/28 20:28:55, Dan Beam wrote:
> can we combine the text-decoration rule with this one?
> 
> :-webkit-any(a[href], [is='action-link']):hover {
>   text-decoration: none;
> }

Sure.

I combined it slightly differently so as not to break
anything else, please follow-up if that's not what you
had in mind.

:-webkit-any(a[href], [is='action-link']:hover) {

[dmazzoni, 2016-09-28 22:12:15]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-09-28 22:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2373713004/#ps80001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-09-28 22:13:00]

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

[Dan Beam, 2016-09-28 22:15:08]

still lgtm

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

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:120: 
On 2016/09/28 22:11:43, dmazzoni wrote:
> On 2016/09/28 20:28:55, Dan Beam wrote:
> > can we combine the text-decoration rule with this one?
> > 
> > :-webkit-any(a[href], [is='action-link']):hover {
> >   text-decoration: none;
> > }
> 
> Sure.
> 
> I combined it slightly differently so as not to break
> anything else, please follow-up if that's not what you
> had in mind.
> 
> :-webkit-any(a[href], [is='action-link']:hover) {

well, that's fine, but it's equivalent to

a[href],
[is='action-link']:hover {
  /* ... */
}

which is more common.  :-webkit-any() is really only helpful as a prefixing
selector (i.e. any(thing1, thing2) more stuff {...})

[dmazzoni, 2016-09-28 22:17:50]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-09-28 22:29:43]

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

https://codereview.chromium.org/2373713004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:120: 
On 2016/09/28 22:15:08, Dan Beam wrote:
> On 2016/09/28 22:11:43, dmazzoni wrote:
> > On 2016/09/28 20:28:55, Dan Beam wrote:
> > > can we combine the text-decoration rule with this one?
> > > 
> > > :-webkit-any(a[href], [is='action-link']):hover {
> > >   text-decoration: none;
> > > }
> > 
> > Sure.
> > 
> > I combined it slightly differently so as not to break
> > anything else, please follow-up if that's not what you
> > had in mind.
> > 
> > :-webkit-any(a[href], [is='action-link']:hover) {
> 
> well, that's fine, but it's equivalent to
> 
> a[href],
> [is='action-link']:hover {
>   /* ... */
> }
> 
> which is more common.  :-webkit-any() is really only helpful as a prefixing
> selector (i.e. any(thing1, thing2) more stuff {...})

Oh, of course. Fixed.

Your suggestion would have only removed the underline on hover,
which isn't what I wanted - and I didn't want to remove
the underline from all action-links in this change.

[dmazzoni, 2016-09-28 22:29:49]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-09-28 22:29:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2373713004/#ps100001
(title: "Got rid of redundant -webkit-any")

[commit-bot: I haz the power, 2016-09-28 22:30: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, 2016-09-28 23:35:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-28 23:46:52]

Description was changed from

==========
Fix style issues with MD accessibility page

Fix subheading style by using h2 instead of h3 (it has the correct
style by default now).

Add 16px space before settings-dropdown-menu inside of a list-item.

Update text link style based on bug 649427 (should this be in
settings_shared_css.html or is this fine for now?)

BUG=649421, 649431, 649427
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix style issues with MD accessibility page

Fix subheading style by using h2 instead of h3 (it has the correct
style by default now).

Add 16px space before settings-dropdown-menu inside of a list-item.

Update text link style based on bug 649427 (should this be in
settings_shared_css.html or is this fine for now?)

BUG=649421, 649431, 649427
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-09-28 23:46:53]

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