Skip to content

Commit

Permalink
[118 Merge] Fix warning dialog when updating on metered connection
Browse files Browse the repository at this point in the history
As evident in the feedback reports in the bug, devices that are on a
metered connection (cellular, tether) are unable to update. In this
case, a warning dialog should show which prompts the user to consent
to downloading the update over their metered connection. However,
that warning dialog was not showing due to a misuse of innerHTML and
resulted in the device unable to update as long as they were on the
metered connection.

This CL avoids using innerHTML, the source of the error, altogether
and uses a plain element property instead for displaying the warning
message.

Dialog: http://screen/BiP7p4qLNCUvj7P.png

(cherry picked from commit a7309a5)

Bug: b:304807561
Test: browser_tests --gtest_filter="OSSettingsAboutPage*"
Change-Id: Ic2dc27ad8263a16739b811854ea3529d6143a50d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4932690
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Wes Okuhara <wesokuhara@google.com>
Commit-Queue: Wes Okuhara <wesokuhara@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1209051}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4936767
Cr-Commit-Position: refs/branch-heads/5993@{#1266}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}
  • Loading branch information
Wes Okuhara authored and Chromium LUCI CQ committed Oct 13, 2023
1 parent f739555 commit c761b6c
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 20 deletions.
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<style include="settings-shared"></style>
<cr-dialog id="dialog" close-text="$i18n{close}">
<cr-dialog id="dialog" close-text="$i18n{close}" show-on-attach>
<div slot="title">$i18n{aboutUpdateWarningTitle}</div>
<div slot="body">
<div id="update-warning-message"></div>
[[warningMessage_]]
</div>
<div slot="button-container">
<cr-button id="cancel" class="cancel-button"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ import {CrDialogElement} from 'chrome://resources/cr_elements/cr_dialog/cr_dialo
import {I18nMixin} from 'chrome://resources/cr_elements/i18n_mixin.js';
import {PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';

import {castExists} from '../assert_extras.js';

import {AboutPageBrowserProxy, AboutPageBrowserProxyImpl, AboutPageUpdateInfo} from './about_page_browser_proxy.js';
import {getTemplate} from './update_warning_dialog.html.js';

Expand All @@ -42,28 +40,28 @@ class SettingsUpdateWarningDialogElement extends
return {
updateInfo: {
type: Object,
observer: 'updateInfoChanged_',
observer: 'onUpdateInfoChanged_',
},

warningMessage_: {
type: String,
value: '',
},
};
}

updateInfo?: AboutPageUpdateInfo;

private browserProxy_: AboutPageBrowserProxy;
private warningMessage_: string;

constructor() {
super();

this.browserProxy_ = AboutPageBrowserProxyImpl.getInstance();
}

override connectedCallback() {
super.connectedCallback();

this.$.dialog.showModal();
}

private onCancelClick_() {
private onCancelClick_(): void {
this.$.dialog.close();
}

Expand All @@ -77,15 +75,13 @@ class SettingsUpdateWarningDialogElement extends
this.$.dialog.close();
}

private updateInfoChanged_() {
private onUpdateInfoChanged_(): void {
if (!this.updateInfo || this.updateInfo.size === undefined) {
console.warn('ERROR: Update size is undefined');
return;
}

const warningMessage =
castExists(this.shadowRoot!.getElementById('update-warning-message'));
warningMessage.innerHTML = this.i18n(
this.warningMessage_ = this.i18n(
'aboutUpdateWarningMessage',
// Convert bytes to megabytes
Math.floor(Number(this.updateInfo.size) / (1024 * 1024)));
Expand Down
29 changes: 25 additions & 4 deletions chrome/test/data/webui/settings/chromeos/os_about_page_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,23 @@ suite('AboutPageTest', function() {
* @param {!UpdateStatus} status
* @param {{
* progress: number|undefined,
* message: string|undefined
* rollback: bool|undefined
* powerwash: bool|undefined
* message: string|undefined,
* rollback: bool|undefined,
* powerwash: bool|undefined,
* version: string|undefined,
* size: string|undefined,
* }} opt_options
*/
function fireStatusChanged(status, opt_options) {
const options = opt_options || {};
webUIListenerCallback('update-status-changed', {
progress: options.progress === undefined ? 1 : options.progress,
message: options.message,
status: status,
status,
rollback: options.rollback,
powerwash: options.powerwash,
version: options.version,
size: options.size,
});
}

Expand Down Expand Up @@ -255,6 +259,23 @@ suite('AboutPageTest', function() {
assertEquals(expectedMessage, statusMessageEl.textContent);
});

test(
'Warning dialog is shown when attempting to update over metered network',
async () => {
await initNewPage();

fireStatusChanged(
UpdateStatus.NEED_PERMISSION_TO_UPDATE,
{version: '9001.0.0', size: '9999'});
flush();

const warningDialog =
page.shadowRoot.querySelector('settings-update-warning-dialog');
assertTrue(!!warningDialog);
assertTrue(
warningDialog.$.dialog.open, 'Warning dialog should be open');
});

test('NoInternet', function() {
assertTrue(page.$.updateStatusMessage.hidden);
aboutBrowserProxy.sendStatusNoInternet();
Expand Down

0 comments on commit c761b6c

Please sign in to comment.