Unified icon-buttons used in the new GAIA flow.

Unified icon-buttons used in the new GAIA flow.

This CL introduces the 'gaia-icon-button' decorator, which can be applied to any
<button>.
All icon buttons introduced recently are <button is="gaia-icon-button">s now.

BUG=478545, 472671
TEST=manual

Committed: https://crrev.com/38e0418e79bd5b0f3e2e86beab7d6c60970532aa
Cr-Commit-Position: refs/heads/master@{#329284}

[dzhioev (left Google), 2015-05-08 04:32:44]

dzhioev@chromium.org changed reviewers:
+ glevin@chromium.org

[dzhioev (left Google), 2015-05-08 04:32:45]

Hi Greg,

Rationale for this CL:
a) I've noticed that your CL https://codereview.chromium.org/1121723004 doesn't
cover icon-buttons that are displayed on offline-login and password-changed
screens. They were <gaia-core-icon-button>s.
b) I'm going to add another icon-button in scope of http://crbug.com/472671, but
I don't want to add new "*.png" assets for that.

Now all icon-buttons used in the new GAIA-flow are <button
is="gaia-icon-button">. They use SVG icons from Polymer's <core-icons>. Buttons
that you've changed recently have exactly same look and fell as before
(including focusing, activation, etc).

PTAL

[dzhioev (left Google), 2015-05-08 04:37:03]

dzhioev@chromium.org changed reviewers:
+ oshima@chromium.org

[dzhioev (left Google), 2015-05-08 04:40:55]

+oshima@ for chrome/app/theme/* OWNER's review
Mitsuru, please review.

[oshima, 2015-05-08 15:36:25]

c/a/theme lgtm

[Greg Levin, 2015-05-08 19:18:07]

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:19: /* Prevents button
focusing after mouse click. */
Is this desirable?  Most buttons seem to retain focus after a mouse click,
including shelf (e.g. Shut down) and GAIA (e.g. NEXT) buttons.  I'd lean towards
removing this, unless it's solving a particular problem.

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
This change (?) has removed the blue focus rectangle from the shelf buttons
(i.e. Shut down, Browse as guest, Add person).  TABing to them no longer gives a
visual cue that they have focus.

Also, nit: I'm not familiar with js style guidelines, but should this line wrap
to meet 80-column limit?

[dzhioev (left Google), 2015-05-08 21:15:46]

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:19: /* Prevents button
focusing after mouse click. */
On 2015/05/08 19:18:07, Greg Levin wrote:
> Is this desirable?  Most buttons seem to retain focus after a mouse click,
> including shelf (e.g. Shut down) and GAIA (e.g. NEXT) buttons.  I'd lean
towards
> removing this, unless it's solving a particular problem.

