New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 872936 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

White wallpaper in OOBE should be rendered by wallpaper controller, not by OOBE HTML

Project Member Reported by jdufault@chromium.org, Aug 9

Issue description

Right now the white OOBE background is being done in HTML. This has a couple of issues:

- on boot, the white background is not applied until after OOBE is loaded
- if oobe crashes/etc the white background will go away
- the status area renders assuming the white background

The biggest issue here is the boot experience, which looks very janky/unpolished.

Either we need to hide native UI until webui is ready, or we move the white background rendering to native UI. I think the right solution here is to move background rendering to native UI. It works in both webui and views and has less overdraw.
 
Summary: White wallpaper in OOBE should be rendered by wallpaper controller, not by OOBE HTML (was: White wallpaper in OOBE should be rendered by wallpaper, not as a fake overlay by oobe)
We really should try to merge this to 69 as well.
Cc: elizabethchiu@chromium.org
+elizabethchiu@ for thoughts

Deciding when to show the white background is a bit tricky; there are a number of cases to consider.

1 Going through OOBE normally
2 Partially completing OOBE, never adding a user, coming back to the device a week later where the device boots directly into add user
3 Device is enterprise enrolled but doesn't have any users
4 Device is enterprise enrolled with users, but then the user removes all users from login
5 Add user when there are already users
6 Special screens like powerwash

Some of these scenarios have an add user dialog that cannot be dismissed (cases 1,2,3,4).
> Some of these scenarios have an add user dialog that cannot be dismissed (cases 1,2,3,4).

We should consider using white background to subtly inform the user that the gaia/add user dialog cannot be dismissed.
Two options:

1) Create a |ShowOOBEWallpaper| mojo call in wallpaper.mojom (Or more generally, |ShowSolidColorWallpaper(SkColor color)|). |wallpaper_controller_client| in //chrome may call this method when session state is OOBE, but there should be another caller responsible for calling |ShowUserWallpaper(AccountId id)| to update the wallpaper when OOBE completes (or whenever white background is no longer desired.) It can also call |ShowSigninWallpaper| if there's no user logged in yet, in which case it will display the default wallpaper.

2) Put the entire logic in wallpaper controller. It will observe the session state and show white wallpaper if it' SessionState::OOBE, and a user wallpaper otherwise.

I prefer 1) because it's more flexible.
Or, option 3): If the primary reason for doing this is to ensure a smooth boot experience, I think it's better to: apply the white wallpaper before OOBE dialog is shown, but as soon as the dialog (which still keeps the full screen html white background) is ready, notify the wallpaper controller to revert to the default wallpaper behind the scene. It makes sense to let the html dialog maintain full-screen, since it's a little weird if the full screen actually consists partially of the OOBE dialog and partially of wallpaper.

(If OOBE crashes, user may see the default wallpaper behind, but it may not be a big deal.)
Per offline discussion with wzang@, I also think that option 3) from #6 is reasonable.
If the requirements are simple enough that we can just observe session state to determine wallpaper color, I think we should go with solution 2; however, I doubt this will be the case.

I think a slight variation on solution 1 is the best, perhaps |SetWallpaperOverride(optional<Color> override_color)|. This way, login and OOBE can separately control wallpaper color as needed. Separate control here is good, because OOBE does not have all of the context it needs to make the decision w.r.t. wallpaper. For example, login instantiates OOBE for the power-wash dialog, which should most likely not have a white background (whereas power-wash in OOBE likely will have a white background). As a note, one of the things login has been doing is removing chrome/'s ability to control what the UI looks like (ie, exactly this).

Here are the issues I see with option 3:
- complexity: we now have two places that render a white wallpaper, which are used at differing times
- new class of bugs: it's now possible for wallpaper render state to get out of sync, ie, if ash/ thinks OOBE is rendering wallpaper but OOBE is not for whatever reason
- challenges with login: OOBE dialog is not always full-screen when used in login
- works around the real bug (see below)

