Layout nits for the signin email confirmation dialog.

Layout nits for the signin email confirmation dialog.

This CL does the following layout changes for the sign-in email confirmation
dialog:
1. Expands the dialog width from 448px > 512px to accommodate text-length
2. Change the height og the title bar to have a 16px padding at the top and
at the bottom.
3. Change title line-height to 22px
4. Chaneg the title font color to: #333333
5. Change radio_off color: #757575 (this is google-grey-600)

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

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

[msarda, 2017-01-10 14:47:43]

Description was changed from

==========
Layout nits for the signin email confirmation dialog.

This CL does the following layout changes for the sign-in email confirmation
dialog:
1. Expands the dialog width from 448px > 512px to accommodate text-length
2. Change the height og the title bar to have a 16px padding at the top and
at the bottom.
3. Change title line-height to 22px
4. Chaneg the title font color to: #333333
5. Change radio_off color: #757575 (this is google-grey-600)

BUG=667665
==========

to

==========
Layout nits for the signin email confirmation dialog.

This CL does the following layout changes for the sign-in email confirmation
dialog:
1. Expands the dialog width from 448px > 512px to accommodate text-length
2. Change the height og the title bar to have a 16px padding at the top and
at the bottom.
3. Change title line-height to 22px
4. Chaneg the title font color to: #333333
5. Change radio_off color: #757575 (this is google-grey-600)

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

[msarda, 2017-01-10 14:51:48]

msarda@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[msarda, 2017-01-10 14:51:49]

Please take a look.

[Dan Beam, 2017-01-10 18:35:15]

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

[Dan Beam, 2017-01-10 18:35:16]

https://codereview.chromium.org/2625663003/diff/1/chrome/browser/resources/si...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
(right):

https://codereview.chromium.org/2625663003/diff/1/chrome/browser/resources/si...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:64:
$i18n{signinEmailConfirmationCreateProfileButtonTitle}
if you wrap these $i18n{} in elements, you can configure their line-height.

https://jsfiddle.net/rtvqztbr/

when it's just text, the internal styles of <paper-radio-button> win

[anthonyvd, 2017-01-10 20:35:12]

lgtm

[msarda, 2017-01-11 18:09:59]

Hey Dan,

May I ask you to do an owners review for chrome/browser/resources/signin/?

Thank you.

[Dan Beam, 2017-01-11 18:56:02]

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:18:
margin-left: -8px;
should this be -webkit-margin-start: -8px; so it flips to the right side in RTL?
or also include margin-right: -8px as well?

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:20:
}
this could be expressed more shortly as 

padding: 8px 24px 0;

as a reviewer this is nice because this is: 

top left-and-right bottom

which I know is always going to work well in RTL

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:25:
that is very large*/
/* CSS comments
 * are typically formatted
 * like this. */

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js:14:
$('createNewUserRadioButtonTitle').innerHTML = loadTimeData.getStringF(
could this be .textContent instead?

[msarda, 2017-01-12 16:11:08]

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:18:
margin-left: -8px;
On 2017/01/11 18:56:02, Dan Beam wrote:
> should this be -webkit-margin-start: -8px; so it flips to the right side in
RTL?
> or also include margin-right: -8px as well?

Done.

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:20:
}
On 2017/01/11 18:56:02, Dan Beam wrote:
> this could be expressed more shortly as 
> 
> padding: 8px 24px 0;
> 
> as a reviewer this is nice because this is: 
> 
> top left-and-right bottom
> 
> which I know is always going to work well in RTL

Done.

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:25:
that is very large*/
On 2017/01/11 18:56:02, Dan Beam wrote:
> /* CSS comments
>  * are typically formatted
>  * like this. */

