Add timeout + error for users with managed accounts to sign in flow on Android |
|||||||||||||||||||||||||||||||||||||||||||
Issue descriptionVersion: 53.0.2785.125 OS: Android NRD91D on Nexus 9 What steps will reproduce the problem? (1) Never have run Chrome before (so FRE not run). (2) Join a network with a captive portal - in this case Emirates Airlines in-air WiFi, which is run by OnAir. (3) Launch Chrome in an attempt to sign into the WiFi network. What is the expected output? The FRE should not allow the user to get stuck due to limited or no connectivity. What do you see instead? The FRE runs and I see the screen to sign into my Google account. Selecting the account and clicking the "Continue" button, Chrome apparently freezes. There is no spinner or any other indication that something is happening. There is no way to get out of this state apart from hitting the back button or killing Chrome from the Android recents menu. I do not believe this is limited to the captive portal use case. After I managed to sign into the captive portal (by avoiding the sign-in flow), I tried the sign-in flow again and it stalled in the same manner. It seems likely that this will happen any time the network is either unable to contact Google or the network is very slow. My concern is that new users of Chrome in emerging markets will be often faced with this problem due to slow networks or having to sign into a captive portal (which are common in EM). So I think this is likely a serious problem if it is impeding completion of the FRE. I believe this could be reproduced locally by trying the FRE on, say, GIN-2gpoor which would have similar network performance.
,
Sep 26 2016
I can reproduce it by disabling wifi with managed account. We might need a timeout mechanism for the first fix, then spin and error screen.
,
Sep 26 2016
Got it, so this seems to be specific to managed/enterprise accounts (Matt, let us know if you encountered this with an @gmail.com account). Updating the title accordingly, updating a couple labels, and adding designers for UX input. Let's: 1) Add a timeout. If the timeout is encountered, we should display a modal error dialogue saying something like "Timed out while trying to sign in to your account managed by <domain name>" (language TBD with UX). We could have a button for "Retry" and a button for "Cancel sign in." 2) Add a spinner (next to the "Continue" button?) to indicate that we're making progress. Let's focus on (1) first, since that's crucial for this not just being broken. I'll chat with Chris and Alan tomorrow about language/UX. In the meantime Ganggui, can you investigate adding a timeout + error modal in?
,
Sep 28 2016
Experimented the timeout mechanism, could start the work when language/UX is ready.
,
Sep 29 2016
Final decision on the language: Unable to sign in Google took to long to respond [ CANCEL ] [ RETRY ]
,
Sep 30 2016
In addition, how long do we expected to wait? 10s?
,
Sep 30 2016
#8 LGTM. Alan, can you please confirm the dialogue in Ganggui's most recent screenshot looks good to you? Ganggui - are there other timeout durations in Chrome that we can use as guidance? If we don't have any precedent to follow, then 10s seems fine to me.
,
Oct 3 2016
LGTM. Thanks Ganggui!
,
Oct 3 2016
Ganggui, let's move ahead with the dialogue in #8. If there's precedent for timeout values we can use from somewhere else, let's try to re-use those. Otherwise, 10s seems fine. We also should really add a spinner to indicate that something is loading. Otherwise, the user will be sitting on this screen for 10s waiting for the request to complete, but might not realize that something is actually happening. Alan, keeping this assigned to you for now. Can you recommend some type of spinner UI? Should we just put a spinner in the center of the screen? Note that the dialogue obviously won't be visible yet, it'll just be the regular sign in screen with the "CONTINUE" button grayed out.
,
Oct 4 2016
Just find a bug, https://bugs.chromium.org/p/chromium/issues/detail?id=652858, and it looks will have better eng solution with 'spinner screen' together. So I will wait for UX input of spinner screen. Note that we show 'import sync data' and 'managed account sign in confirmation' dialogue when switching sync account in 'sync customization preference screen'. The 'spinner screen' have to be displayed there as well.
,
Oct 4 2016
Screens order: 'sign in' full screen -> optional 'switch from managed account' dialog or 'import sync data' dialog -> new 'spinner screen' -> optional 'confirm managed account sign in' dialog
,
Oct 4 2016
Thanks Ganggui. I chatted with Alan about this today, he's going to take a look. Alan, Ganggui's point in #12 is a good one. This spinner may appear on top of the sign in screen, but it also may appear on top of the sync settings screen (if a user is switching sync accounts to a managed account). That's the screen behind the dialogue in this screenshot: https://drive.google.com/a/google.com/file/d/0Byv8caX1Ib9dcGt3cF9lYy1Ta1E/view?usp=sharing
,
Oct 5 2016
,
Oct 14 2016
,
Nov 4 2016
Alan, friendly ping. Can you take a look at this spinner UX when you get a chance?
,
Nov 12 2016
Woot, thanks Alan! Ganggui, do these mocks make sense? The error dialogue should appear after the timeout on top of the spinner.
,
Nov 15 2016
Looks good to me, so we do not have cancel button, clicking on back button cancels the operation. Note that there might be a dialogue after (like timeout dialogue) and before (like 'import sync data') this full screen spinner.
,
Nov 15 2016
Correct, clicking on system back (or the back button in the upper-left on the settings screen) should cancel the operation. In the case of regular sign-in, clicking system back should bring the user back to the sign-in screen. In the case of settings/switch sync account, clicking system back should just remove the spinner and bring the user back to the sync settings screen; clicking the back in the upper left hand corner should bring the user back to the account settings screen. Makes sense that there may be dialogues both before and after these spinner screens - I think that's alright. When you have time to implement this, could you take a video of both flows (sign-in and settings), so we can get a sense for what it looks like? Thanks Ganggui!
,
Dec 12 2016
Re-assign to bzanotti@. He may have better solution with considering of the upcoming changes. Here is my previous attempt CL https://codereview.chromium.org/2385273003/.
,
Dec 22 2016
Ganggui: Thanks for the link. What do you mean by upcoming changes? Am I missing something? Also, FWIW, 10 seconds is the timeout we are already using on iOS (with a cancel button to replace the system back).
,
Dec 22 2016
The changes Ganggui is referring to are other points in the FRE where we may be spinning/timing out (e.g. fetching Finch seed on the welcome screen before the sign in screen appears). I chatted with him about it offline, I can catch you up the next time we meet in person. For now, I don't think we need to worry about it, I think we should treat all these changes independently.
,
Jan 23 2017
--Chrome Identity automated triaging-- This bug is Assigned and has gone one month without any activity, so it is being moved to Available to indicate that it is not actively being worked on. If you are working on this bug, please mark yourself as the owner and move back to Assigned. Please see https://goo.gl/78kbny for more details. Please remove the Services>SignIn or UI>Browser>Profiles components if this bug isn't related to Chrome Identity. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 23 2017
This is still a valid bug that we should fix. Removing the milestone label since it's not associated with a particular milestone. cc'ing the rest of the signin team.
,
Feb 8 2017
,
Feb 15 2017
,
Mar 17 2017
--Chrome Identity automated triaging-- This bug is Assigned and has gone one month without any activity, so it is being moved to Available to indicate that it is not actively being worked on. If you are working on this bug, please mark yourself as the owner and move back to Assigned. Please see https://goo.gl/78kbny for more details. Please remove the Services>SignIn or UI>Browser>Profiles components if this bug isn't related to Chrome Identity. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 17 2017
,
Mar 22 2017
Alan, I have a couple of questions. Spinner is called ProgressBar in Android terminology, so I'm going to use ProgressBar term to be consistent with the code. 1. On xxhdpi screens (like Nexus 5), 24dp ProgressBar looks really small (see attached screenshot). Moreover, updating_gms_progress_view dialog uses 32dp ProgressBar. updating_gms_progress_view dialog can be show right before account selection, so it probably makes sense to be consistent with it. 2. Is it critical to have opaque dialog in FRE and transparent in Sync Settings? We don't have any special handling of access point in that part of code so far. I'm not sure that additional complexity is good in this case.
,
Mar 22 2017
FWIW, I don't feel strongly that we need to make the FRE one opaque. I'm fine with them both being transparent.
,
Mar 23 2017
,
Mar 23 2017
Alan, while testing this feature I've come with another question. Following one of the comments on code review, I've decided to restart network request every time user presses [RETRY], because if there was something wrong with network, it may take a long time for network stack to retry connection. The problem with this approach is that time to get that account management policy on poor 2G network is more than 10 seconds. In my experience, it's usually 15-20 seconds on GIN-2g-poor. You can see the effect on the following video: https://drive.google.com/open?id=0B5n1MGXpFFZcNWNiQk1MX3R4R3c The only way to successfully get account management policy on such a network is to ignore the dialog and keep waiting (you can see it in the end of the video). To gracefully handle such networks, it would probably make sense to increase timeout to something around 30 seconds and add a cancel button to progress dialog. Something like that: https://drive.google.com/open?id=0B5n1MGXpFFZcWVZubjBOcGtIZkE What do you think?
,
Mar 23 2017
+1 to the idea of increasing the timeout and adding a "Cancel" button. Alan - could you help us clean up the spec for what that spinner dialogue (with a "Cancel" button) should look like? Note that it'll need to work on top of both the sign-in screen (during the sign-in case) and the sync settings screen (during the switch sync account case).
,
Mar 23 2017
,
Mar 27 2017
I had a chat with Alan. He is fine with my proposal in comment 34 as long as I change ProgressBar color to Google Blue 500. Eli, what wording should we use for this? What do you think of "Getting account management policy..."? I've uploaded CL that should fix this bug: https://codereview.chromium.org/2772203004. Feel free to chime in or add yourself to reviewers. Known issues: the accentColor style parameter that Chrome uses to modify ProgressBar style works on Android L+ only. It also affects ProgressBar shown after tapping "ACCEPT & CONTINUE" on the very first screen, so current implementation is a bit more consistent. I've added some screenshots to illustrate it.
,
Mar 27 2017
+Shimi for quick strings help For background - we need to show a loading dialogue (see comment above) when users are trying to sign into a managed account on a poor wifi connection. They should be able to cancel, since it can take a while, so that they can continue without signing in. If the request times out (after 30 seconds), we will show an error dialogue (see comment#8) alerting the user that it's failed and offering the ability to "Retry." Can you help us with the strings for both of these dialogues? Let us know if anything is unclear.
,
Mar 28 2017
For loading: "Contacting Google...this may take a minute" < We've used this language in other places in Google products to set expectations. For the error, only a minor change for "Unable": Can't sign in Google took too long to respond I also considered "Signing in...this may take a minute" but the "Contacting Google" language would work nicely with "Google took too long" in the case of a timeout error. In either case, we don't need to expose exactly what we're doing to the user. We can be more general.
,
Mar 28 2017
,
Mar 28 2017
Over to Boris to update the strings. Thanks Shimi!
,
Mar 29 2017
Thanks, Shimi! I've attached screenshots with final dialogs. I have to admit that lack of whitespaces in "Google…this" looks a bit weird for me. Is it just my perception?
,
Mar 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/33295fdb5fa26e409b197b81b2069c435184d6e3 commit 33295fdb5fa26e409b197b81b2069c435184d6e3 Author: bsazonov <bsazonov@chromium.org> Date: Wed Mar 29 18:49:00 2017 Add progress and timeout dialogs for getting account management policy This CL adds two dialogs that are shown by ConfirmSyncDataStateMachine. Dialogs implementation is in ConfirmSyncDataStateMachineDelegate. The first dialog is a progress dialog that is shown while Chrome requests server for account management policy. The second dialog is a timeout dialog that is shown if Chrome doesn't receive account management policy within 30 seconds. BUG= 650121 Review-Url: https://codereview.chromium.org/2772203004 Cr-Commit-Position: refs/heads/master@{#460471} [add] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java/res/layout/signin_progress_bar_dialog.xml [modify] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java [add] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java [modify] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java [modify] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java/strings/android_chrome_strings.grd [modify] https://crrev.com/33295fdb5fa26e409b197b81b2069c435184d6e3/chrome/android/java_sources.gni
,
Mar 29 2017
The first loading dialogue looks a little off to me as well. Why is there so much whitespace between the "Cancel" button and the bottom of the line of text? Also, is the text between the two dialogues different? It looks darker/more bold in the second dialogue to me. Alan - could you please comment from a UX perspective?
,
Mar 30 2017
Eli, thanks for bringing this up. You're right - I've added too much padding to the bottom of progress dialog content. I've found a dedicated style with appropriate paddings for such a case: AlertDialogContent. CL to use this style for timeout dialog: https://codereview.chromium.org/2786943002. I've attached screenshots of Layout Inspector for old and new layouts. I've also attached screenshot with final view of the dialog. Alan, could you please validate these? Considering text color - it looks like it is a problem in our style for AlertDialog. I've found two different colors: 1. #333333: all texts in progress dialog, timeout dialog title text, timeout dialog buttons text. 2. #000000: message text in timeout dialog. Not sure where it comes from, though. I use regular AlertDialog.Builder to create timeout dialog, so I would expect this issue to be widespread. It probably makes sense to add another bug for it.
,
Mar 30 2017
The screenshot of the progress dialogue in #46 along with the screenshot of the timeout dialogue in #43 LGTM. Alan - please confirm those both look good to you as well.
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1eda31a97825bb7504ca74b5b14a6cfc73de03d commit c1eda31a97825bb7504ca74b5b14a6cfc73de03d Author: bsazonov <bsazonov@chromium.org> Date: Fri Mar 31 13:55:22 2017 Fix progress and timeout dialog for getting account management policy This CL fixes two issues introduced by https://crrev.com/2772203004 1. Bottom padding of dialog content for progress dialog. This CL replaces manual padding control with AlertDialogContent style. 2. Retry button behavior in timeout dialog: it wasn't showing progress dialog again because of a bug introduced during review. BUG= 650121 Review-Url: https://codereview.chromium.org/2786943002 Cr-Commit-Position: refs/heads/master@{#461108} [modify] https://crrev.com/c1eda31a97825bb7504ca74b5b14a6cfc73de03d/chrome/android/java/res/layout/signin_progress_bar_dialog.xml [modify] https://crrev.com/c1eda31a97825bb7504ca74b5b14a6cfc73de03d/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java
,
Apr 4 2017
>> The screenshot of the progress dialogue in #46 along with the screenshot of the timeout dialogue in #43 LGTM. Alan - please confirm those both look good to you as well. All button-text (retry AND cancel) on dialogs should use #4285F4 (Google Blue 500). https://material.googleplex.com/components/dialogs.html#dialogs-full-screen-dialogs
,
Apr 4 2017
Other than the button-text coloring, everything LGTM.
,
Apr 4 2017
,
Apr 5 2017
Boris - once you've updated the button coloring, could you post one final set of screenshots (or a video) of the entire flow? I'd like to get a sense for how jarring the difference in styling is between the two dialogues.
,
Apr 7 2017
Eventually, I've found a way to fix timeout message text color as well. CL is here: https://codereview.chromium.org/2801213003/. Attached screenshots of final dialogs. Eli, what do you think?
,
Apr 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/175d75afa30ba7c98b3c183e21edf233aaa9269f commit 175d75afa30ba7c98b3c183e21edf233aaa9269f Author: bsazonov <bsazonov@chromium.org> Date: Fri Apr 07 14:35:13 2017 Fix UI style of signin AlertDialogs on Android This CL adds SigninAlertDialogTheme style that uses Google Blue 500 color for button text and forces message text to use Chrome default text color. BUG= 650121 ,669787 Review-Url: https://codereview.chromium.org/2801213003 Cr-Commit-Position: refs/heads/master@{#462853} [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/res/values-v17/styles.xml [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/src/org/chromium/chrome/browser/signin/AccountAdder.java [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmImportSyncDataDialog.java [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmManagedSyncDataDialog.java [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineDelegate.java [modify] https://crrev.com/175d75afa30ba7c98b3c183e21edf233aaa9269f/chrome/android/java/src/org/chromium/chrome/browser/signin/SignOutDialogFragment.java
,
Apr 7 2017
Looks great to me! Thanks much for resolving this Boris, great job. Can we mark this as Fixed, or are there any other pending changes you have for this?
,
Apr 10 2017
Thanks, Eli. Last obvious improvement for this flow would be to leave "CONTINUE" button enabled while showing dialogs on top of it. Currently it toggles between enabled blue state and disabled gray one and it looks a bit strange. However, it is not possible to fix it before I address https://crbug.com/707786. I'll mark this as Fixed and add an appropriate note to https://crbug.com/707786. BTW, I've checked these dialogs on Chrome Canary 59.0.3067.0 - everything looks good.
,
Apr 10 2017
Awesome, thanks Boris, great work! |
|||||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||||||||
Comment 1 by ew...@chromium.org
, Sep 26 2016