Closed Bug 972061 Opened 6 years ago Closed 6 years ago

Update style of Sync intro and manage pages

Categories

(Firefox :: Sync, defect, P2)

x86
All
defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 --- verified

People

(Reporter: zaach, Assigned: zaach)

References

Details

Attachments

(1 file, 4 obsolete files)

This will update the Sync intro and manage pages in about:accounts with the latest styles from jgruen.
Attachment #8375159 - Flags: review?(gavin.sharp)
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?
Attachment #8375159 - Flags: review?(gavin.sharp)
Sorry, the last chunk/comment is referring to:

> -@-webkit-keyframes spin {
> -  from {
> -    -webkit-transform: rotate(0deg);
> +@  from {
>    }
>    to {
> -    -webkit-transform: rotate(365deg);
>    }
>  }
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.
Flags: needinfo?(jgruen)
Remove unnecessary prefixes and fragments; fix indentation.
Attachment #8375159 - Attachment is obsolete: true
Attachment #8375260 - Flags: review?(ttaubert)
Flags: needinfo?(jgruen)
Duplicate of this bug: 965482
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.
Attachment #8375260 - Flags: review?(ttaubert) → feedback+
(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?
(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.
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.
Attachment #8375260 - Attachment is obsolete: true
Attachment #8377935 - Flags: review?(ttaubert)
Attached patch slight tweak to top margin (obsolete) — Splinter Review
A slight tweak to the top margin to accommodate smaller screens.
Attachment #8377935 - Attachment is obsolete: true
Attachment #8377935 - Flags: review?(ttaubert)
Attachment #8381107 - Flags: review?(ttaubert)
Priority: -- → P2
Duplicate of this bug: 976588
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.
Attachment #8381107 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
New and improved patch with jgruen's super selective style sheet.
Attachment #8381107 - Attachment is obsolete: true
Attachment #8390929 - Flags: review?(ttaubert)
   ______                _       __
  / ____/______  _______(_)___ _/ /
 / /   / ___/ / / / ___/ / __ `/ / 
/ /___/ /  / /_/ / /__/ / /_/ / /  
\____/_/   \__,_/\___/_/\__,_/_/
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!

_______     .__     
\_  __ \  __|  |___ 
 |  | \/ /__    __/ 
 |__|       |__|
Attachment #8390929 - Flags: review?(ttaubert) → review+
Attachment #8390929 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1bea2a295c8a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: twalker
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
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.