Skip to content

Commit

Permalink
CrOS Settings: Fix direct navigation to About > Additional details page
Browse files Browse the repository at this point in the history
Cherry-pick to M103.

The bug was introduced in http://crrev.com/c/3579664 where the order
of MainPageBehavior and RouteObserverBehavior were swapped, causing
their respective attached() method call orders to flip.

Restoring the original order of the behaviors calls the respective
attached() methods in the correct order and initializes `this.scroller` before it is read. Given the new behavior precedence, super.currentRouteChanged() will undesirably call
RouteObserverBehavior.currentRouteChanged(). Directly calling MainPageBehavior.currentRouteChanged() works around this and restores
the original implementation.

(cherry picked from commit a60f51e)

Bug: chromium:1324103
Test: out/Default/browser_tests --gtest_filter='*OsSettingsUi2*'
Test: (Manual) Page is reachable and operable via direct link
Test: (Manual) Page is reachable and operable via search result
Test: (Manual) Page is reachable and operable via menu navigation
Change-Id: Ibb30663ca3fc0b08c14c5a51d666a3df102eadeb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645673
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Wes Okuhara <wesokuhara@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1003412}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3652482
Cr-Commit-Position: refs/branch-heads/5060@{#68}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Wes Okuhara authored and Chromium LUCI CQ committed May 18, 2022
1 parent df14c5c commit ab0ba06
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,16 @@ import {AboutPageBrowserProxy, AboutPageBrowserProxyImpl, AboutPageUpdateInfo, B
* @extends {PolymerElement}
* @implements {DeepLinkingBehaviorInterface}
* @implements {WebUIListenerBehaviorInterface}
* @implements {RouteObserverBehaviorInterface}
* @implements {MainPageBehaviorInterface}
* @implements {RouteObserverBehaviorInterface}
* @implements {I18nBehaviorInterface}
*/
const OsSettingsAboutPageBase = mixinBehaviors(
[
DeepLinkingBehavior,
WebUIListenerBehavior,
RouteObserverBehavior,
MainPageBehavior,
RouteObserverBehavior,
I18nBehavior,
],
PolymerElement);
Expand Down Expand Up @@ -314,7 +314,12 @@ class OsSettingsAboutPageElement extends OsSettingsAboutPageBase {
* @param {!Route=} oldRoute
*/
currentRouteChanged(newRoute, oldRoute) {
super.currentRouteChanged(newRoute, oldRoute);
// super.currentRouteChanged() does not produce desired results since
// RouteObserverBehavior has higher precedence than MainPageBehavior given
// this element's behavior list order. In order to trigger the
// MainPageBehavior method, we must directly call it.
// See https://crbug.com/1324103 for more details.
MainPageBehavior.currentRouteChanged.call(this, newRoute, oldRoute);

// Does not apply to this page.
if (newRoute !== routes.ABOUT_ABOUT) {
Expand Down
25 changes: 16 additions & 9 deletions chrome/test/data/webui/settings/chromeos/os_settings_ui_test_2.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,7 @@ import {FakeContactManager} from '../../nearby_share/shared/fake_nearby_contact_
import {FakeNearbyShareSettings} from '../../nearby_share/shared/fake_nearby_share_settings.m.js';

import {FakeUserActionRecorder} from './fake_user_action_recorder.js';

/**
* Checks whether a given element is visible to the user.
* @param {!Element} element
* @returns {boolean}
*/
function isVisible(element) {
return !!(element && element.getBoundingClientRect().width > 0);
}
import {isVisible} from './test_util.js';

suite('os-settings-ui', () => {
let ui;
Expand Down Expand Up @@ -210,6 +202,21 @@ suite('os-settings-ui', () => {
assertEquals(aboutSection, aboutPage.shadowRoot.activeElement);
});

test(
'Detailed build info page is directly navigable and renders',
async () => {
const router = Router.getInstance();
router.navigateTo(routes.DETAILED_BUILD_INFO);

const settingsMain = ui.shadowRoot.querySelector('os-settings-main');
const aboutPage =
settingsMain.shadowRoot.querySelector('os-settings-about-page');
const detailedBuildInfoPage =
aboutPage.shadowRoot.querySelector('settings-detailed-build-info');
await waitBeforeNextRender(detailedBuildInfoPage);
assertTrue(isVisible(detailedBuildInfoPage));
});

test('userActionRouteChange', function() {
assertEquals(userActionRecorder.navigationCount, 0);
Router.getInstance().navigateTo(routes.BASIC);
Expand Down
15 changes: 15 additions & 0 deletions chrome/test/data/webui/settings/chromeos/test_util.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/**
* Returns whether the element both exists and is visible.
* @param {?Element} element
* @return {boolean}
*/
export function isVisible(element) {
// offsetWidth and offsetHeight reflect more ways that an element could be
// hidden, compared to checking the hidden attribute directly.
return !!element && element.getBoundingClientRect().width > 0 &&
element.getBoundingClientRect().height > 0;
}

0 comments on commit ab0ba06

Please sign in to comment.