992388 - Restyle FxA verification emails errors/success alert and migration prompt in sync preferences

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Chris Karlof [:ckarlof]]

The relevant code is in browser/components/preferences/sync.js and browser/components/preferences/in-content/sync.js. On success, the browser shows a prompt indicating success. On failure, it does nothing. 

It could fail for a variety of reasons, e.g., 

1) lack of network
2) various server errors, such as authentication errors and rate limiting

This probably deserves a better treatment, but for the Fx29 timeframe, we could just trigger the success prompt on all errors. I consider the resend button to be a bit of a "close door" button on the elevator anyway.

Comment 1

[Chris Karlof [:ckarlof]]

What do you think :markh?

Comment 2

[Chris Karlof [:ckarlof]]

We can do a proper error message for Fx31, but we need copy. For Fx29/30, we can leave it as is, or just show the success message on error. Right now it does nothing. Ryan, John, thoughts?

Comment 3

[John Gruen [:jgruen]]

Throwing the success box will provide a direct affordance (even if it's misleading). Probably a reasonable fix for 29. 

For 31+ we can use something from this here (https://www.dropbox.com/s/6aoi7vk4jqn4v60/400%20and%20500%20Errors.pdf) as as a starting point.

Comment 4

[John Gruen [:jgruen]]

Will follow up with Matej on Monday.

Comment 5

[Matej Novak [:matej]]

(In reply to John Gruen from comment #4)
> Will follow up with Matej on Monday.

I'm on PTO Monday and Tuesday, but feel free to ping me today.

Comment 6

[Chris Karlof [:ckarlof]]

It's too late for changes for Fx29, but we can target Fx30 or Fx31.

Comment 7

[Ryan Feeley [:rfeeley]]

I updated the Email Sent screen to include the potential error messages: https://www.dropbox.com/s/eith1ey3i8f7295/Email%20Sent.pdf

Comment 8

[Chris Karlof [:ckarlof]]

This needs string changes.

Comment 9

[Chris Karlof [:ckarlof]]

This needs string changes, so let's get the strings defined before Fx31 moves to aurora.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Chris, if you want to see that bugs fixed in 31, this will have to land soon (we merge in 3 weeks).

Comment 11

[Chris Karlof [:ckarlof]]

Given that our email verification rate is fairly high, I don't see any immediate evidence this is causing problems. It's a poor experience, but it doesn't need to be on 31.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

The migration code *does* handle errors [1], so we actually do have strings for this [2] - so it would actually be trivial to fix. Chris, given this, do you want to change the priority?

[1] https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/FxaMigrator.jsm#507
[2] https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/accounts.properties#47

Comment 13

[Chris Karlof [:ckarlof]]

I'm happy at P2. There's another sharp edge to address here. If a user's email address hard bounces, then we delete her account in our server. It would be nice to provide better treatment here because I believe it's a little difficult to subsequently re-signup with deliverable email.

Comment 14

[Edouard Oger [:eoger]]

Attachment: Status messages proposal.gif

I hacked something this afternoon, what do you think?
We would of course change the strings to have all the information in one sentence.

Comment 15

[Ryan Feeley [:rfeeley]]

Looks great! Michael Maslaney did some visual design specs for this, although we would use the system font not Fira Sans. https://bugzilla.mozilla.org/show_bug.cgi?id=1091815

I like the close X for the notification.

Are you using existing colours or should I get them from Michael or Stephen Horlander?

Comment 16

[Edouard Oger [:eoger]]

Attachment: bug_992388_screenshots.zip

After watching the screenshots rfeeley provided, I took the initiative to also restyle the migration prompts. Here are the screenshots.

What else needs to be done Ryan?
I think the |verif_mail_ok| background color looks weird.

Comment 17

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Here's my current working tree

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8646719 [details] [diff] [review]
bug-992388.patch

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

Looks great!

::: browser/components/preferences/in-content/sync.js
@@ +445,5 @@
>  
>          // The "Learn more" link.
>          if (!email) {
>            let learnMoreLink = document.createElement("label");
> +          learnMoreLink.id = "learnMoreLink";

is there a reason we can't have this in the .xul and just toggle its display value?

@@ +665,5 @@
>        });
>    },
>  
>    verifyFirefoxAccount: function() {
> +    let changesyncStatusMessage = (data, isError = false) => {

ISTM it would be safe to assume that !data means isError = true;

@@ +688,5 @@
> +
> +    let onSuccess = data => {
> +      if (data) {
> +        changesyncStatusMessage(data);
> +      }

please cuddle the else on this line

::: browser/components/preferences/in-content/sync.xul
@@ +55,5 @@
>          <button id="sync-migrate-resend"/>
>        </hbox>
>      </deck>
>    </vbox>
> +  <button id="syncStatusMessageClose">✖</button>

Is this unicode string used elsewhere? I've never seen it used before, so if it is used elsewhere, that's fine, but if it's not we should find something else similar and copy that.

::: browser/locales/en-US/chrome/browser/accounts.properties
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +updateToFxa.title = Update

This string seems a little too "terse" - please check with Ryan that he's happy with that (he can see it in migration1.png in the .zip file)

@@ +44,5 @@
>  verificationSentTitle = Verification Sent
>  # LOCALIZATION NOTE (verificationSentHeading) - %S = Email address of user's Firefox Account
>  verificationSentHeading = A verification link has been sent to %S
>  verificationSentDescription = Please check your email and click the link to begin syncing.
> +verificationSentFull = A verification link has been sent to %S. Please check your email and click the link to begin syncing.

please copy the LOCALIZATION NOTE (changing the entity name) from above for this string.

Comment 19

[Ryan Feeley [:rfeeley]]

Hey hey Michael: your in-content prefs notification styles are spreading! Check out bug_992388_screenshots.zip screenshots above and let us know what HEX to use for red and green colours (and text colour white?).

Comment 20

[Michael Maslaney [:mmaslaney] ([e-mail])]

Nice!

Please find the color palette here: 
http://people.mozilla.org/~jgruen/chameleon/old/#nav5

Comment 21

[Edouard Oger [:eoger]]

Attachment: new_screenshots.zip

New colors screenshots

Comment 22

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Updated patch

Comment 23

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #21)
> Created attachment 8647234 [details]
> new_screenshots.zip
> 
> New colors screenshots

Note that the 3.55.24 screenshot is showing a successful resend of the verification mail, but the text is missing the email address (ie, it says "A verification link has been sent to . Please check your email..."

Comment 24

[Edouard Oger [:eoger]]

I was too lazy to wait for the fxa-auth-server send email unban, so I triggered the success message myself. No worries!

Comment 25

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #24)
> I was too lazy to wait for the fxa-auth-server send email unban, so I
> triggered the success message myself. No worries!

Fair enough :)

Comment 26

[Chris Karlof [:ckarlof]]

Is this blocked by the mmaslaney question? Ryan, can you help?

Comment 27

[Ryan Feeley [:rfeeley]]

"Learn more" link should be white and underlined, instead of blue on orange. Otherwise matching.

Comment 28

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Ready for review!

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8652471 [details] [diff] [review]
bug-992388.patch

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

Looks good, but I think we need to sort out that close button before landing.

::: browser/components/preferences/in-content/sync.xul
@@ +55,5 @@
>          <button id="sync-migrate-resend"/>
>        </hbox>
>      </deck>
>    </vbox>
> +  <button id="syncStatusMessageClose">✖</button>

As I mentioned in comment 18, I'm not sure this is the way to do this - I'd expect a close-icon class like most other close buttons have.

Comment 30

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Updated with close-icon and a bug fix related to the learnMoreLink.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ad328359d97

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_bug731866.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=4466630&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/59062d51df42

Comment 33

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Corrected patch for tests.

Comment 34

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a33e9cd8586

Comment 35

[Edouard Oger [:eoger]]

Comment on attachment 8655119 [details] [diff] [review]
bug-992388.patch

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

Bad patch, display problems.

Comment 36

[Edouard Oger [:eoger]]

Attachment: bug-992388.patch

Removed the animations (side-effect of changing how we display the error message to pass tests :/)

Comment 37

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cecdb2418d75

Comment 38

[Chris Karlof [:ckarlof]]

Markh will pick this up to add back in the animations in the next iteration.

Comment 39

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8654253 [details] [diff] [review]
bug-992388.patch

I think we should run with this version and fix the test.

Comment 40

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-992388-part-2-have-visibility-collapse-be-treate.patch

Jaws,
 The part1 of this bug is adding a hbox to the Sync prefs pane which is hidden using |visibility: collapse;|, but this causes browser_bug731866.js to fail as it only checks for visibility="hidden". We are doing it this way to get a pretty opacity transition and it looks fine in all our testing (ie, it is only visible when it should be).

The one-line fix for this is:

function is_hidden(aElement) {
...
-  if (style.visibility != "visible")
+  if (style.visibility != "visible" && style.visibility != "collapse")
     return true;

but I thought I better check there isn't something I'm missing! I've also changed the test to report the element ID that is failing, but that's obviously not part of the fix but makes tracking failures easier.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8656942 [details] [diff] [review]
0002-Bug-992388-part-2-have-visibility-collapse-be-treate.patch

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

Yeah, I'm OK with this, but note that visibility:collapse; still allows margins to apply for XUL elements.

Do you have margins on your hbox? Due to this, maybe is_element_hidden should take an extra argument that allows visibility:collapse; to be considered as hidden? I'll let you use your judgement here. r=me

Comment 42

[Mark Hammond [:markh] [:mhammond]]

Thanks Jaws! While we don't have margins, on reflection I'm not that happy with this change, and it can be avoided simply by adding a "container" with the data-category attribute which seems cleaner - so I think I'll do that instead.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cc9866c6053

Comment 43

[Pulsebot]

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

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdd344bc5aef