Yes, we do this thing for the most buttons in our UI. Buttons affected in this
CL had such behavior before too (see removed lines in 'screen_gaia_signin.js').
This prevents focusing in "mousedown"->move mouse away->"mouseup" case (see
video https://goo.gl/Hmswa4 ) If I remember things right, UX designers
considered such behavior as an UI glitch.

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/08 19:18:07, Greg Levin wrote:
> This change (?) has removed the blue focus rectangle from the shelf buttons
> (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer gives
a
> visual cue that they have focus.
Nice catch! Adding attribute selector increased this selector's specificity and
made it higher than "button.custom-appearance:focus:focus" specificity. Added
another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29

> Also, nit: I'm not familiar with js style guidelines, but should this line
wrap
> to meet 80-column limit?
It should, but I don't know how to break long selectors correctly. Breaking line
before ':' will change selector's meaning.

[dzhioev (left Google), 2015-05-08 21:21:16]

dzhioev@chromium.org changed reviewers:
+ achuith@chromium.org

[dzhioev (left Google), 2015-05-08 21:27:47]

Achuith, please review.
Everybody (except me) from "chrome/browser/resources/chromeos/login/OWNERS" are
on weekends until next Tuesday because of Victory Day, and you seem like most
appropriate person from "chrome/browser/resources/chromeos/OWNERS" for this CL
(stevenjb@ is on vacation). Feel free to reassign this to somebody else.

[dzhioev (left Google), 2015-05-08 21:44:16]

dzhioev@chromium.org changed reviewers:
+ michaelpg@chromium.org
- achuith@chromium.org

[dzhioev (left Google), 2015-05-08 21:44:17]

I realized, that I don't need OWNERS review, because I am an owner. So I'm
removing Achuith and adding Michael instead.
Michael, PTAL.

[michaelpg, 2015-05-08 22:02:23]

LGTM w/ nits

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/08 21:15:45, dzhioev wrote:
> On 2015/05/08 19:18:07, Greg Levin wrote:
> > This change (?) has removed the blue focus rectangle from the shelf buttons
> > (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer
gives
> a
> > visual cue that they have focus.
> Nice catch! Adding attribute selector increased this selector's specificity
and
> made it higher than "button.custom-appearance:focus:focus" specificity. Added
> another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> 
> > Also, nit: I'm not familiar with js style guidelines, but should this line
> wrap
> > to meet 80-column limit?
> It should, but I don't know how to break long selectors correctly. Breaking
line
> before ':' will change selector's meaning.

I don't think we technically have a line length limit for CSS anywhere.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.html (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.html:25: <polymer-element
name="gaia-icon-button" extends="button" attributes="icon"
Note: this kind of inheritance isn't available in Polymer 0.8. I'd suggest using
a standalone element or looking into prototype mixins.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:15:
this.classList.toggle('custom-appearance', true);
this.classList.add('custom-appearance');

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_icon_button.css (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_icon_button.css:22: background:
rgba(0,0,0,.16);
nit: space after commas

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_icon_button.css:28: background:
rgba(0,0,0,.12);
nit: space after commas

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.css
(right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.css:132:
color: rgba(0,0,0,.54);
same nits

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.css (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/screen_gaia_signin.css:78: color:
rgb(127,127,127);
same nits

[dzhioev (left Google), 2015-05-08 22:29:03]

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/08 22:02:22, michaelpg wrote:
> On 2015/05/08 21:15:45, dzhioev wrote:
> > On 2015/05/08 19:18:07, Greg Levin wrote:
> > > This change (?) has removed the blue focus rectangle from the shelf
buttons
> > > (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer
> gives
> > a
> > > visual cue that they have focus.
> > Nice catch! Adding attribute selector increased this selector's specificity
> and
> > made it higher than "button.custom-appearance:focus:focus" specificity.
Added
> > another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> > 
> > > Also, nit: I'm not familiar with js style guidelines, but should this line
> > wrap
> > > to meet 80-column limit?
> > It should, but I don't know how to break long selectors correctly. Breaking
> line
> > before ':' will change selector's meaning.
> 
> I don't think we technically have a line length limit for CSS anywhere.

We do have a limit, 80 characters as usual.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.html (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.html:25: <polymer-element
name="gaia-icon-button" extends="button" attributes="icon"
On 2015/05/08 22:02:22, michaelpg wrote:
> Note: this kind of inheritance isn't available in Polymer 0.8. I'd suggest
using
> a standalone element or looking into prototype mixins.

We are going to use <paper-icon-button> in 0.8 anyway. We can't use it now
because it is broken in 0.5.*

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:15:
this.classList.toggle('custom-appearance', true);
On 2015/05/08 22:02:23, michaelpg wrote:
> this.classList.add('custom-appearance');

Done.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_icon_button.css (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_icon_button.css:22: background:
rgba(0,0,0,.16);
On 2015/05/08 22:02:23, michaelpg wrote:
> nit: space after commas

Done.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_icon_button.css:28: background:
rgba(0,0,0,.12);
On 2015/05/08 22:02:23, michaelpg wrote:
> nit: space after commas

Done.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.css
(right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.css:132:
color: rgba(0,0,0,.54);
On 2015/05/08 22:02:23, michaelpg wrote:
> same nits

Done.

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.css (right):

https://codereview.chromium.org/1133733003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/screen_gaia_signin.css:78: color:
rgb(127,127,127);
On 2015/05/08 22:02:23, michaelpg wrote:
> same nits

Done.

[michaelpg, 2015-05-08 22:33:39]

SLGTM, thanks!

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/08 22:29:02, dzhioev wrote:
> On 2015/05/08 22:02:22, michaelpg wrote:
> > On 2015/05/08 21:15:45, dzhioev wrote:
> > > On 2015/05/08 19:18:07, Greg Levin wrote:
> > > > This change (?) has removed the blue focus rectangle from the shelf
> buttons
> > > > (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer
> > gives
> > > a
> > > > visual cue that they have focus.
> > > Nice catch! Adding attribute selector increased this selector's
specificity
> > and
> > > made it higher than "button.custom-appearance:focus:focus" specificity.
> Added
> > > another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> > > 
> > > > Also, nit: I'm not familiar with js style guidelines, but should this
line
> > > wrap
> > > > to meet 80-column limit?
> > > It should, but I don't know how to break long selectors correctly.
Breaking
> > line
> > > before ':' will change selector's meaning.
> > 
> > I don't think we technically have a line length limit for CSS anywhere.
> 
> We do have a limit, 80 characters as usual. 

Ah ok. Can you point me to the style guide?

[dzhioev (left Google), 2015-05-08 22:57:19]

On 2015/05/08 22:33:39, michaelpg wrote:
> SLGTM, thanks!
> 
> https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
> File ui/login/oobe.css (right):
> 
>
https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
> ui/login/oobe.css:37:
>
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
> {
> On 2015/05/08 22:29:02, dzhioev wrote:
> > On 2015/05/08 22:02:22, michaelpg wrote:
> > > On 2015/05/08 21:15:45, dzhioev wrote:
> > > > On 2015/05/08 19:18:07, Greg Levin wrote:
> > > > > This change (?) has removed the blue focus rectangle from the shelf
> > buttons
> > > > > (i.e. Shut down, Browse as guest, Add person).  TABing to them no
longer
> > > gives
> > > > a
> > > > > visual cue that they have focus.
> > > > Nice catch! Adding attribute selector increased this selector's
> specificity
> > > and
> > > > made it higher than "button.custom-appearance:focus:focus" specificity.
> > Added
> > > > another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> > > > 
> > > > > Also, nit: I'm not familiar with js style guidelines, but should this
> line
> > > > wrap
> > > > > to meet 80-column limit?
> > > > It should, but I don't know how to break long selectors correctly.
> Breaking
> > > line
> > > > before ':' will change selector's meaning.
> > > 
> > > I don't think we technically have a line length limit for CSS anywhere.
> > 
> > We do have a limit, 80 characters as usual. 
> 
> Ah ok. Can you point me to the style guide?

No I can't, because it is not in style guide =) Presubmit gives a warning about
long lines.
Style guide https://www.chromium.org/developers/web-development-style-guide
explicitly specifies limit for HTML.

[Greg Levin, 2015-05-11 14:33:50]

LGTM

I'm still not sure about the focus-on-click issue, but it certainly doesn't need
to hold up this CL.

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:19: /* Prevents button
focusing after mouse click. */
On 2015/05/08 21:15:45, dzhioev wrote:
> On 2015/05/08 19:18:07, Greg Levin wrote:
> > Is this desirable?  Most buttons seem to retain focus after a mouse click,
> > including shelf (e.g. Shut down) and GAIA (e.g. NEXT) buttons.  I'd lean
> towards
> > removing this, unless it's solving a particular problem.
> 
> Yes, we do this thing for the most buttons in our UI. Buttons affected in this
> CL had such behavior before too (see removed lines in
'screen_gaia_signin.js').
> This prevents focusing in "mousedown"->move mouse away->"mouseup" case (see
> video https://goo.gl/Hmswa4 ) If I remember things right, UX designers
> considered such behavior as an UI glitch.

Right, but it's not clear to me that we *want* to prevent focus on
"mousedown"->move mouse away->"mouseup" (md-mma-mu).  As I said above, most of
the other buttons on the login page (ChromeOS and GAIA) take focus on md-mma-mu.
 This is also the default behavior in web pages (md-mma-mu a button or link on a
web page, and that element will be focused), and consequently, in a lot of
places in ChromeOS (e.g. Files app, Settings page).  There are other places
where this doesn't happen, of course... it's not really consistent.

It's not a major issue either way, but it would be nice to have some guidance
from UX.  If we decide to prevent focus-on-md-mma-mu, then we should probably
file bugs against ChromeOS and GAIA to remove this behavior from the rest of the
login screen, just to keep things consistent.

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/08 21:15:45, dzhioev wrote:
> On 2015/05/08 19:18:07, Greg Levin wrote:
> > This change (?) has removed the blue focus rectangle from the shelf buttons
> > (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer
gives
> a
> > visual cue that they have focus.
> Nice catch! Adding attribute selector increased this selector's specificity
and
> made it higher than "button.custom-appearance:focus:focus" specificity. Added
> another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> 
> > Also, nit: I'm not familiar with js style guidelines, but should this line
> wrap
> > to meet 80-column limit?
> It should, but I don't know how to break long selectors correctly. Breaking
line
> before ':' will change selector's meaning.

I don't really know css syntax, but experimentally, this seems to work:

button.custom-appearance:not(.button-restricted):not(.button-fancy):not(
    [is="gaia-icon-button"]) {

I'll leave it to you to decide which is aesthetically preferable.

[dzhioev (left Google), 2015-05-11 23:05:58]

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/gaia_buttons.js (right):

https://codereview.chromium.org/1133733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/gaia_buttons.js:19: /* Prevents button
focusing after mouse click. */
On 2015/05/11 14:33:50, Greg Levin wrote:
> On 2015/05/08 21:15:45, dzhioev wrote:
> > On 2015/05/08 19:18:07, Greg Levin wrote:
> > > Is this desirable?  Most buttons seem to retain focus after a mouse click,
> > > including shelf (e.g. Shut down) and GAIA (e.g. NEXT) buttons.  I'd lean
> > towards
> > > removing this, unless it's solving a particular problem.
> > 
> > Yes, we do this thing for the most buttons in our UI. Buttons affected in
this
> > CL had such behavior before too (see removed lines in
> 'screen_gaia_signin.js').
> > This prevents focusing in "mousedown"->move mouse away->"mouseup" case (see
> > video https://goo.gl/Hmswa4 ) If I remember things right, UX designers
> > considered such behavior as an UI glitch.
> 
> Right, but it's not clear to me that we *want* to prevent focus on
> "mousedown"->move mouse away->"mouseup" (md-mma-mu).  As I said above, most of
> the other buttons on the login page (ChromeOS and GAIA) take focus on
md-mma-mu.
>  This is also the default behavior in web pages (md-mma-mu a button or link on
a
> web page, and that element will be focused), and consequently, in a lot of
> places in ChromeOS (e.g. Files app, Settings page).  There are other places
> where this doesn't happen, of course... it's not really consistent.
> 
> It's not a major issue either way, but it would be nice to have some guidance
> from UX.  If we decide to prevent focus-on-md-mma-mu, then we should probably
> file bugs against ChromeOS and GAIA to remove this behavior from the rest of
the
> login screen, just to keep things consistent.

Got it, I'll keep current behavior for now and consult with UX about this.

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css
File ui/login/oobe.css (right):

https://codereview.chromium.org/1133733003/diff/60001/ui/login/oobe.css#newco...
ui/login/oobe.css:37:
button.custom-appearance:not(.button-restricted):not(.button-fancy):not([is="gaia-icon-button"])
{
On 2015/05/11 14:33:50, Greg Levin wrote:
> On 2015/05/08 21:15:45, dzhioev wrote:
> > On 2015/05/08 19:18:07, Greg Levin wrote:
> > > This change (?) has removed the blue focus rectangle from the shelf
buttons
> > > (i.e. Shut down, Browse as guest, Add person).  TABing to them no longer
> gives
> > a
> > > visual cue that they have focus.
> > Nice catch! Adding attribute selector increased this selector's specificity
> and
> > made it higher than "button.custom-appearance:focus:focus" specificity.
Added
> > another ":focus" to fix it. http://imgur.com/gallery/Q3cUg29
> > 
> > > Also, nit: I'm not familiar with js style guidelines, but should this line
> > wrap
> > > to meet 80-column limit?
> > It should, but I don't know how to break long selectors correctly. Breaking
> line
> > before ':' will change selector's meaning.
> 
> I don't really know css syntax, but experimentally, this seems to work:
> 
> button.custom-appearance:not(.button-restricted):not(.button-fancy):not(
>     [is="gaia-icon-button"]) {
> 
> I'll leave it to you to decide which is aesthetically preferable.

Done, split line after ":not("

[dzhioev (left Google), 2015-05-11 23:06:06]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-05-11 23:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
michaelpg@chromium.org, glevin@chromium.org
Link to the patchset: https://codereview.chromium.org/1133733003/#ps120001
(title: "Rebased onto ToT.")

[commit-bot: I haz the power, 2015-05-11 23:08:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133733003/120001

[commit-bot: I haz the power, 2015-05-11 23:57:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-11 23:58:32]

Patchset 7 (id:??) landed as
https://crrev.com/38e0418e79bd5b0f3e2e86beab7d6c60970532aa
Cr-Commit-Position: refs/heads/master@{#329284}