Reland "OOBE: Arc-ToS Improvements"

This is a reland of ddd08c52

Original change's description:
> OOBE: Arc-ToS Improvements
>
> Migrate ArcToSScreen to multistep behavior.
> Get rid of display: none; and display: block;
>
> Bug: 1156667
> Change-Id: I4c2fc425edd4883c5edfceb0a5a9f67a6227de7b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2603744
> Commit-Queue: Ossama Mahmoud <osamafathy@google.com>
> Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
> Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#844554}

Bug: 1156667
Change-Id: I53e9d533c9e3b7d5b5ba44dcf55997a3d9f6fab6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637560Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: Ossama Mahmoud <osamafathy@google.com>
Cr-Commit-Position: refs/heads/master@{#845620}

chrome/browser/chromeos/login/arc_terms_of_service_browsertest.cc

@@ -41,6 +41,7 @@
 #include "chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
+#include "chrome/test/base/interactive_test_utils.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/dbus/session_manager/fake_session_manager_client.h"
 #include "components/arc/arc_prefs.h"
@@ -97,11 +98,11 @@ const test::UIPath kArcReviewSettingsCheckbox = {kArcTosID,
                                                  "arcReviewSettingsCheckbox"};
 const test::UIPath kArcTosAcceptButton = {kArcTosID, "arcTosAcceptButton"};
 const test::UIPath kArcTosBackButton = {kArcTosID, "arcTosBackButton"};
-const test::UIPath kArcTosDialog = {kArcTosID, "arcTosDialog"};
 const test::UIPath kArcTosNextButton = {kArcTosID, "arcTosNextButton"};
 const test::UIPath kArcTosOverlayWebview = {kArcTosID, "arcTosOverlayWebview"};
 const test::UIPath kArcTosRetryButton = {kArcTosID, "arcTosRetryButton"};
 const test::UIPath kArcTosView = {kArcTosID, "arcTosView"};
+const test::UIPath kArcTosDialog = {kArcTosID, "arcTosDialog"};
 
 ArcPlayTermsOfServiceConsent BuildArcPlayTermsOfServiceConsent(bool accepted) {
   ArcPlayTermsOfServiceConsent play_consent;
@@ -261,9 +262,7 @@ class ArcTermsOfServiceScreenTest : public OobeBaseTest {
 
   void WaitForTermsOfServiceWebViewToLoad() {
     OobeScreenWaiter(ArcTermsOfServiceScreenView::kScreenId).Wait();
-    test::OobeJS()
-        .CreateHasClassWaiter(true, "arc-tos-loaded", kArcTosDialog)
-        ->Wait();
+    test::OobeJS().CreateVisibilityWaiter(true, kArcTosDialog)->Wait();
   }
 
   void WaitForScreenExitResult() {
@@ -533,6 +532,25 @@ IN_PROC_BROWSER_TEST_F(ArcTermsOfServiceScreenTest, RetryAndBackButtonClicked) {
               1)));
 }
 
+IN_PROC_BROWSER_TEST_F(ArcTermsOfServiceScreenTest, NextButtonFocused) {
+  TriggerArcTosScreen();
+  WaitForTermsOfServiceWebViewToLoad();
+  test::OobeJS().CreateFocusWaiter(kArcTosNextButton)->Wait();
+
+  // TODO(crbug/1167720): Make this a method of JSChecker
+  ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
+      nullptr, ui::VKEY_RETURN, false /* control */, false /* shift */,
+      false /* alt */, false /* command */));
+  test::OobeJS().CreateVisibilityWaiter(true, kArcTosAcceptButton)->Wait();
+
+  EXPECT_THAT(histogram_tester_.GetAllSamples(
+                  "OOBE.ArcTermsOfServiceScreen.UserActions"),
+              ElementsAre(base::Bucket(
+                  static_cast<int>(
+                      ArcTermsOfServiceScreen::UserAction::kNextButtonClicked),
+                  1)));
+}
+
 // There are two checkboxes for enabling/disabling arc backup restore and
 // arc location service. This parameterized test executes all 4 combinations
 // of enabled/disabled states and checks that advancing to the next screen by

chrome/browser/chromeos/login/oobe_interactive_ui_test.cc

