Focus style for Add supervised user button

Focus style for Add supervised user btn

BUG=642330
TEST=On login page, click More options [...] button, and TAB through
elements.  Blue focus rect should be seen on Add supervised user button.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2610193002
Cr-Commit-Position: refs/heads/master@{#448377}
Committed: https://chromium.googlesource.com/chromium/src/+/c5a91c9388a44ff129f6234a84a399b1abd97cba

[Greg Levin, 2017-01-04 18:51:25]

Description was changed from

==========
Focus style for Add supervised user btn

BUG=642330
TEST=On login page, click More options [...] button, and TAB through
elements.  Blue focus rect should be seen on Add supervised user button.
==========

to

==========
Focus style for Add supervised user btn

BUG=642330
TEST=On login page, click More options [...] button, and TAB through
elements.  Blue focus rect should be seen on Add supervised user button.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Greg Levin, 2017-01-04 19:07:56]

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

[commit-bot: I haz the power, 2017-01-04 19:08: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-04 20:21:12]

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

[commit-bot: I haz the power, 2017-01-04 20:21:13]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2017-01-04 21:31:25]

glevin@chromium.org changed reviewers:
+ alemate@chromium.org, estade@chromium.org

[Greg Levin, 2017-01-04 21:31:25]

Please have a look!

NOTE: While not actually dependent, this CL goes along with
  https://codereview.chromium.org/2609393003
which lets the ESC button close this "menu" and improves focus and TABing, and
this CL
  https://codereview.chromium.org/2607103003/
which implements the same style on other login shelf buttons.

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.css (right):

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:75: margin: 0 0 -10px
-10px;
I'm still learning the subtleties of css.  This appears to have the same effect
as

  bottom: 0
  left: 5px;
  margin: 0 0 0 0;

What's the merit of the way that it's currently written?

[Evan Stade, 2017-01-04 23:43:17]

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.css (right):

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:70: color: black
!important;
side note: we try hard to avoid !important

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:75: margin: 0 0 -10px
-10px;
On 2017/01/04 21:31:25, Greg Levin wrote:
> I'm still learning the subtleties of css.  This appears to have the same
effect
> as
> 
>   bottom: 0

do you not want bottom:5px? Why is this button moving down by 5px?

>   left: 5px;
>   margin: 0 0 0 0;
> 
> What's the merit of the way that it's currently written?

CSS is a house of cards because of cascading rules. Did you check what this
looks like in RTL? Looking at code history can also help shed light, but the
change where this is added[1] isn't that helpful. Since this mysterious pattern
was applied without much commentary (either in code or in rietveld), I'd just
assume it was an oversight.

If your change seems to do the right thing (and do you even need the margin
rule?) and you're feeling lucky, take the chance.

[1] https://codereview.chromium.org/1013613002

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:89: right: 0;
I believe you could just move this rule into the non-rtl block because if both
left and right are specified, the one that wins out depends on text direction
(indeed that's why this works, as we aren't resetting left in this block).

[Greg Levin, 2017-01-05 18:25:54]

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/header_bar.css (right):

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:70: color: black
!important;
On 2017/01/04 23:43:17, Evan Stade wrote:
> side note: we try hard to avoid !important

Acknowledged (no idea why this was marked important)

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:75: margin: 0 0 -10px
-10px;
On 2017/01/04 23:43:17, Evan Stade wrote:
> On 2017/01/04 21:31:25, Greg Levin wrote:
> > I'm still learning the subtleties of css.  This appears to have the same
> > effect as
> > 
> >   bottom: 0
> 
> do you not want bottom:5px? Why is this button moving down by 5px?

Requested in issue comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=642330#c5
Also see comments 6 & 7.  I don't know why this was originally offset 5px up and
right, but no one seems to object to it being directly over the [...] button
instead.

> >   left: 5px;
> >   margin: 0 0 0 0;
> > 
> > What's the merit of the way that it's currently written?
> 
> CSS is a house of cards because of cascading rules. Did you check what this
> looks like in RTL? Looking at code history can also help shed light, but the
> change where this is added[1] isn't that helpful. Since this mysterious
> pattern was applied without much commentary (either in code or in rietveld),
> I'd just assume it was an oversight.
> 
> If your change seems to do the right thing (and do you even need the margin
> rule?) and you're feeling lucky, take the chance.
> 
> [1] https://codereview.chromium.org/1013613002

All these offsets and margins seem to be for moving the box +5px up and right. 
Without that, they seem unnecessary (except "bottom: 0", without which the box
sinks half-way off the screen; no idea why).  Also works fine RTL.

https://codereview.chromium.org/2610193002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/header_bar.css:89: right: 0;
On 2017/01/04 23:43:17, Evan Stade wrote:
> I believe you could just move this rule into the non-rtl block because if both
> left and right are specified, the one that wins out depends on text direction
> (indeed that's why this works, as we aren't resetting left in this block).

Turns out this rule was only needed because "left: 15px" was putting the RTL box
in the wrong place.  Without that line, the RTL version of the box is in the
right place by default.

[Evan Stade, 2017-01-05 18:34:37]

lgtm

[Greg Levin, 2017-01-09 21:43:43]

glevin@chromium.org changed reviewers:
+ xiyuan@chromium.org
- alemate@chromium.org

[Greg Levin, 2017-01-09 21:43:43]

-alemate (seems unavailable)
+xiyuan (please have a look!)

[xiyuan, 2017-01-09 21:45:59]

lgtm

[Greg Levin, 2017-02-06 19:31:42]

The CQ bit was checked by glevin@chromium.org

[commit-bot: I haz the power, 2017-02-06 19:32:35]

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-06 20:35:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486409502669210,
"parent_rev": "10e14c24b244e7f58074ae74b0181cb3cb60551e", "commit_rev":
"c5a91c9388a44ff129f6234a84a399b1abd97cba"}

[commit-bot: I haz the power, 2017-02-06 20:36:33]

Description was changed from

==========
Focus style for Add supervised user btn

BUG=642330
TEST=On login page, click More options [...] button, and TAB through
elements.  Blue focus rect should be seen on Add supervised user button.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Focus style for Add supervised user btn

BUG=642330
TEST=On login page, click More options [...] button, and TAB through
elements.  Blue focus rect should be seen on Add supervised user button.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2610193002
Cr-Commit-Position: refs/heads/master@{#448377}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5a91c9388a44ff129f6234a84a3...
==========

[commit-bot: I haz the power, 2017-02-06 20:36:34]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c5a91c9388a44ff129f6234a84a3...