972061 - Update style of Sync intro and manage pages

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Zachary Carter [:zaach]]

This will update the Sync intro and manage pages in about:accounts with the latest styles from jgruen.

Comment 1

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8375159 [details] [diff] [review]
0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Review of attachment 8375159 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing from Gavin, he's a busy man. I skimmed this quickly, can you please fix indentation everywhere and remove -webkit, -o, and -moz prefixes? We can of course keep -moz prefixes where we need them but there is no need for backwards compatibility as we target Fx 29.

Also, this unfortunately didn't apply to fx-team tip, is your tree up-to-date? Is that based off another tree?

Do we have a mockup somewhere that you could attach to this bug? That I could compare your patch to?

::: browser/base/content/aboutaccounts/aboutaccounts.css
@@ +37,5 @@
> +  margin:0 auto;
> +  overflow:hidden;
> +  text-indent: 100%;
> +  white-space: nowrap;
> +  width:150px;

Nit: please put a space after the colon.

@@ +46,5 @@
>  }
>  
>  /* Retina */
>  @media
> +only screen and (-webkit-min-device-pixel-ratio: 2),

This is an internal page so I don't think supporting Opera and WebKit makes any sense :)

@@ +51,2 @@
>  only screen and (   min--moz-device-pixel-ratio: 2),
>  only screen and (   -moz-min-device-pixel-ratio: 2),

Same here, we target Fx 29 so do we need those two at all?

::: browser/base/content/aboutaccounts/main.css
@@ +2,5 @@
> +*,
> +*:before,
> +*:after {
> +     -moz-box-sizing: border-box;
> +          box-sizing: border-box;

We should only need one of those, no? And the indentation is a little off, too.

@@ +45,5 @@
> +}
> +
> +#fxa-signup-header.fox-logo {
> +     -moz-animation: movein 0.5s;
> +          animation: movein 0.5s;

We don't need the -moz rule and the indentation is off.

@@ +52,3 @@
>  #stage {
> +     -moz-animation: fadein 0.5s;
> +          animation: fadein 0.5s;

We don't need the -moz rule and the indentation is off.

@@ +65,5 @@
> +@    from { opacity: 0; }
> +    to   { opacity: 1; }
> +}
> +
> +@-moz-keyframes fadein {

Do we need this with the -moz prefix?

@@ +123,5 @@
> +  padding: 0;
> +}
> +
> +#legal-header h3 {
> +    font-size: 12px;

Nit: indentation off.

@@ +233,5 @@
> +}
> +
> +.tooltip-below[title]:after,
> +.tooltip-below[title]::after {
> +  top:  top:    -moz-calc(102% + 2px);

This shouldn't work and is overriden below?

@@ +244,5 @@
> +.input-row input[type='password'],
> +.input-row select {
> +  border: 1px solid #8A9BA8;
> +          border-radius: 5px;
> +          box-shadow: inset 0 1px 1px #f2f2f2;

Nit: indentation.

@@ +619,1 @@
>    }

So what's this used for?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Sorry, the last chunk/comment is referring to:

> -@-webkit-keyframes spin {
> -  from {
> -    -webkit-transform: rotate(0deg);
> +@  from {
>    }
>    to {
> -    -webkit-transform: rotate(365deg);
>    }
>  }

Comment 4

[Zachary Carter [:zaach]]

Thanks Tim; this was against mc. The mockups seem to be out of date or missing from the UX Desktop PDF: http://is.gd/Sync_FxA_Latest_Desktop_UX_PDF. Here are the closests: https://www.dropbox.com/s/6g9fnusqq3j2rqu/Sync.FxA%20Intro.pdf and https://www.dropbox.com/s/xcf1jyfur5ev59u/Sync.FxA%20Manage.pdf. It looks like just the splash image has been updated.

Most of the CSS is taken verbatim from the the ones we use on the remote pages, though I did try to strip some of the -webkit prefixes. I'll try to remove things we obviously don't need. For deeper questions about the CSS I'll defer to John.

Comment 5

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Remove unnecessary prefixes and fragments; fix indentation.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8375260 [details] [diff] [review]
0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Review of attachment 8375260 [details] [diff] [review]:
-----------------------------------------------------------------

Giving it another look, why is there so much space at the bottom of the intro page? It feels weird that I can see the whole div but we still show scroll bars.

Also, can we preload "Fira Sans" and the Firefox logo in the background when showing the intro page? Without the cached version of those files the transition to the next page didn't look very smooth, I started out with an empty header and didn't see any animation. I couldn't reproduce that with an empty profile again, btw. Not sure why it gave me such a bad experience the first time.
(I would be totally fine with this being a follow-up.)

Other than that, it looks good to me. I see no glitches and it matches the mockups. There are *a lot* of rules that we don't need as they are only used by the page loaded in the iframe.

::: browser/base/content/aboutaccounts/main.css
@@ +31,5 @@
>    display: block;
>    margin: 160px 0 0 0;
>  }
>  
> +.fox-logo {

The fox logo is coming from the in-content page, we don't have anything like this. I think we can remove this.

@@ +42,5 @@
> +  width: 96px;
> +  z-index: 999;
> +}
> +
> +#fxa-signup-header.fox-logo {

Same here.

@@ +63,5 @@
> +  from { opacity: 0; }
> +  to   { opacity: 1; }
> +}
> +
> +@keyframes movein {

This is only used by the .fox-logo and can be removed.

@@ +96,5 @@
>    font-weight: 200;
> +  margin: 20px 0 32px 0;
> +}
> +
> +#legal-header h2 {