@@ -80,6 +80,8 @@ using net::test_server::HttpResponse;
 namespace chromeos {
 namespace {
 
+constexpr char kArcTosID[] = "arc-tos";
+const test::UIPath kArcTosDialog = {kArcTosID, "arcTosDialog"};
 enum class ArcState { kNotAvailable, kAcceptTerms };
 
 std::string ArcStateToString(ArcState arc_state) {
@@ -151,10 +153,7 @@ void WaitForGaiaSignInScreen(bool arc_available) {
   // TODO(https://crbug/com/959902): Fix ARC terms of service screen to better
   //     handle this case.
   if (arc_available) {
-    test::OobeJS()
-        .CreateHasClassWaiter(true, "arc-tos-loaded",
-                              {"arc-tos", "arcTosDialog"})
-        ->Wait();
+    test::OobeJS().CreateVisibilityWaiter(true, kArcTosDialog)->Wait();
   }
 
   LOG(INFO) << "OobeInteractiveUITest: Switched to 'gaia-signin' screen.";

chrome/browser/resources/chromeos/login/arc_terms_of_service.css

@@ -18,27 +18,7 @@ cr-checkbox {
   --cr-checkbox-label-padding-start: 16px;
 }
 
-.arc-tos-loaded .arc-tos-loading,
-.arc-tos-loaded .arc-tos-error,
-.arc-tos-loaded #arcTosRetryButton,
-.arc-tos-loading .arc-tos-content,
-.arc-tos-loading .arc-tos-error,
-.arc-tos-loading #arcTosRetryButton,
-.arc-tos-loading #arcTosAcceptButton,
-.error .arc-tos-content,
-.error .arc-tos-loading,
-.error #arcTosAcceptButton,
-.error #arcTosNextButton {
-  display: none;
-}
-
-.arc-tos-loading #arcTosNextButton,
-.arc-tos-loading #arcTosRetryButton {
-  pointer-events: none;
-}
-
-.arc-tos-loading p,
-.error p {
+#errorMessage {
   color: rgba(0, 0, 0, 0.87);
   font-size: 16px;
   font-weight: 400;
@@ -61,33 +41,17 @@ cr-checkbox {
   font-weight: 600;
 }
 
-
 #arcTosContainer::-webkit-scrollbar {
   display: none;
 }
 
-#arcTosView {
-  display: block;
-  height: 100%;
-  margin: auto;
-  min-height: 40px;
-  padding: 0;
-  width: 100%;
-}
-
-#arcTosViewContainer {
-  box-sizing: border-box;
-  margin: 0;
-  padding: 0;
-}
-
 #arcTosBackButton {
   padding-inline-end: 6px;
 }
 
 #arcTosOverlayWebview {
   border: 1px solid #d9d9d9;
-  display: block;
+  display: flex;
   height: 300px;
   width: 100%;
 }

chrome/browser/resources/chromeos/login/arc_terms_of_service.html

@@ -19,21 +19,69 @@
   <template>
     <style include="oobe-dialog-host"></style>
     <link rel="stylesheet" href="arc_terms_of_service.css">