Done.

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js:14:
$('createNewUserRadioButtonTitle').innerHTML = loadTimeData.getStringF(
On 2017/01/11 18:56:02, Dan Beam wrote:
> could this be .textContent instead?

Not, this cannot be .textContent as this contains HTML (the value of the string
is "<strong>This wasn't me.</strong> Create a new profile for xxx@gmail.com")

[Dan Beam, 2017-01-12 18:32:59]

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js:14:
$('createNewUserRadioButtonTitle').innerHTML = loadTimeData.getStringF(
On 2017/01/12 16:11:07, msarda wrote:
> On 2017/01/11 18:56:02, Dan Beam wrote:
> > could this be .textContent instead?
> 
> Not, this cannot be .textContent as this contains HTML (the value of the
string
> is "<strong>This wasn't me.</strong> Create a new profile for
mailto:xxx@gmail.com")

please use I18nBehavior and i18n() or i18nAdvanced() instead then (or
parseHtmlSubset directly). innerHTML can cause security issues. I just reviewed
a sandbox bypass fix where innerHTML was part of the exploit chain.

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
(right):

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:19:
-webkit-margin-start: -8px;
this should go at the top of this rule

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:42:
height: auto;
please alphabetize these rules

[msarda, 2017-01-13 09:52:03]

Patchset #5 (id:80001) has been deleted

[msarda, 2017-01-13 09:54:16]

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js
(right):

https://codereview.chromium.org/2625663003/diff/40001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.js:14:
$('createNewUserRadioButtonTitle').innerHTML = loadTimeData.getStringF(
On 2017/01/12 18:32:58, Dan Beam wrote:
> On 2017/01/12 16:11:07, msarda wrote:
> > On 2017/01/11 18:56:02, Dan Beam wrote:
> > > could this be .textContent instead?
> > 
> > Not, this cannot be .textContent as this contains HTML (the value of the
> string
> > is "<strong>This wasn't me.</strong> Create a new profile for
> mailto:xxx@gmail.com")
> 
> please use I18nBehavior and i18n() or i18nAdvanced() instead then (or
> parseHtmlSubset directly). innerHTML can cause security issues. I just
reviewed
> a sandbox bypass fix where innerHTML was part of the exploit chain.

Done.

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
(right):

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:19:
-webkit-margin-start: -8px;
On 2017/01/12 18:32:58, Dan Beam wrote:
> this should go at the top of this rule

Done.

https://codereview.chromium.org/2625663003/diff/60001/chrome/browser/resource...
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:42:
height: auto;
On 2017/01/12 18:32:58, Dan Beam wrote:
> please alphabetize these rules

Done (I also alphabetize the classes).

[Dan Beam, 2017-01-13 17:08:54]

lgtm

[msarda, 2017-01-13 19:48:44]

The CQ bit was checked by msarda@chromium.org

[msarda, 2017-01-13 19:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2625663003/#ps100001
(title: "Nits and use I18nBehavior.i18n")

[commit-bot: I haz the power, 2017-01-13 19:49:31]

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-13 21:03:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484336924838640,
"parent_rev": "bca99cd5304a9e2a6fb249d7d3c352854752cd69", "commit_rev":
"a0b7a374a98a7aa4902e922ad7b7279ade10c3da"}

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

Description was changed from

==========
Layout nits for the signin email confirmation dialog.

This CL does the following layout changes for the sign-in email confirmation
dialog:
1. Expands the dialog width from 448px > 512px to accommodate text-length
2. Change the height og the title bar to have a 16px padding at the top and
at the bottom.
3. Change title line-height to 22px
4. Chaneg the title font color to: #333333
5. Change radio_off color: #757575 (this is google-grey-600)

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

to

==========
Layout nits for the signin email confirmation dialog.

This CL does the following layout changes for the sign-in email confirmation
dialog:
1. Expands the dialog width from 448px > 512px to accommodate text-length
2. Change the height og the title bar to have a 16px padding at the top and
at the bottom.
3. Change title line-height to 22px
4. Chaneg the title font color to: #333333
5. Change radio_off color: #757575 (this is google-grey-600)

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

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

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

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a0b7a374a98a7aa4902e922ad7b7...