MD Settings: <settings-dialog>, restore focus effect for the cancel button.

MD Settings: <settings-dialog>, restore focus effect for the cancel button.

BUG=608618
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/33bb44e0acccaf4aef4cf9342ae3f5eb9dce856d
Cr-Commit-Position: refs/heads/master@{#391688}

[dpapad, 2016-05-03 18:58:50]

Description was changed from

==========
MD Settings: <settings-dialog>, restore focus effect for the cancel button.

BUG=608618
==========

to

==========
MD Settings: <settings-dialog>, restore focus effect for the cancel button.

BUG=608618
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-03 19:00:07]

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

[dpapad, 2016-05-03 19:00:07]

keyboard-focus CSS class specifies "font-weight: bold", but it was being
accidentally overriden.

[Dan Beam, 2016-05-03 19:06:28]

drive-by

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

https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_dialog.html:73: :host ::content
.button-container .cancel-button:not(.keyboard-focus) {
i don't think .keyboard-focus is part of the public API; can you use something
that is publicly documented on
https://elements.polymer-project.org/elements/paper-button?  maybe
[aria-pressed] or --paper-button-*-focus?

[dpapad, 2016-05-03 19:25:36]

On 2016/05/03 at 19:06:28, dbeam wrote:
> drive-by
> 
>
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/settings_dialog.html (right):
> 
>
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/settings_dialog.html:73: :host ::content
.button-container .cancel-button:not(.keyboard-focus) {
> i don't think .keyboard-focus is part of the public API; can you use something
that is publicly documented on
https://elements.polymer-project.org/elements/paper-button?  maybe
[aria-pressed] or --paper-button-*-focus?

I agree that using .keyboard-focus is not ideal. The current problem I am seeing
is that the ":host ::content foo" selector is treated as having higher
specificity than wanted. For a comparison see this difference between
action-button and cancel-button which makes action-button not face the focus
issue,

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

In the first case, the "font-weight: 500" does not have higher specificity than
.keyboard-focus and therefore it works fine. An easy solution would be to move
the .cancel-button styling to settings_shared_css.html instead of
settings_dialog.html (and remove ":host ::content" part of the selector).

[dpapad, 2016-05-04 00:14:42]

On 2016/05/03 at 19:25:36, dpapad wrote:
> On 2016/05/03 at 19:06:28, dbeam wrote:
> > drive-by
> > 
> >
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> > File chrome/browser/resources/settings/settings_dialog.html (right):
> > 
> >
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> > chrome/browser/resources/settings/settings_dialog.html:73: :host ::content
.button-container .cancel-button:not(.keyboard-focus) {
> > i don't think .keyboard-focus is part of the public API; can you use
something that is publicly documented on
https://elements.polymer-project.org/elements/paper-button?  maybe
[aria-pressed] or --paper-button-*-focus?
> 
> I agree that using .keyboard-focus is not ideal. The current problem I am
seeing is that the ":host ::content foo" selector is treated as having higher
specificity than wanted. For a comparison see this difference between
action-button and cancel-button which makes action-button not face the focus
issue,
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> In the first case, the "font-weight: 500" does not have higher specificity
than .keyboard-focus and therefore it works fine. An easy solution would be to
move the .cancel-button styling to settings_shared_css.html instead of
settings_dialog.html (and remove ":host ::content" part of the selector).

PTAL. For the reasons about specificity mentioned above, font-weight can't be
specified within settings-dialog itself, so I moved to settings_shared_css.html
instead, and removed usage of the non public keyboard-focus CSS class.

[dpapad, 2016-05-04 20:20:45]

On 2016/05/04 at 00:14:42, dpapad wrote:
> On 2016/05/03 at 19:25:36, dpapad wrote:
> > On 2016/05/03 at 19:06:28, dbeam wrote:
> > > drive-by
> > > 
> > >
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> > > File chrome/browser/resources/settings/settings_dialog.html (right):
> > > 
> > >
https://codereview.chromium.org/1951503002/diff/1/chrome/browser/resources/se...
> > > chrome/browser/resources/settings/settings_dialog.html:73: :host ::content
.button-container .cancel-button:not(.keyboard-focus) {
> > > i don't think .keyboard-focus is part of the public API; can you use
something that is publicly documented on
https://elements.polymer-project.org/elements/paper-button?  maybe
[aria-pressed] or --paper-button-*-focus?
> > 
> > I agree that using .keyboard-focus is not ideal. The current problem I am
seeing is that the ":host ::content foo" selector is treated as having higher
specificity than wanted. For a comparison see this difference between
action-button and cancel-button which makes action-button not face the focus
issue,
> > 
> >
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

> >
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > In the first case, the "font-weight: 500" does not have higher specificity
than .keyboard-focus and therefore it works fine. An easy solution would be to
move the .cancel-button styling to settings_shared_css.html instead of
settings_dialog.html (and remove ":host ::content" part of the selector).
> 
> PTAL. For the reasons about specificity mentioned above, font-weight can't be
specified within settings-dialog itself, so I moved to settings_shared_css.html
instead, and removed usage of the non public keyboard-focus CSS class.

Friendly ping.

[dschuyler, 2016-05-04 22:17:18]

lgtm

[dpapad, 2016-05-04 22:28:42]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-05-04 22:29:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951503002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951503002/20001

[commit-bot: I haz the power, 2016-05-04 23:30:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-04 23:31:25]

Description was changed from

==========
MD Settings: <settings-dialog>, restore focus effect for the cancel button.

BUG=608618
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: <settings-dialog>, restore focus effect for the cancel button.

BUG=608618
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/33bb44e0acccaf4aef4cf9342ae3f5eb9dce856d
Cr-Commit-Position: refs/heads/master@{#391688}
==========

[commit-bot: I haz the power, 2016-05-04 23:31:26]

Patchset 2 (id:??) landed as
https://crrev.com/33bb44e0acccaf4aef4cf9342ae3f5eb9dce856d
Cr-Commit-Position: refs/heads/master@{#391688}