Closed Bug 516350 Opened 12 years ago Closed 12 years ago

about:weave round 3


(Firefox :: Sync, defect, P1)






(Reporter: hello, Assigned: hello)



(2 files, 1 obsolete file)

Attached patch v1Splinter Review
3rd pass for about:weave.

v1 of patch has:

* revamped setup screens, more similar to wizard flow now
* setup tries to pick up where you left off if you close about:weave/restart the browser
* menus, account info bubble, link to log
* don't use Museo font, it's ginormous
Attachment #400451 - Flags: review?(edilee)
Attached patch v1 u8 (no binary parts) (obsolete) — Splinter Review
Imported then re-exported with -u8 stripping out binary parts. (Original patch didn't remove Museo fonts though)
Assignee: nobody → thunder
Various highish usage notes:

- one time when upgrading from previous version, I ended up on the Data pane
-- "prev" went to "enter passphrase" and "next" went to "syncing soon"

- one time loading about:weave showed empty sign into form (autologin false)
-- top box showed "signed out" with a "sign out" choice
-- clicking "sign out" resulted in the sign into form but was now populated

- when signed in, there's a box "Signed in as <username>" with sign out/my acc.
-- what else should be clickable? (other than Tools) nothing right now
-- "My Account" only shows <username> and ****s, which isn't any more info yet you can edit the password/passphrase boxes then hit close to do nothing ?

- small width causes top/right boxes to get pushed down and can't be clicked (probably pushing against the weave logo in the top/left)

- signing in from another about:weave page causes top/right status to update but the "sign into" bubble remains

- when signing in for the first time, errors alert but continue to next pane (Data)
-- "prev" leads to "enter phassphrase" and "next" is done
-- top box shows "Signed in as <bad username>" on done page

- signing in for first time correctly allows "prev" to "account created -> enter passphrase"
-- while on the Data pane, the top box still says "Signed out" while status bar shows the username already

- signing in with a failure results in alert and the signin bubble closes

- create account help bubble looks odd with old content (passphrase)

- should the captcha input have a text label Capcha? right now the input is centered while everything else is pushed right

- can't tab to the agree "checkbox"

- clicking next for account creation triggers a login (status bar shows username).. perhaps it should wait until the user completes the next page of entering a passphrase

- refreshing about:weave before setting a passphrase triggers a log-in then loads "secret phrase" bubble after a few seconds.. perhaps it should just check if a passphrase is set before logging in and then show "secret phrase" then login on "next". This would only happen for a new account (which was already created) because existing users enter both password and passphrase before signing in

- can accidentally double click "next" from account creation:
2009-09-15 12:49:43	Net.Resource         DEBUG	PUT Length: 396
2009-09-15 12:49:43	Net.Resource         DEBUG	PUT Length: 396
2009-09-15 12:49:43	Net.Resource         DEBUG	PUT request for
2009-09-15 12:49:43	Net.Resource         DEBUG	PUT request for
2009-09-15 12:49:43	Net.Resource         DEBUG	PUT fail: 400
2009-09-15 12:49:43	About:Weave          WARN	Account creation error: invalid-captcha
2009-09-15 12:49:45	Net.Resource         DEBUG	PUT success: 200
this results in an alert but then loads the "secret phrase" bubble

- "You're all set! The sync process will begin shortly, please be patient while synchronizes your data."
.. be patient while *Weave* synchronizes ..

- "You can close this tab now." with a "close" button below, which only closes the bubble and not the tab.
Ok, I saw the mockups, so things kinda make more sense now ;)

The "my account" bubble, which currently shows username, ***, ****; will eventually have change password/change secret stuff, so that'll be more useful. Still not sure if there's a use for showing the hidden password.

Clicking the device probably won't do anything for now.. but the cloud will eventually show other clients and stats. Partial download stats probably goes better with the local client though.. ? and a way to get back to "Data"?

Account creation bubble is kinda tall, and I need to scroll on my 13" macbook. Would it be bad to have the bubble hide the broken arrow.. or just shift everything up?
Initially landed the patch v1 on mconnor's about-weave branch:
Reduce calls to login() to account creation (after passphrase) and regular sign-in. Use event handlers to follow up after logging in. Wrap login/createAccount calls with disabling "next" and activiting the throbber. 

This fixes a number of issues and removes/cleans up the code that calls login and simplifies other logic to determine what initial bubble to show.
I explicitly did not use DTDs, and do not want this change:

Otherwise we should really land this ASAP, as we're already late for the pre3 release.  What are the open issues preventing it from landing?
We should discuss that decision, and all of the implications therein, before we lock into anything.  I think it creates more problems than it solves.  If the final decision is to not use DTDs, I'll fix that for the next milestone, but I don't think it makes sense to delay pre3 or ship without change password/passphrase exposed.
I made a diff of only changes on about-weave branch not including changes on mainline weave..

hg clone weave super; cd super
hg pull ../about-weave/
hg merge -r 5a066d7c327e
hg commit -m 'Merge initial about:weave + conflict resolution to weave.'
hg merge
hg commit -m 'Merge about-weave to intermediate initial landing.'
Attachment #400551 - Attachment is obsolete: true
Attachment #401258 - Flags: review?(thunder)
Comment on attachment 401258 [details] [diff] [review]
super diff since v1

