Settings: Combine safetyCheck and privacy sections in re-design.
- Hide the "Safety check" menu entry
- Add a new |nest-under-section| attribute on <settings-section> and
leverage it in main_page_behavior.js to show multiple
<settings-section> instances at the same time.
- Update tests.
Bug: 1204457
Change-Id: Iddb6049343a58162ff1fcd3ca3999ebde7f5e101
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2893208
Reviewed-by: John Lee <johntlee@chromium.org>
Commit-Queue: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#882916}
diff --git a/chrome/browser/resources/settings/basic_page/basic_page.html b/chrome/browser/resources/settings/basic_page/basic_page.html
index 5fce87a..741d47b 100644
--- a/chrome/browser/resources/settings/basic_page/basic_page.html
+++ b/chrome/browser/resources/settings/basic_page/basic_page.html
@@ -98,7 +98,7 @@
<template is="dom-if" if="[[showPage_(pageVisibility.safetyCheck)]]"
restamp>
<settings-section page-title="$i18n{safetyCheckSectionTitle}"
- section="safetyCheck"
+ section="safetyCheck" nest-under-section="privacy"
id="safetyCheckSettingsSection">
<settings-safety-check-page prefs="{{prefs}}">
</settings-safety-check-page>
diff --git a/chrome/browser/resources/settings/route.js b/chrome/browser/resources/settings/route.js
index d1bb364b..fe6ea73 100644
--- a/chrome/browser/resources/settings/route.js
+++ b/chrome/browser/resources/settings/route.js
@@ -143,7 +143,11 @@
r.PRIVACY = r.BASIC.createSection('/privacy', 'privacy');
addPrivacyChildRoutes(r);
- r.SAFETY_CHECK = r.BASIC.createSection('/safetyCheck', 'safetyCheck');
+ if (loadTimeData.getBoolean('enableLandingPageRedesign')) {
+ r.SAFETY_CHECK = r.PRIVACY.createSection('/safetyCheck', 'safetyCheck');
+ } else {
+ r.SAFETY_CHECK = r.BASIC.createSection('/safetyCheck', 'safetyCheck');
+ }
}
// <if expr="not chromeos and not lacros">
diff --git a/chrome/browser/resources/settings/settings_menu/settings_menu.html b/chrome/browser/resources/settings/settings_menu/settings_menu.html
index 2ecb987..eace8e27 100644
--- a/chrome/browser/resources/settings/settings_menu/settings_menu.html
+++ b/chrome/browser/resources/settings/settings_menu/settings_menu.html
@@ -111,12 +111,14 @@
<iron-icon icon="settings:assignment"></iron-icon>
$i18n{autofillPageTitle}
</a>
- <a role="menuitem" href="/safetyCheck"
- hidden="[[!pageVisibility.safetyCheck]]"
- id="safetyCheck">
- <iron-icon icon="settings20:safety-check"></iron-icon>
- $i18n{safetyCheckSectionTitle}
- </a>
+ <template is="dom-if" if="[[!enableLandingPageRedesign_]]">
+ <a role="menuitem" href="/safetyCheck"
+ hidden="[[!pageVisibility.safetyCheck]]"
+ id="safetyCheck">
+ <iron-icon icon="settings20:safety-check"></iron-icon>
+ $i18n{safetyCheckSectionTitle}
+ </a>
+ </template>
<a role="menuitem" href="/privacy"
hidden="[[!pageVisibility.privacy]]">
<iron-icon icon="cr:security"></iron-icon>
diff --git a/chrome/browser/resources/settings/settings_menu/settings_menu.js b/chrome/browser/resources/settings/settings_menu/settings_menu.js
index a10c4d72..c1e430e 100644
--- a/chrome/browser/resources/settings/settings_menu/settings_menu.js
+++ b/chrome/browser/resources/settings/settings_menu/settings_menu.js
@@ -13,13 +13,12 @@
import 'chrome://resources/polymer/v3_0/iron-collapse/iron-collapse.js';
import 'chrome://resources/polymer/v3_0/iron-icon/iron-icon.js';
import 'chrome://resources/polymer/v3_0/iron-selector/iron-selector.js';
-import '../i18n_setup.js';
import '../icons.js';
import '../settings_shared_css.js';
import {assert} from 'chrome://resources/js/assert.m.js';
import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
-
+import {loadTimeData} from '../i18n_setup.js';
import {PageVisibility} from '../page_visibility.js';
import {Route, RouteObserverBehavior, Router} from '../router.js';
@@ -42,6 +41,12 @@
* @type {!PageVisibility}
*/
pageVisibility: Object,
+
+ /** @private */
+ enableLandingPageRedesign_: {
+ type: Boolean,
+ value: () => loadTimeData.getBoolean('enableLandingPageRedesign'),
+ },
},
/** @param {!Route} newRoute */
diff --git a/chrome/browser/resources/settings/settings_page/main_page_behavior.js b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
index 746b2e7..333dccfc 100644
--- a/chrome/browser/resources/settings/settings_page/main_page_behavior.js
+++ b/chrome/browser/resources/settings/settings_page/main_page_behavior.js
@@ -199,6 +199,41 @@
},
/**
+ * Finds the settings-section instances corresponding to the given route. If
+ * the section is lazily loaded it force-renders it.
+ * Note: If the section resides within "advanced" settings, a
+ * 'hide-container' event is fired (necessary to avoid flashing). Callers
+ * are responsible for firing a 'show-container' event.
+ * @param {!Route} route
+ * @return {!Promise<!Array<!SettingsSectionElement>>}
+ * @private
+ */
+ ensureSectionsForRoute_(route) {
+ const sections = this.querySettingsSections_(route.section);
+ if (sections.length > 0) {
+ return Promise.resolve(sections);
+ }
+
+ // The function to use to wait for <dom-if>s to render.
+ const waitFn = beforeNextRender.bind(null, this);
+
+ return new Promise(resolve => {
+ if (this.shouldExpandAdvanced_(route)) {
+ this.fire('hide-container');
+ waitFn(() => {
+ this.$$('#advancedPageTemplate').get().then(() => {
+ resolve(this.querySettingsSections_(route.section));
+ });
+ });
+ } else {
+ waitFn(() => {
+ resolve(this.querySettingsSections_(route.section));
+ });
+ }
+ });
+ },
+
+ /**
* @param {!Route} route
* @private
*/
@@ -255,19 +290,23 @@
},
/**
- * Shows the section corresponding to |newRoute| and hides the previously
- * |active| section (if any).
+ * Shows the section(s) corresponding to |newRoute| and hides the previously
+ * |active| section(s), if any.
* @param {!Route} newRoute
*/
- switchToSection_(newRoute) {
- this.ensureSectionForRoute_(newRoute).then(section => {
+ switchToSections_(newRoute) {
+ this.ensureSectionsForRoute_(newRoute).then(sections => {
// Clear any previously |active| section.
- const oldSection = this.$$(`settings-section[active]`);
- if (oldSection) {
- oldSection.toggleAttribute('active', false);
+ const oldSections =
+ this.shadowRoot.querySelectorAll(`settings-section[active]`);
+ for (const s of oldSections) {
+ s.toggleAttribute('active', false);
}
- section.toggleAttribute('active', true);
+ for (const s of sections) {
+ s.toggleAttribute('active', true);
+ }
+
this.fire('show-container');
});
},
@@ -431,13 +470,13 @@
processTransitionRedesign_(oldRoute, newRoute, oldState, newState) {
if (oldState === RouteState.TOP_LEVEL) {
if (newState === RouteState.SECTION) {
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
} else if (newState === RouteState.SUBPAGE) {
this.enterSubpage_(newRoute);
} else if (newState === RouteState.TOP_LEVEL) {
// Case when navigating from '/?search=foo' to '/' (clearing search
// results).
- this.switchToSection_(TOP_LEVEL_EQUIVALENT_ROUTE);
+ this.switchToSections_(TOP_LEVEL_EQUIVALENT_ROUTE);
}
// Nothing to do here for the case of RouteState.DIALOG.
return;
@@ -445,12 +484,12 @@
if (oldState === RouteState.SECTION) {
if (newState === RouteState.SECTION) {
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
} else if (newState === RouteState.SUBPAGE) {
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
this.enterSubpage_(newRoute);
} else if (newState === RouteState.TOP_LEVEL) {
- this.switchToSection_(TOP_LEVEL_EQUIVALENT_ROUTE);
+ this.switchToSections_(TOP_LEVEL_EQUIVALENT_ROUTE);
this.scroller.scrollTop = 0;
}
// Nothing to do here for the case of RouteState.DIALOG.
@@ -460,7 +499,7 @@
if (oldState === RouteState.SUBPAGE) {
if (newState === RouteState.SECTION) {
this.enterMainPage_(/** @type {!Route} */ (oldRoute));
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
} else if (newState === RouteState.SUBPAGE) {
// Handle case where the two subpages belong to
// different sections, but are linked to each other. For example
@@ -488,12 +527,12 @@
if (oldState === RouteState.INITIAL) {
if ([RouteState.SECTION, RouteState.DIALOG].includes(newState)) {
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
} else if (newState === RouteState.SUBPAGE) {
- this.switchToSection_(newRoute);
+ this.switchToSections_(newRoute);
this.enterSubpage_(newRoute);
} else if (newState === RouteState.TOP_LEVEL) {
- this.switchToSection_(TOP_LEVEL_EQUIVALENT_ROUTE);
+ this.switchToSections_(TOP_LEVEL_EQUIVALENT_ROUTE);
}
return;
}
@@ -515,4 +554,24 @@
return /** @type {?SettingsSectionElement} */ (
this.$$(`settings-section[section="${section}"]`));
},
+
+ /*
+ * @param {string} sectionName Section name of the element to get.
+ * @return {!Array<!SettingsSectionElement>}
+ */
+ querySettingsSections_(sectionName) {
+ const result = [];
+ const section = this.getSection(sectionName);
+
+ if (section) {
+ result.push(section);
+ }
+
+ const extraSections = this.shadowRoot.querySelectorAll(
+ `settings-section[nest-under-section="${sectionName}"]`);
+ if (extraSections.length > 0) {
+ result.push(...extraSections);
+ }
+ return result;
+ }
};
diff --git a/chrome/test/data/webui/settings/basic_page_test.js b/chrome/test/data/webui/settings/basic_page_test.js
index 15c52119..7f57821c 100644
--- a/chrome/test/data/webui/settings/basic_page_test.js
+++ b/chrome/test/data/webui/settings/basic_page_test.js
@@ -11,7 +11,7 @@
import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
import {loadTimeData, pageVisibility, Router, routes} from 'chrome://settings/settings.js';
-import {flushTasks, isVisible} from '../test_util.m.js';
+import {eventToPromise, flushTasks, isVisible} from '../test_util.m.js';
// clang-format on
// Register mocha tests.
@@ -60,11 +60,35 @@
suite('SettingsBasicPageRedesign', () => {
let page = null;
- setup(function() {
+ suiteSetup(function() {
+ assertTrue(loadTimeData.getBoolean('enableLandingPageRedesign'));
+ const attribute =
+ loadTimeData.getString('enableLandingPageRedesignAttribute');
+ assertEquals('enable-landing-page-redesign', attribute);
+
+ // Do this manually as it is normally part of settings.html, which is not
+ // part of this test.
+ document.documentElement.toggleAttribute(attribute, true);
+ });
+
+ setup(async function() {
PolymerTest.clearBody();
page = document.createElement('settings-basic-page');
document.body.appendChild(page);
page.scroller = document.body;
+
+ // Need to wait for the 'show-container' event to fire after every
+ // transition, to ensure no logic related to previous transitions is still
+ // running when later transitions are tested.
+ const whenDone = eventToPromise('show-container', page);
+
+ // Ensure that all settings-section instances are rendered.
+ flush();
+ await page.$$('#advancedPageTemplate').get();
+ const sections = page.shadowRoot.querySelectorAll('settings-section');
+ assertTrue(sections.length > 1);
+
+ await whenDone;
});
/** @param {string} section */
@@ -81,22 +105,6 @@
}
test('OnlyOneSectionShown', async () => {
- assertTrue(loadTimeData.getBoolean('enableLandingPageRedesign'));
- const attribute =
- loadTimeData.getString('enableLandingPageRedesignAttribute');
- assertEquals('enable-landing-page-redesign', attribute);
-
-
- // Do this manually as it is normally part of settings.html, which is not
- // part of this test.
- document.documentElement.toggleAttribute(attribute, true);
-
- // Ensure that all settings-section instances are rendered.
- flush();
- await page.$$('#advancedPageTemplate').get();
- const sections = page.shadowRoot.querySelectorAll('settings-section');
- assertTrue(sections.length > 1);
-
// RouteState.INITIAL -> RoutState.TOP_LEVEL
// Check that only one is marked as |active|.
assertActiveSection(routes.PEOPLE.section);
@@ -105,7 +113,9 @@
// RouteState.TOP_LEVEL -> RoutState.SECTION
// Check that navigating to a different route correctly updates the page.
+ let whenDone = eventToPromise('show-container', page);
Router.getInstance().navigateTo(routes.SEARCH);
+ await whenDone;
await flushTasks();
assertActiveSection(routes.SEARCH.section);
assertTrue(!!page.shadowRoot.querySelector(
@@ -128,7 +138,9 @@
}
// RouteState.SECTION -> RoutState.SECTION
+ whenDone = eventToPromise('show-container', page);
Router.getInstance().navigateTo(routes.APPEARANCE);
+ await whenDone;
await flushTasks();
assertActiveSection(routes.APPEARANCE.section);
assertTrue(!!getCardElement());
@@ -136,7 +148,9 @@
assertFalse(!!getSubpage());
// RouteState.SECTION -> RoutState.SUBPAGE
+ whenDone = eventToPromise('show-container', page);
Router.getInstance().navigateTo(routes.FONTS);
+ await whenDone;
await flushTasks();
assertActiveSection(routes.APPEARANCE.section);
assertTrue(!!getCardElement());
@@ -144,7 +158,9 @@
assertTrue(!!getSubpage());
// RouteState.SUBPAGE -> RoutState.SECTION
+ whenDone = eventToPromise('show-container', page);
Router.getInstance().navigateTo(routes.APPEARANCE);
+ await whenDone;
await flushTasks();
assertActiveSection(routes.APPEARANCE.section);
assertTrue(!!getCardElement());
@@ -152,8 +168,29 @@
assertFalse(!!getSubpage());
// RouteState.SECTION -> RoutState.TOP_LEVEL
+ whenDone = eventToPromise('show-container', page);
Router.getInstance().navigateTo(routes.BASIC);
+ await whenDone;
await flushTasks();
assertActiveSection(routes.PEOPLE.section);
});
+
+ // Test cases where a settings-section is appearing next to another section
+ // using the |nest-under-section| attribute. Only one such case currently
+ // exists.
+ test('SometimesMoreSectionsShown', async () => {
+ const whenDone = eventToPromise('show-container', page);
+ Router.getInstance().navigateTo(routes.PRIVACY);
+ await whenDone;
+ await flushTasks();
+
+ const activeSections =
+ page.shadowRoot.querySelectorAll('settings-section[active]');
+ assertEquals(2, activeSections.length);
+ assertEquals(routes.SAFETY_CHECK.section, activeSections[0].section);
+ assertEquals(
+ routes.PRIVACY.section,
+ activeSections[0].getAttribute('nest-under-section'));
+ assertEquals(routes.PRIVACY.section, activeSections[1].section);
+ });
});