+
+    <!-- LOADING DIALOG -->
+    <oobe-dialog id="arcTosLoadingDialog" has-buttons for-step="loading"
+        role="dialog"
+        aria-label$="[[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]">
+      <h1 slot="title">
+        [[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]
+      </h1>
+      <p slot="subtitle">
+        [[i18nDynamic(locale, 'arcTermsOfServiceScreenDescription')]]
+      </p>
+      <iron-icon src="chrome://oobe/playstore.svg" slot="oobe-icon">
+      </iron-icon>
+      <div slot="footer"
+          class="arc-tos-loading flex layout center-justified vertical">
+        <throbber-notice text-key="arcTermsOfServiceLoading">
+        </throbber-notice>
+      </div>
+    </oobe-dialog>
+
+    <!-- ERROR DIALOG -->
+    <oobe-dialog id="arcTosErrorDialog" has-buttons for-step="error"
+        role="dialog"
+        aria-label$="[[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]">
+      <h1 slot="title">
+        [[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]
+      </h1>
+      <p slot="subtitle">
+        [[i18nDynamic(locale, 'arcTermsOfServiceScreenDescription')]]
+      </p>
+      <iron-icon src="chrome://oobe/playstore.svg" slot="oobe-icon">
+      </iron-icon>
+      <div slot="footer">
+        <p id="errorMessage">
+          [[i18nDynamic(locale, 'arcTermsOfServiceError')]]
+        </p>
+      </div>
+      <div slot="bottom-buttons">
+        <oobe-text-button id="arcTosRetryButton" class="focus-on-show"
+            inverse on-click="onRetry_"
+            text-key="arcTermsOfServiceRetryButton"></oobe-text-button>
+      </div>
+    </oobe-dialog>
+
     <!-- As this dialog have pre-loading logic that require access to elements,
          dialog is marked as no-lazy. -->
-    <oobe-dialog id="arcTosDialog" has-buttons class="arc-tos-loading"
+    <oobe-dialog id="arcTosDialog" has-buttons for-step="loaded"
         role="dialog"
         aria-label$="[[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]"
-        title-key="arcTermsOfServiceScreenHeading"
-        subtitle-key="arcTermsOfServiceScreenDescription" no-lazy>
+        no-lazy>
+        <h1 slot="title">
+          [[i18nDynamic(locale, 'arcTermsOfServiceScreenHeading')]]
+        </h1>
+        <p slot="subtitle">
+          [[i18nDynamic(locale, 'arcTermsOfServiceScreenDescription')]]
+        </p>
       <iron-icon src="chrome://oobe/playstore.svg" slot="oobe-icon">
       </iron-icon>
       <div id="arcTosContainer" slot="footer" class="flex layout vertical">
-        <div id="arcTosViewContainer" class="arc-tos-content flex">
-          <webview id="arcTosView" allowTransparency
-              class="oobe-tos-webview"
-              on-contentload="onTermsViewContentLoad_"></webview>
-        </div>
+        <webview id="arcTosView" allowTransparency
+            role="document" class="flex oobe-tos-webview"
+            on-contentload="onTermsViewContentLoad_">
+        </webview>
         <div id="arcPolicyLink" class="arc-tos-content"
             on-click="onPolicyLinkClick_">
           <a class="oobe-local-link" is="action-link">
@@ -113,24 +161,13 @@
             </cr-checkbox>
           </div>
         </div>
-        <div class="arc-tos-loading flex layout center-justified vertical">
-          <throbber-notice text-key="arcTermsOfServiceLoading">
-          </throbber-notice>
-        </div>
-        <div class="arc-tos-error">
-          <p>[[i18nDynamic(locale, 'arcTermsOfServiceError')]]</p>
-        </div>
       </div>
       <div slot="back-navigation">
         <oobe-back-button id="arcTosBackButton" hidden="[[!demoMode]]"
             on-click="onBack_"></oobe-back-button>
       </div>
       <div slot="bottom-buttons">
-        <oobe-text-button id="arcTosRetryButton"
-            inverse on-click="onRetry_"
-            disabled="[[arcTosButtonsDisabled]]"
-            text-key="arcTermsOfServiceRetryButton"></oobe-text-button>
-        <oobe-text-button id="arcTosNextButton"
+        <oobe-text-button id="arcTosNextButton" class="focus-on-show"
             inverse on-click="onNext_"
             disabled="[[arcTosButtonsDisabled]]"
             hidden="[[showFullDialog]]"
@@ -187,7 +224,8 @@
     <oobe-modal-dialog id="arcTosOverlayPrivacyPolicy"
         on-close="onOverlayClosed_">
       <div slot="content" id = "arcTosOverlayWebviewContainer">
-        <webview id="arcTosOverlayWebview" hidden="[[overlayLoading_]]"
+        <webview id="arcTosOverlayWebview"
+            hidden="[[overlayLoading_]]" class="flex oobe-tos-webview"
             on-contentload="onAcrTosOverlayContentLoad_">
         </webview>
         <throbber-notice class="flex layout center-justified vertical"

chrome/browser/resources/chromeos/login/arc_terms_of_service.js

@@ -6,11 +6,21 @@
  * @fileoverview Polymer element for displaying material design for ARC Terms Of
  * Service screen.
  */
+'use strict';
+
+(function() {
+
+// Enum that describes the current state of the Arc Terms Of Service screen
+const UIState = {
+  LOADING: 'loading',
+  LOADED: 'loaded',
+  ERROR: 'error',
+};
 
 Polymer({
   is: 'arc-tos-element',
 
-  behaviors: [OobeI18nBehavior, OobeDialogHostBehavior, LoginScreenBehavior],
+  behaviors: [OobeI18nBehavior, MultiStepBehavior, LoginScreenBehavior],
 
   EXTERNAL_API: [
     'setMetricsMode',
@@ -180,6 +190,13 @@ Polymer({
    */
   termsOfServiceHostName_: 'https://play.google.com',
 
+
+  defaultUIStep() {
+    return UIState.LOADING;
+  },
+
+  UI_STEPS: UIState,
+
   /** @override */
   ready() {
     this.initializeLoginScreen('ArcTermsOfServiceScreen', {
@@ -201,7 +218,6 @@ Polymer({
    * Event handler that is invoked just before the screen is shown.
    */
   onBeforeShow() {
-    this.focusButton_();
     this.is_shown_ = true;
     window.setTimeout(this.applyOobeConfiguration_.bind(this), 0);
 
@@ -267,20 +283,6 @@ Polymer({
     this.$.arcTosNextButton.focus();
   },
 
-  focusButton_() {
-    var id;
-    if (this.hasClass_('arc-tos-loaded')) {
-      id = 'arcTosNextButton';
-    } else if (this.hasClass_('error')) {
-      id = 'arcTosRetryButton';
-    }
-
-    if (typeof id === 'undefined')
-      return;
-
-    Polymer.RenderStatus.afterNextRender(this, () => this.$[id].focus());
-  },
-
   /**
    * Makes sure that UI is initialized.
    *
@@ -410,7 +412,7 @@ Polymer({
     countryCode = countryCode.toLowerCase();
 
     if (this.language_ && this.language_ == language && this.countryCode_ &&
-        this.countryCode_ == countryCode && !this.classList.contains('error') &&
+        this.countryCode_ == countryCode && this.uiStep != UIState.ERROR &&
         !this.usingOfflineTerms_ && this.tosContent_) {
       this.enableButtons_(true);
       return;
@@ -436,7 +438,7 @@ Polymer({
 
     // Try to use currently loaded document first.
     var self = this;
-    if (termsView.src != '' && this.classList.contains('arc-tos-loaded')) {
+    if (termsView.src != '' && this.isLoaded_()) {
       var navigateScript = 'processLangZoneTerms(true, \'' + language +
           '\', \'' + countryCode + '\');';
       termsView.executeScript({code: navigateScript}, function(results) {
@@ -489,7 +491,7 @@ Polymer({
    * @param {boolean} child whether current account is a child account.
    */
   setArcManaged(managed, child) {
-    this.$.arcTosViewContainer.hidden = managed;
+    this.$.arcTosView.hidden = managed;
     this.isChild = child;
   },
 
@@ -531,9 +533,7 @@ Polymer({
     this.usingOfflineTerms_ = false;
     var termsView = this.$.arcTosView;
     termsView.src = this.termsOfServiceHostName_ + '/about/play-terms.html';
-    this.removeClass_('arc-tos-loaded');
-    this.removeClass_('error');
-    this.addClass_('arc-tos-loading');
+    this.setUIStep(UIState.LOADING);
     this.enableButtons_(false);
   },
 
@@ -551,36 +551,6 @@ Polymer({
     this.demoMode = false;
   },
 
-  /**
-   * Adds new class to the list of classes of root OOBE style.
-   * @param {string} className class to remove.
-   *
-   * @private
-   */
-  addClass_(className) {
-    this.$.arcTosDialog.classList.add(className);
-  },
-
-  /**
-   * Removes class from the list of classes of root OOBE style.
-   * @param {string} className class to remove.
-   *
-   * @private
-   */
-  removeClass_(className) {
-    this.$.arcTosDialog.classList.remove(className);
-  },
-
-  /**
-   * Checks if class exists in the list of classes of root OOBE style.
-   * @param {string} className class to check.
-   *
-   * @private
-   */
-  hasClass_(className) {
-    return this.$.arcTosDialog.classList.contains(className);
-  },
-
   /**
    * Returns a match pattern compatible version of termsOfServiceHostName_ by
    * stripping the port number part of the hostname. During tests
@@ -642,10 +612,7 @@ Polymer({
    * @private
    */
   setTermsViewContentLoadedState_() {
-    this.removeClass_('arc-tos-loading');
-    this.removeClass_('error');
-    this.addClass_('arc-tos-loaded');
-
+    this.setUIStep(UIState.LOADED);
     this.enableButtons_(true);
     this.showFullDialog = false;
     this.$.arcTosNextButton.focus();
@@ -673,9 +640,7 @@ Polymer({
    */
   showError_() {
     this.termsError = true;
-    this.removeClass_('arc-tos-loading');
-    this.removeClass_('arc-tos-loaded');
-    this.addClass_('error');
+    this.setUIStep(UIState.ERROR);
 
     this.enableButtons_(true);
     this.$.arcTosRetryButton.focus();
@@ -706,9 +671,7 @@ Polymer({
    * Shows loading screen for debugging purpose
    */
   showLoadingScreenForTesting() {
-    this.removeClass_('arc-tos-loaded');
-    this.removeClass_('error');
-    this.addClass_('arc-tos-loading');
+    this.setUIStep(UIState.LOADING);
     this.enableButtons_(false);
   },
 
@@ -814,5 +777,6 @@ Polymer({
       this.lastFocusedElement_.focus();
       this.lastFocusedElement_ = null;
     }
-  }
+  },
 });
+})();