>   init: function init() {
>     About._log = Log4Moz.repository.getLogger("About:Weave");
>     About._log.trace("Loading About:Weave");
>     About.refreshClientType();
>     About.hideQuota();
>     About._localizePage();
>+    About._installObservers();

Observers should only be installed after setup completes.  The connection arrow should remain in the "broken" state until setup is complete.

Note that as we discussed earlier, we can grandfather in our current users, and consider an upgrade from 0.6 + existing username/password/passphrase to = setupComplete.

>+    // In the common case, the user is already logged in, so show the status
>+    if (Weave.Service.isLoggedIn) {

If setup hasn't been completed, we should have them finish it.  Part of setup includes logging the user in; if I complete half of the setup and then restart the browser I want it to resume where it left off.

>+    // Start from the beginning if there's no user to sign-in
>+    else if (!Weave.Service.username)
>+      About.showBubble("welcome");
>+    // Username is set but no password saved, so do a regular sign-in
>+    else if (!Weave.Service.password)
>+      About.showBubble("signin");
>+    // User didn't finish setting a passphrase, so show the secret description
>+    else if (!Weave.Service.passphrase)
>+      About.showBubble("newacct2");
>+    // Got all the pieces for sign-in, so show the bubble with populated fields
>+    else
>+      About.showBubble("signin");

After setup is complete, I don't think we should point users to newacct2, as that bubble should tell the user they successfully created an account, and we need them to enter a *new* passphrase now.

If we have a password but not a passphrase saved, it makes sense prompt the user for their *existing* passphrase, unless they have no keys on the server.  That is partly why my version did login(), I wanted to check whether keys were there (though I didn't actually check).

For now, it could be acceptable to just show the regular login form if there's no passphrase saved.

>   onLoginFinish: function onLoginFinish() {
>     About.setStatus('idle');
>+    // Save login settings on success
>+    Weave.Service.persistLogin();
>+    // Nothing left to do, so just hide the form
>+    if (About.setupComplete)
>+      About.hideBubble();
>+    // First successful sign-in shows the data configuration
>+    else
>+      About.showBubble("data");

>   },
>   onLoginError: function onLoginError() {
>     About.setStatus('offline');
>-    // fixme?
>+    // Show the full sign-in form to try again
>+    About.showBubble("signin");
>+    alert("Couldn't sign in: " + Weave.Utils.getErrorString(
>+      Weave.Service.status.login)); //FIXME

This isn't right, imo.  If sign in fails after account creation, for example, we shouldn't show the sign in dialog.

>   onBubble_signin: function() {
>     // next/sign in button gets disabled until onSigninInput() enables it
>     $('#signin .buttons .next').attr('disabled', true);
>-    if (!About.setupComplete) {
>-      About.resetLogin();

Removing this means that after any login attempt (maybe an unsuccessful one), Firefox can prefill the form values and we won't clear them.  I think we should.

Remember that we can automatically set setupComplete for users upgrading from 0.6.

>+    // Previously logged in user, so show "sign in"
>+    if (user) {
>+      $("#signin-username").val(user);
>+      $("#signin .buttons .next").val("sign in"); // fixme: l10n
>+      $("#signin .buttons .prev").hide();
>+    }
>+    // No username means we might need to setup data or create account
>+    else {
>+      $("#signin .buttons .next").val("next"); // fixme: l10n
>+      $("#signin .buttons .prev").show();
>+      pass = passph = "";
>+    }

I think these need to be keyed on whether setup has been completed, rather than whether the username is set.  If setup is complete, we should show the regular login dialog (no prev button, next is 'sign in').

>+  doWrappedFor: function doWrappedFor(bubble, func /*, args */) {
>+    Weave.Service[func].apply(Weave.Service, Array.slice(arguments, 2));

I think you need to save & return the return value of Weave.Service[func] here.

>-    let ret = Weave.Service.createAccount($('#newacct-username').val(),
>-                                          $('#newacct-password').val(),
>-                                          $('#newacct-email').val(),
>-                                          $('#captcha-challenge').val(),
>-                                          $('#captcha-response').val());
>+    let username = $("#newacct-username").val();
>+    let password = $("#newacct-password").val();
>+    About.doWrappedFor("#newacct", "createAccount", username, password,
>+      $("#newacct-email").val(), $("#captcha-challenge").val(),
>+      $("#captcha-response").val());
>+    // User created successfully, so save the user/pass and move on
>     if (ret == null) {

ret is no longer defined

>-      Weave.Service.login($('#newacct-username').val(),
>-                          $('#newacct-password').val(),
>-                          $('#newacct-passphrase').val());

I think we should try to login.  We have had problems before with the server returning 200 but the account creation didn't really succeed... it's an odd case, but I don't see any reason not to make sure.

>-.logo {float: left;}
>+.logo {
>+    bottom: 0;
>+    left: 20px;
>+    opacity: .7;
>+    position: absolute;

Note that at some point we might want to use the space at the bottom for other stuff.  Aza had some ideas for stuff we could do here.

On mobile, we might want to use this space too, but we don't know/have mockups for that yet.

I don't have any big problems with this change, though.

> #top-menu > li {
>+    z-index: 2;

odd, I thought .menu-dropdown's z-index:1 was enough?  It works for me here.

>         <div id="newacct2" class="bubble">
>           <h1 id="newacct2-title">Secret Phrase</h1>
>-          <p>User creation successful!</p>

That string should be there.  We need to tell the user we successfully created the account.
Attachment #401258 - Flags: review?(thunder)
Attachment #400451 - Flags: review?(edilee) fixes most of my comments.  Closing this bug.
Closed: 12 years ago
Resolution: --- → FIXED
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.