Chrome OS Oobe: Show Ash dialog only if required

When starting a managed guest session (with no Terms of Service), a
blank Ash dialog is shown for a brief moment before the session starts.
This is caused by the Ash dialog for Oobe being shown even when all the
Oobe screens are skipped.

This CL changes the behavior of the Ash dialog to only show when
required by using a OobeUI::Observer. The dialog is not shown by
default, and will be shown when a Oobe screen is shown.

OobeScreenWaiter now checks that the native window is visible by
default. A small number of tests had to be changed to either show the
native window or ignore the visibility check. The reasoning for the
changes are in the comments for their respective tests.

The WebUI behavior is unchanged.

Bug: 1000164
Change-Id: I9995f4beba85d7c5af7b13f81f1a654e2d52f372
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1790450
Commit-Queue: Jit Yao Yap <jityao@google.com>
Reviewed-by: Denis Kuznetsov <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697539}

chrome/browser/chromeos/login/configuration_based_oobe_browsertest.cc

@@ -350,7 +350,7 @@ IN_PROC_BROWSER_TEST_F(OobeConfigurationEnrollmentTest, TestEnrollUsingToken) {
 
 // Check that HID detection screen is shown if it is not specified by
 // configuration.
-IN_PROC_BROWSER_TEST_F(OobeConfigurationTestNoHID, TestLeaveWelcomeScreen) {
+IN_PROC_BROWSER_TEST_F(OobeConfigurationTestNoHID, TestShowHID) {
   LoadConfiguration();
   OobeScreenWaiter(HIDDetectionView::kScreenId).Wait();
 }

chrome/browser/chromeos/login/enrollment/hands_off_enrollment_browsertest.cc

@@ -152,7 +152,10 @@ IN_PROC_BROWSER_TEST_F(HandsOffEnrollmentTest, EnrollmentError) {
   WizardController::default_controller()->AdvanceToScreen(
       WelcomeView::kScreenId);
 
-  OobeScreenWaiter(NetworkScreenView::kScreenId).Wait();
+  OobeScreenWaiter screen_waiter(NetworkScreenView::kScreenId);
+  // WebUI window is not visible until the screen animation finishes.
+  screen_waiter.set_no_check_native_window_visible();
+  screen_waiter.Wait();
 
   OobeScreenWaiter(EnrollmentScreenView::kScreenId).Wait();
 

chrome/browser/chromeos/login/oobe_interactive_ui_test.cc

@@ -3,6 +3,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
+
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/public/cpp/test/shell_test_api.h"
 #include "base/bind.h"
@@ -12,6 +14,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "build/branding_buildflags.h"
+#include "chrome/browser/chrome_browser_main.h"
+#include "chrome/browser/chrome_browser_main_extra_parts.h"
 #include "chrome/browser/chromeos/arc/arc_service_launcher.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h"
@@ -53,6 +57,8 @@
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
 #include "ui/display/display_switches.h"
 
 using net::test_server::BasicHttpResponse;
@@ -348,6 +354,58 @@ class ScopedQuickUnlockPrivateGetAuthTokenFunctionObserver
       ScopedQuickUnlockPrivateGetAuthTokenFunctionObserver);
 };
 
+// Observes an |aura::Window| to see if the window was visible at some point in
+// time.
+class NativeWindowVisibilityObserver : public aura::WindowObserver {
+ public:
+  NativeWindowVisibilityObserver() = default;
+  // aura::Window will remove observers on destruction.
+  ~NativeWindowVisibilityObserver() override = default;
+
+  void Observe(aura::Window* window) {
+    window_ = window;
+    window_->AddObserver(this);
+  }
+
+  void OnWindowVisibilityChanged(aura::Window* window, bool visible) override {
+    if (visible)
+      was_visible_ = visible;
+  }
+
+  bool was_visible() { return was_visible_; }
+
+ private:
+  // The window was visible at some point in time.
+  bool was_visible_ = false;
+
+  aura::Window* window_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeWindowVisibilityObserver);
+};
+
+// Sets the |NativeWindowVisibilityObserver| to observe the
+// |LoginDisplayHost|'s |NativeWindow|. This needs to be done in
+// |PostProfileInit()| as the |default_host| will not be initialized before
+// this.
+class NativeWindowVisibilityBrowserMainExtraParts
+    : public ChromeBrowserMainExtraParts {
+ public:
+  NativeWindowVisibilityBrowserMainExtraParts(
+      NativeWindowVisibilityObserver* observer)
+      : observer_(observer) {}
+  ~NativeWindowVisibilityBrowserMainExtraParts() override = default;
+
+  // ChromeBrowserMainExtraParts:
+  void PostProfileInit() override {
+    observer_->Observe(LoginDisplayHost::default_host()->GetNativeWindow());
+  }
+
+ private:
+  NativeWindowVisibilityObserver* observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeWindowVisibilityBrowserMainExtraParts);
+};
+
 class OobeEndToEndTestSetupMixin : public InProcessBrowserTestMixin {
  public:
   struct Parameters {
@@ -678,7 +736,8 @@ class PublicSessionOobeTest
       : PublicSessionOobeTest(false /*requires_terms_of_service*/) {}
 
   explicit PublicSessionOobeTest(bool requires_terms_of_service)
-      : requires_terms_of_service_(requires_terms_of_service) {
+      : requires_terms_of_service_(requires_terms_of_service),
+        observer_(std::make_unique<NativeWindowVisibilityObserver>()) {
     // Prevents Chrome from starting to quit right after login display is
     // finalized.
     login_manager_.set_should_launch_browser(true);
@@ -722,11 +781,26 @@ class PublicSessionOobeTest
     MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
   }
 
+  void CreatedBrowserMainParts(content::BrowserMainParts* parts) override {
+    MixinBasedInProcessBrowserTest::CreatedBrowserMainParts(parts);
+    static_cast<ChromeBrowserMainParts*>(parts)->AddParts(
+        new NativeWindowVisibilityBrowserMainExtraParts(observer_.get()));
+  }
+
+  void TearDownOnMainThread() override {
+    observer_.reset();
+    MixinBasedInProcessBrowserTest::TearDownOnMainThread();
+  }
+
+  bool DialogWasVisible() { return observer_->was_visible(); }
+
   LoginManagerMixin login_manager_{&mixin_host_, {}};
 
  private:
   const bool requires_terms_of_service_;
 
+  std::unique_ptr<NativeWindowVisibilityObserver> observer_;
+
   OobeEndToEndTestSetupMixin setup_{&mixin_host_, nullptr, GetParam()};
   DeviceStateMixin device_state_{
       &mixin_host_, DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED};
@@ -734,6 +808,8 @@ class PublicSessionOobeTest
 
 IN_PROC_BROWSER_TEST_P(PublicSessionOobeTest, NoTermsOfService) {
   login_manager_.WaitForActiveSession();
+  // Check that the dialog was never shown.
+  EXPECT_FALSE(DialogWasVisible());
 }
 
 INSTANTIATE_TEST_SUITE_P(

chrome/browser/chromeos/login/saml/saml_browsertest.cc

@@ -1482,6 +1482,9 @@ void SAMLPasswordAttributesTest::SetUpOnMainThread() {
 // Verifies that password attributes are extracted and stored during a
 // successful log in - but only if the appropriate policy is enabled.
 IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginSucceeded) {
+  // LoginDisplayHostMojo does not show Oobe dialog by default.
+  LoginDisplayHost::default_host()->ShowGaiaDialog(true, EmptyAccountId());
+
   fake_saml_idp()->SetLoginHTMLTemplate("saml_login.html");
   fake_saml_idp()->SetSamlResponseFile("saml_with_password_attributes.xml");
   StartSamlAndWaitForIdpPageLoad(kFirstSAMLUserEmail);
@@ -1513,6 +1516,9 @@ IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginSucceeded) {
 
 // Verify that no password attributes are stored when login fails.
 IN_PROC_BROWSER_TEST_P(SAMLPasswordAttributesTest, LoginFailed) {
+  // LoginDisplayHostMojo does not show Oobe dialog by default.
+  LoginDisplayHost::default_host()->ShowGaiaDialog(true, EmptyAccountId());
+
   fake_saml_idp()->SetLoginHTMLTemplate("saml_login.html");
   fake_saml_idp()->SetSamlResponseFile("saml_with_password_attributes.xml");
   StartSamlAndWaitForIdpPageLoad(kFirstSAMLUserEmail);

chrome/browser/chromeos/login/test/oobe_screen_waiter.cc

@@ -6,7 +6,11 @@
 
 #include "base/run_loop.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/chromeos/login/ui/webui_login_view.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/window.h"
+#include "ui/views/controls/webview/webview.h"
+#include "ui/views/view.h"
 
 namespace chromeos {
 
@@ -18,13 +22,20 @@ OobeScreenWaiter::~OobeScreenWaiter() = default;
 void OobeScreenWaiter::Wait() {
   DCHECK_EQ(State::IDLE, state_);
 
-  if (GetOobeUI()->current_screen() == target_screen_) {
+  if ((!check_native_window_visible_ || IsNativeWindowVisible()) &&
+      IsTargetScreenReached()) {
     state_ = State::DONE;
     return;
   }
   DCHECK(!run_loop_);
 
   oobe_ui_observer_.Add(GetOobeUI());
+  if (check_native_window_visible_) {
+    aura::Window* native_window =
+        LoginDisplayHost::default_host()->GetNativeWindow();
+    DCHECK(native_window);
+    native_window_observer_.Add(native_window);
+  }
 
   state_ = State::WAITING_FOR_SCREEN;
 
@@ -35,6 +46,8 @@ void OobeScreenWaiter::Wait() {
   DCHECK_EQ(State::DONE, state_);
 
   oobe_ui_observer_.RemoveAll();
+  if (check_native_window_visible_)
+    native_window_observer_.RemoveAll();
 
   if (assert_last_screen_)
     EXPECT_EQ(target_screen_, GetOobeUI()->current_screen());
@@ -60,10 +73,33 @@ void OobeScreenWaiter::OnCurrentScreenChanged(OobeScreenId current_screen,
     return;
   }
 
-  if (new_screen == target_screen_)
+  if (check_native_window_visible_ && !IsNativeWindowVisible()) {
+    return;
+  }
+
+  if (IsTargetScreenReached())
     EndWait();
 }
 
+void OobeScreenWaiter::OnWindowVisibilityChanged(aura::Window* window,
+                                                 bool visible) {
+  DCHECK_NE(state_, State::IDLE);
+  DCHECK(check_native_window_visible_);
+
+  if (IsNativeWindowVisible() && IsTargetScreenReached())
+    EndWait();
+}
+
+bool OobeScreenWaiter::IsTargetScreenReached() {
+  return GetOobeUI()->current_screen() == target_screen_;
+}
+
+bool OobeScreenWaiter::IsNativeWindowVisible() {
+  aura::Window* native_window =
+      LoginDisplayHost::default_host()->GetNativeWindow();
+  return native_window && native_window->IsVisible();
+}
+
 void OobeScreenWaiter::OnDestroyingOobeUI() {
   oobe_ui_observer_.RemoveAll();
 

chrome/browser/chromeos/login/test/oobe_screen_waiter.h

@@ -10,6 +10,7 @@
 #include "chrome/browser/chromeos/login/oobe_screen.h"
 #include "chrome/browser/chromeos/login/test/test_condition_waiter.h"
 #include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
+#include "ui/aura/window_observer.h"
 
 namespace base {
 class RunLoop;
@@ -21,13 +22,17 @@ class OobeUI;
 
 // A waiter that blocks until the target oobe screen is reached.
 class OobeScreenWaiter : public OobeUI::Observer,
-                         public test::TestConditionWaiter {
+                         public test::TestConditionWaiter,
+                         public aura::WindowObserver {
  public:
   explicit OobeScreenWaiter(OobeScreenId target_screen);
   ~OobeScreenWaiter() override;
 
   void set_no_assert_last_screen() { assert_last_screen_ = false; }
   void set_assert_next_screen() { assert_next_screen_ = true; }
+  void set_no_check_native_window_visible() {
+    check_native_window_visible_ = false;
+  }
 
   // OobeUI::Observer implementation:
   void OnCurrentScreenChanged(OobeScreenId current_screen,
@@ -37,12 +42,21 @@ class OobeScreenWaiter : public OobeUI::Observer,
   // TestConditionWaiter;
   void Wait() override;
 
+  // aura::WindowObserver:
+  void OnWindowVisibilityChanged(aura::Window* window, bool visible) override;
+
  private:
   enum class State { IDLE, WAITING_FOR_SCREEN, DONE };
 
   OobeUI* GetOobeUI();
   void EndWait();
 
+  // Returns true if the target screen is reached.
+  bool IsTargetScreenReached();
+
+  // Returns true if the native window is visible.
+  bool IsNativeWindowVisible();
+
   const OobeScreenId target_screen_;
 
   State state_ = State::IDLE;
@@ -52,12 +66,20 @@ class OobeScreenWaiter : public OobeUI::Observer,
   // This applies to the time period Wait() is running only.
   bool assert_last_screen_ = true;
 
-  // If set, the watier will assert OOBE UI does not transition to any other
-  // screen before transisioning to the target screen.
+  // If set, the waiter will assert OOBE UI does not transition to any other
+  // screen before transitioning to the target screen.
   bool assert_next_screen_ = false;
 
+  // If set, the waiter will only finish waiting if the target screen has been
+  // reached and the native window is visible. The assert_last_screen and
+  // assert_next_screen checks will only be done if the native window is
+  // visible. True by default.
+  bool check_native_window_visible_ = true;
+
   ScopedObserver<OobeUI, OobeScreenWaiter> oobe_ui_observer_{this};
 
+  ScopedObserver<aura::Window, OobeScreenWaiter> native_window_observer_{this};
+
   std::unique_ptr<base::RunLoop> run_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(OobeScreenWaiter);

chrome/browser/chromeos/login/ui/login_display_host_mojo.cc

@@ -78,6 +78,7 @@ LoginDisplayHostMojo::~LoginDisplayHostMojo() {
   LoginScreenClient::Get()->SetDelegate(nullptr);
   if (dialog_) {
     dialog_->GetOobeUI()->signin_screen_handler()->SetDelegate(nullptr);
+    StopObservingOobeUI();
     dialog_->Close();
   }
 }
@@ -85,6 +86,7 @@ LoginDisplayHostMojo::~LoginDisplayHostMojo() {
 void LoginDisplayHostMojo::OnDialogDestroyed(
     const OobeUIDialogDelegate* dialog) {
   if (dialog == dialog_) {
+    StopObservingOobeUI();
     dialog_ = nullptr;
     wizard_controller_.reset();
   }
@@ -101,25 +103,25 @@ void LoginDisplayHostMojo::ShowPasswordChangedDialog(bool show_password_error,
   DCHECK(GetOobeUI());
   GetOobeUI()->signin_screen_handler()->ShowPasswordChangedDialog(
       show_password_error, email);
-  dialog_->Show();
+  ShowDialog();
 }
 
 void LoginDisplayHostMojo::ShowWhitelistCheckFailedError() {
   DCHECK(GetOobeUI());
   GetOobeUI()->signin_screen_handler()->ShowWhitelistCheckFailedError();
-  dialog_->Show();
+  ShowDialog();
 }
 
 void LoginDisplayHostMojo::ShowErrorScreen(LoginDisplay::SigninError error_id) {
   DCHECK(GetOobeUI());
   GetOobeUI()->signin_screen_handler()->ShowErrorScreen(error_id);
-  dialog_->Show();
+  ShowDialog();
 }
 
 void LoginDisplayHostMojo::ShowSigninUI(const std::string& email) {
   DCHECK(GetOobeUI());
   GetOobeUI()->signin_screen_handler()->ShowSigninUI(email);
-  dialog_->Show();
+  ShowDialog();
 }
 
 void LoginDisplayHostMojo::HandleDisplayCaptivePortal() {
@@ -174,12 +176,15 @@ void LoginDisplayHostMojo::SetStatusAreaVisible(bool visible) {
 }
 
 void LoginDisplayHostMojo::StartWizard(OobeScreenId first_screen) {
-  DCHECK(GetOobeUI());
+  OobeUI* oobe_ui = GetOobeUI();
+  DCHECK(oobe_ui);
+  // Dialog is not shown immediately, and will be shown only when a screen
+  // change occurs. This prevents the dialog from showing when there are no
+  // screens to show.
+  ObserveOobeUI();
 
   wizard_controller_ = std::make_unique<WizardController>();
   wizard_controller_->Init(first_screen);
-
-  dialog_->Show();
 }
 
 WizardController* LoginDisplayHostMojo::GetWizardController() {
@@ -241,11 +246,11 @@ void LoginDisplayHostMojo::OnPreferencesChanged() {
 }
 
 void LoginDisplayHostMojo::OnStartAppLaunch() {
-  dialog_->ShowFullScreen();
+  ShowFullScreen();
 }
 
 void LoginDisplayHostMojo::OnStartArcKiosk() {
-  dialog_->ShowFullScreen();
+  ShowFullScreen();
 }
 
 void LoginDisplayHostMojo::OnBrowserCreated() {
@@ -264,7 +269,7 @@ void LoginDisplayHostMojo::ShowGaiaDialog(bool can_close,
 
   ShowGaiaDialogCommon(prefilled_account);
 
-  dialog_->Show();
+  ShowDialog();
 }
 
 void LoginDisplayHostMojo::HideOobeDialog() {
@@ -280,7 +285,7 @@ void LoginDisplayHostMojo::HideOobeDialog() {
   }
 
   LoadWallpaper(focused_pod_account_id_);
-  dialog_->Hide();
+  HideDialog();
 }
 
 void LoginDisplayHostMojo::UpdateOobeDialogSize(int width, int height) {
@@ -451,6 +456,17 @@ void LoginDisplayHostMojo::OnOldEncryptionDetected(
     const UserContext& user_context,
     bool has_incomplete_migration) {}
 
+void LoginDisplayHostMojo::OnCurrentScreenChanged(OobeScreenId current_screen,
+                                                  OobeScreenId new_screen) {
+  DCHECK(dialog_);
+  if (!dialog_->IsVisible())
+    ShowDialog();
+}
+
+void LoginDisplayHostMojo::OnDestroyingOobeUI() {
+  StopObservingOobeUI();
+}
+
 void LoginDisplayHostMojo::LoadOobeDialog() {
   if (dialog_)
     return;
@@ -484,4 +500,45 @@ void LoginDisplayHostMojo::OnChallengeResponseKeysPrepared(
   existing_user_controller_->Login(user_context, chromeos::SigninSpecifics());
 }
 
+void LoginDisplayHostMojo::ShowDialog() {
+  ObserveOobeUI();
+  dialog_->Show();
+}
+
+void LoginDisplayHostMojo::ShowFullScreen() {
+  ObserveOobeUI();
+  dialog_->ShowFullScreen();
+}
+
+void LoginDisplayHostMojo::HideDialog() {
+  // Stop observing so that dialog will not be shown when a screen change
+  // occurs. Screen changes can occur even when the dialog is not shown (e.g.
+  // with hidden error screens).
+  StopObservingOobeUI();
+  dialog_->Hide();
+}
+
+void LoginDisplayHostMojo::ObserveOobeUI() {
+  if (added_as_oobe_observer_)
+    return;
+
+  OobeUI* oobe_ui = GetOobeUI();
+  if (!oobe_ui)
+    return;
+
+  oobe_ui->AddObserver(this);
+  added_as_oobe_observer_ = true;
+}
+
+void LoginDisplayHostMojo::StopObservingOobeUI() {
+  if (!added_as_oobe_observer_)
+    return;
+
+  added_as_oobe_observer_ = false;
+
+  OobeUI* oobe_ui = GetOobeUI();
+  if (oobe_ui)
+    oobe_ui->RemoveObserver(this);
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/login/ui/login_display_host_mojo.h

@@ -18,6 +18,7 @@
 #include "chrome/browser/chromeos/login/ui/login_display_host_common.h"
 #include "chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.h"
 #include "chrome/browser/ui/ash/login_screen_client.h"
+#include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
 #include "chromeos/login/auth/auth_status_consumer.h"
 #include "chromeos/login/auth/challenge_response_key.h"
 
@@ -34,7 +35,8 @@ class MojoSystemInfoDispatcher;
 // screen.
 class LoginDisplayHostMojo : public LoginDisplayHostCommon,
                              public LoginScreenClient::Delegate,
-                             public AuthStatusConsumer {
+                             public AuthStatusConsumer,
+                             public OobeUI::Observer {
  public:
   LoginDisplayHostMojo();
   ~LoginDisplayHostMojo() override;
@@ -126,6 +128,11 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
   void OnOldEncryptionDetected(const UserContext& user_context,
                                bool has_incomplete_migration) override;
 
+  // OobeUI::Observer:
+  void OnCurrentScreenChanged(OobeScreenId current_screen,
+                              OobeScreenId new_screen) override;
+  void OnDestroyingOobeUI() override;
+
  private:
   void LoadOobeDialog();
 
@@ -137,6 +144,17 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
       base::OnceCallback<void(bool)> on_auth_complete_callback,
       std::vector<ChallengeResponseKey> challenge_response_keys);
 
+  // Helper methods to show and hide the dialog.
+  void ShowDialog();
+  void ShowFullScreen();
+  void HideDialog();
+
+  // Adds this as a |OobeUI::Observer| if it has not already been added as one.
+  void ObserveOobeUI();
+
+  // Removes this as a |OobeUI::Observer| if it has been added as an observer.
+  void StopObservingOobeUI();
+
   // State associated with a pending authentication attempt.
   struct AuthState {
     AuthState(AccountId account_id, base::OnceCallback<void(bool)> callback);
@@ -181,6 +199,9 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
 
   SecurityTokenPinDialogHostAshImpl security_token_pin_dialog_host_ash_impl_;
 
+  // Set if this has been added as a |OobeUI::Observer|.
+  bool added_as_oobe_observer_ = false;
+
   base::WeakPtrFactory<LoginDisplayHostMojo> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LoginDisplayHostMojo);

chromeos/test/data/oobe_configuration/TestLeaveWelcomeScreen.json

 {
-  "welcomeNext": true,
-}
+  "welcomeNext": true
+}
\ No newline at end of file

chromeos/test/data/oobe_configuration/TestShowHID.json

+{
+  "welcomeNext" : false,
+  "desc" : "Skip welcomeNext as this will trigger an action on the hidden welcome screen"
+}
\ No newline at end of file