Please remove all the #legal-* stuff, about:accounts doesn't have that.

@@ +123,5 @@
> +#legal-copy p {
> +  font-size: 14px;
> +}
> +
> +strong.email {

I see no <strong>s, this can be removed.

@@ +128,5 @@
> +  display: block;
> +}
> +
> +.error,
> +.success {

Please remove all .error/.success rules, form input is validated by the iframe.

@@ +155,5 @@
>    white-space: nowrap;
>    width: 150px;
>  }
>  
>  .graphic-checkbox {

This is used by the iframe, for the verification step. Can be removed.

@@ +161,3 @@
>  }
>  
>  .graphic-mail {

Same here.

@@ +166,3 @@
>  }
>  
>  .input-row {

We have no inputs, please remove this.

@@ +169,5 @@
>    margin-bottom: 15px;
>    width: 100%;
>  }
>  
> +.tooltip {

We have no tooltips for form validation, please remove those rules.

@@ +392,5 @@
>    margin-bottom: 20px;
>    overflow: auto;
>  }
>  
> +ul.links {

We have no <ul class="links">, please remove this.

@@ +419,5 @@
>    float: right;
>  }
>  
> +.privacy-links,
> +.privacy-links a {

Please remove those as well, this link is in the iframe.

@@ +440,1 @@
>  .verification-email-message {

And this.

@@ +440,5 @@
>  .verification-email-message {
>    word-wrap: break-word;
>  }
>  
>  .password-row {

And this and the rules below.

@@ +494,5 @@
> + * give it an absolute position and set its opacity to 0. Whenever tabbed to
> + * using the keyboard, the label acts as if the mouse were hovering over the
> + * label.
> + */
> +.show-password {

And this.

@@ +548,5 @@
>  .cannot-create-account-content {
>    margin-top: 105px;
>  }
>  
>  .spinner {

We have no spinner, please remove.

@@ +584,5 @@
>    html {
>      background: #fff;
>    }
>  
> +  .fox-logo {

Please remove this as well.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> Also, can we preload "Fira Sans" and the Firefox logo in the background when
> showing the intro page? Without the cached version of those files the
> transition to the next page didn't look very smooth, I started out with an
> empty header and didn't see any animation.

I just realized that this is iframe content, can we preload the iframe? Do we do that already?

Comment 9

[Zachary Carter [:zaach]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > Also, can we preload "Fira Sans" and the Firefox logo in the background when
> > showing the intro page? Without the cached version of those files the
> > transition to the next page didn't look very smooth, I started out with an
> > empty header and didn't see any animation.
> 
> I just realized that this is iframe content, can we preload the iframe? Do
> we do that already?

We do that already: the frame loads immediately and pressing the button just reveals the frame.

Comment 10

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Manually culling the rules on subsequent style updates will be error-prone. We should discuss with the designers alternative ways to organize the rules when that time comes.

Comment 11

[Zachary Carter [:zaach]]

Attachment: slight tweak to top margin

A slight tweak to the top margin to accommodate smaller screens.

Comment 13

[Chris Karlof [:ckarlof]]

Bug 976588 has an attachment showing the visual diff: https://bugzilla.mozilla.org/attachment.cgi?id=8381450

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8381107 [details] [diff] [review]
slight tweak to top margin

Review of attachment 8381107 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Zachary Carter [:zaach] from comment #10)
> Manually culling the rules on subsequent style updates will be error-prone.
> We should discuss with the designers alternative ways to organize the rules
> when that time comes.

I understand your frustration here but on desktop we usually avoid introducing superfluous code. I would not have objected a few rules but there were *a lot* of unused rules in the CSS file. I really think we should have a shared.css file or the like that we can just use on desktop and on the web.

r=me with the <h1> question below addressed.

::: browser/base/content/aboutaccounts/main.css
@@ +72,1 @@
>  }

Do we really want to to hide the h1? Can't we make the <h2> the new <h1>? The current h1 is duplicated in the document title anyway.

Comment 15

[Zachary Carter [:zaach]]

Attachment: 0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

New and improved patch with jgruen's super selective style sheet.

Comment 16

[John Gruen [:jgruen]]

   ______                _       __
  / ____/______  _______(_)___ _/ /
 / /   / ___/ / / / ___/ / __ `/ / 
/ /___/ /  / /_/ / /__/ / /_/ / /  
\____/_/   \__,_/\___/_/\__,_/_/

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8390929 [details] [diff] [review]
0001-Bug-972061-Update-style-of-Sync-intro-and-manage-pag.patch

Review of attachment 8390929 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the cleanup!

_______     .__     
\_  __ \  __|  |___ 
 |  | \/ /__    __/ 
 |__|       |__|

Comment 18

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/fx-team/rev/1bea2a295c8a

Comment 19

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dfd3e578df39

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1bea2a295c8a

Comment 21

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29 RC
Build Id: 20140421221237
OS:Win 7 x64, Mac Os x 10.9, Ubuntu 14.04 x64

Comment 22

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 30 Aurora
Build Id: 20140428004000
OS:Win 7 x64, Mac Os x 10.9, Ubuntu 13.04 x64