> since it's a little weird if the full screen actually consists partially of the OOBE dialog and partially of wallpaper.

I don't think this is as strange as it may seem; there are already other elements which are rendered separately, ie, the shelf, the status area, etc, but which look like they're part of the same UI in OOBE (technically the shelf in OOBE is still HTML, but that is a bug and needs get fixed/moved to native shelf). The crux of the issue on boot is that these other elements are ready much faster than OOBE and render assuming OOBE is already rendered.

Fixing the render order such that all appear at the same time would also fix the boot issue here, but I think it is still worth rendering the white background in the wallpaper controller to avoid overdraw and also due to making login simpler (now login does need to dynamically resize the OOBE widget to full-screen when it wants a white background). If we fix the render order issue, then solution 3 is not needed - which implies it is a work-around for the real issue.
We'll need something like a "white background controller" to serve as the source of truth and inform all UI elements to update their states. This is necessary because the criteria for white background should be more complex than a simple session state. Note there's already a bug now: in case 2) of comment #3, the Add user screen is NOT full screen but the status area thinks it is.

> new class of bugs: it's now possible for wallpaper render state to get out of sync, ie, if ash/ thinks OOBE is rendering wallpaper but OOBE is not for whatever reason

This issue is not specific to option 3), as mentioned above, it's always possible to have the out of sync problem if we don't have a source of truth.

> challenges with login: OOBE dialog is not always full-screen when used in login

This is not an issue for option 3), note that if we go with 3), we'll still need to create a ShowSolidColorWallpaper mojo call, and login screen is able to use it. But that means that login full-screen is partly wallpaper and partly OOBE dialog, yet the OOBE full-screen is all dialog (except for using wallpaper as a buffer right after boot). If we want to unify the implementation here, I still prefer to use all html (ie. how hard is it to fix the full-screen dialog issue in login?)

> I don't think this is as strange as it may seem; there are already other elements which are rendered separately, ie, the shelf, the status area, etc

The shelf and status area are indeed separate UI elements, but wallpaper is not. Essentially the use of a white wallpaper relies on the fact that it blends into the OOBE dialog, so that the dialog "looks like" it's full-screen. Using a white wallpaper is not robust: if UX decides to add more elements near the edge of the screen, we'll still need to make the dialog full-screen.

I think we should:
1) Create a source truth for all UI elements to query.
2) To fix the boot experience: if possible, make the white splash screen stay until OOBE dialog is ready, or use a white wallpaper as a buffer.
3) When OOBE is shown, the full screen should come from the html background, and fix the full-screen dialog issue in login.
Talking offline, we should investigate delaying startup so that the splash screen does not go away until we're actually ready. That will deal with the biggest polish issue.

Beyond that, I'm fine with rendering the background in either the dialog or wallpaper view, but the better approach may depend on what the UX ends up being.
Talking offline with elizabethchiu@, it's confirmed that we want to show a full white background as long as no user is added yet (which covers cases 1-3 in #3), for cases 4-6, it's undecided but preferred not to have full screen at this point.

So it's okay to use session state OOBE as the criteria to show full screen background, but I'll still go with option 1) and create a mojo API, in case we need full screen for login later.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/408f12cadfb0abebf664e74809706144b3a60590

commit 408f12cadfb0abebf664e74809706144b3a60590
Author: Wenzhao Zang <wzang@chromium.org>
Date: Sat Aug 11 07:32:51 2018

cros: Dialogs should always be full-screen during OOBE

The dialogs should always be full-screen as long as there's no user
added yet. This covers the corner cases that user stops in the middle
of OOBE and returns later.

Do not show the dialog as full-screen (at least for now, until there's
UX update) for cases that the OOBE is completed, but user pods are not
shown (due to policy, or all users are removed manually on an enrolled
device).

Bug:  872936 
Change-Id: I02a648585a7cbebd1b570096ab3608bff000e96d
Reviewed-on: https://chromium-review.googlesource.com/1171916
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582437}
[modify] https://crrev.com/408f12cadfb0abebf664e74809706144b3a60590/chrome/browser/resources/chromeos/login/oobe_dialog.js

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a

commit 7f67c48fca5aba83717fe9c99b2d92a2049c9f6a
Author: Wenzhao Zang <wzang@chromium.org>
Date: Wed Aug 15 03:51:53 2018

cros: Smoothen the transition between boot splash and full-screen OOBE

1) The full-screen white background in OOBE is rendered in web-ui, so
   there's a delay showing it. Users will see a glimpse of the default
   wallpaper (it's blue on an eve) between the boot splash screen
   and the OOBE screen (both white). To avoid this, we can show a white
   wallpaper during the entire OOBE screen, which can show right after
   the boot splash seamlessly.

2) The original idea was to delay the dismissal of the boot splash
   screen until OOBE dialog is ready. However, boot splash screen will
   not always show (e.g. when a guest user exits, or browser crash and
   restart). In such cases, we also want to avoid showing the blue
   default wallpaper before the white OOBE screen is loaded. Therefore,
   showing a white wallpaper is a better solution.

Bug:  872936 
Change-Id: Iaa600f9d455956743a6cfd3dd26f07ad2a4ed61d
Reviewed-on: https://chromium-review.googlesource.com/1172066
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583154}
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/ash/public/cpp/wallpaper_types.h
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/ash/public/interfaces/wallpaper.mojom
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/ash/wallpaper/wallpaper_controller_unittest.cc
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/chrome/browser/ui/ash/test_wallpaper_controller.cc
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/chrome/browser/ui/ash/test_wallpaper_controller.h
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/chrome/browser/ui/ash/wallpaper_controller_client.cc
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/chrome/browser/ui/ash/wallpaper_controller_client.h
[modify] https://crrev.com/7f67c48fca5aba83717fe9c99b2d92a2049c9f6a/tools/metrics/histograms/enums.xml

Labels: -M-70 M-69 Merge-Request-69
Merge request for the two CLs in #12 and #13.
Merge approved, M69.
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 15

Labels: merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c8be30b4d5fa4c892bbffc5e5a996c2fd30d6a0

commit 6c8be30b4d5fa4c892bbffc5e5a996c2fd30d6a0
Author: Wenzhao Zang <wzang@chromium.org>
Date: Wed Aug 15 21:50:49 2018

[Merge to M69] cros: Dialogs should always be full-screen during OOBE

The dialogs should always be full-screen as long as there's no user
added yet. This covers the corner cases that user stops in the middle
of OOBE and returns later.

Do not show the dialog as full-screen (at least for now, until there's
UX update) for cases that the OOBE is completed, but user pods are not
shown (due to policy, or all users are removed manually on an enrolled
device).

TBR=wzang@chromium.org

(cherry picked from commit 408f12cadfb0abebf664e74809706144b3a60590)

Bug:  872936 
Change-Id: I02a648585a7cbebd1b570096ab3608bff000e96d
Reviewed-on: https://chromium-review.googlesource.com/1171916
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#582437}
Reviewed-on: https://chromium-review.googlesource.com/1176434
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#659}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/6c8be30b4d5fa4c892bbffc5e5a996c2fd30d6a0/chrome/browser/resources/chromeos/login/oobe_dialog.js

Project Member

Comment 17 by bugdroid1@chromium.org, Aug 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d56a0d4f7305cb08e51d2392571b383c283ee56

commit 6d56a0d4f7305cb08e51d2392571b383c283ee56
Author: Wenzhao Zang <wzang@chromium.org>
Date: Wed Aug 15 22:00:38 2018

[Merge to M69] cros: Smoothen the transition between boot splash and full-screen OOBE

1) The full-screen white background in OOBE is rendered in web-ui, so
   there's a delay showing it. Users will see a glimpse of the default
   wallpaper (it's blue on an eve) between the boot splash screen
   and the OOBE screen (both white). To avoid this, we can show a white
   wallpaper during the entire OOBE screen, which can show right after
   the boot splash seamlessly.

2) The original idea was to delay the dismissal of the boot splash
   screen until OOBE dialog is ready. However, boot splash screen will
   not always show (e.g. when a guest user exits, or browser crash and
   restart). In such cases, we also want to avoid showing the blue
   default wallpaper before the white OOBE screen is loaded. Therefore,
   showing a white wallpaper is a better solution.

TBR=wzang@chromium.org

(cherry picked from commit 7f67c48fca5aba83717fe9c99b2d92a2049c9f6a)

Bug:  872936 
Change-Id: Iaa600f9d455956743a6cfd3dd26f07ad2a4ed61d
Reviewed-on: https://chromium-review.googlesource.com/1172066
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#583154}
Reviewed-on: https://chromium-review.googlesource.com/1176440
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#660}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/ash/public/cpp/wallpaper_types.h
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/ash/public/interfaces/wallpaper.mojom
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/ash/wallpaper/wallpaper_controller.cc
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/ash/wallpaper/wallpaper_controller.h
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/ash/wallpaper/wallpaper_controller_unittest.cc
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/chrome/browser/ui/ash/test_wallpaper_controller.cc
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/chrome/browser/ui/ash/test_wallpaper_controller.h
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/chrome/browser/ui/ash/wallpaper_controller_client.cc
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/chrome/browser/ui/ash/wallpaper_controller_client.h
[modify] https://crrev.com/6d56a0d4f7305cb08e51d2392571b383c283ee56/tools/metrics/histograms/enums.xml

This is fixed. But, we can continue discussing whether to remove full-screen-dialog property in OOBE. I don't think we should.
I'm okay leaving it in for the time being. Perhaps it will reduce complexity during oobe when the user session is created.
cindyb@ please always use the merge approval label Merge-Approved-MM to approved a merge. The merge violation will flag such bug as a violation if the label is not properly applicable. Thanks!
(I should mention that it still feels like a bit of a hack, especially given that there are now two places that render the white wallpaper).
Actually I think this is the optimal solution. This is indeed a workaround, but IMO that's the whole point of it: to  work around the real issue that OOBE web-ui is rendered slower than the native elements. Before we re-implement OOBE in views (if that ever happens), it's reasonable to use something as a transition buffer to make sure everything appears together. But, after OOBE dialog has the ability to render full-screen, IMO we should not continue relying on the wallpaper. Perhaps, one improvement is to revert to the regular wallpaper right after OOBE dialog is shown, instead of waiting until user session starts, so that it doesn't feel "duplicate", but other than that, IMO this is the optimal solution.

Note:
1) The UX mentioned once that they wanted to add new elements around the edge of the screen. So full-screen OOBE is a must-have when that happens.

2) If the property is removed, we may run into another polish issue when dismissing OOBE dialog (I haven't verified but I would imagine the wallpaper will always respond faster than web-ui elements to the same signal.)

3) If we ever need full-screen "Add person" in views-login, and if full-screen widget is not possible in login, the worst case is that we can still show a white wallpaper as the background. But there will be additional polish issue to make sure the two things are synchronous, so I think fixing the full-screen widget issue in login is a better solution.

Project Member

Comment 23 by sheriffbot@chromium.org, Aug 16

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69 Merge-Approved-69
Status: Fixed (was: Assigned)
Issue 868041 has been merged into this issue.
Project Member

Comment 27 by sheriffbot@chromium.org, Aug 20

Cc: cindyb@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Verified (was: Fixed)
Verified on M69 (10895.31.0, 69.0.3497.50) beta channel on caroline.
Labels: -Merge-Approved-69

Sign in to add a comment