Skip to content

Commit

Permalink
WebUI: Migrate shared custom_element.js to TypeScript.
Browse files Browse the repository at this point in the history
All clients of that file are already migrated to TS.

By migrating to TS, the signature of $() and $all() methods is improved
to leverage generics and propagate types to the underlying
querySelector/querySelectorAll methods.

As part of this change discovered that the DownloadShelf UI was opting
out of some strict TS checks (by setting "strictNullChecks": false).
Removed the flag and fixed violations.

Bug: 1189595
Change-Id: I681d2506f6e6e9d6fb5e6bb46cf0b225ec42b7aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3344388
Auto-Submit: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Commit-Queue: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#952580}
  • Loading branch information
freshp86 authored and Chromium LUCI CQ committed Dec 16, 2021
1 parent 666c69c commit 32de615
Show file tree
Hide file tree
Showing 11 changed files with 57 additions and 69 deletions.
11 changes: 4 additions & 7 deletions chrome/browser/resources/download_shelf/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,22 @@ export class DownloadShelfAppElement extends CustomElement {
constructor() {
super();

/** @private {!DownloadShelfApiProxy} */
this.apiProxy_ = DownloadShelfApiProxyImpl.getInstance();

const showAllButton = this.$('#show-all-button') as HTMLElement;
const showAllButton = this.$<HTMLElement>('#show-all-button')!;
showAllButton.innerText = loadTimeData.getString('showAll');
showAllButton.addEventListener('click', () => this.onShowAll_());

const closeButton = this.$('#close-button');
const closeButton = this.$('#close-button')!;
closeButton.setAttribute('aria-label', loadTimeData.getString('close'));
closeButton.addEventListener('click', () => this.onClose_());
}

/** @private */
onShowAll_() {
private onShowAll_() {
this.apiProxy_.doShowAll();
}

/** @private */
onClose_() {
private onClose_() {
this.apiProxy_.doClose();
}
}
Expand Down
22 changes: 11 additions & 11 deletions chrome/browser/resources/download_shelf/download_item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,20 @@ export class DownloadItemElement extends CustomElement {
this.opened = false;
this.apiProxy_ = DownloadShelfApiProxyImpl.getInstance();

this.$('#shadow-mask')
.addEventListener('click', () => this.onOpenButtonClick_());
this.$('#dropdown-button')
.addEventListener('click', e => this.onDropdownButtonClick_(e));
this.$('#shadow-mask')!.addEventListener(
'click', () => this.onOpenButtonClick_());
this.$('#dropdown-button')!.addEventListener(
'click', e => this.onDropdownButtonClick_(e));
const discardButton = this.$('#discard-button') as HTMLElement;
discardButton.innerText = loadTimeData.getString('discardButtonText');
discardButton.addEventListener('click', () => this.onDiscardButtonClick_());
this.$('#keep-button')
.addEventListener('click', () => this.onKeepButtonClick_());
this.$('#keep-button')!.addEventListener(
'click', () => this.onKeepButtonClick_());
this.addEventListener('contextmenu', e => this.onContextMenu_(e));

this.$('.progress-indicator').addEventListener('animationend', () => {
this.$('.progress-indicator')
.classList.remove('download-complete-animation');
this.$('.progress-indicator')!.addEventListener('animationend', () => {
this.$('.progress-indicator')!.classList.remove(
'download-complete-animation');
});
}

Expand Down Expand Up @@ -132,8 +132,8 @@ export class DownloadItemElement extends CustomElement {
this.progress = 1;
// Only start animation if it's called from OnDownloadUpdated.
if (this.downloadUpdated_) {
this.$('.progress-indicator')
.classList.add('download-complete-animation');
this.$('.progress-indicator')!.classList.add(
'download-complete-animation');
}
break;
case DownloadState.kInterrupted:
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/resources/download_shelf/download_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class DownloadListElement extends CustomElement {
super();

this.apiProxy_ = DownloadShelfApiProxyImpl.getInstance();
this.listElement_ = this.$('#download-list') as HTMLElement;
this.listElement_ = this.$<HTMLElement>('#download-list')!;
this.resizeObserver_ = new ResizeObserver(() => this.updateElements_());
this.resizeObserver_.observe(this.listElement_);

Expand Down Expand Up @@ -123,7 +123,7 @@ export class DownloadListElement extends CustomElement {
private clear_() {
while (this.listenerIds_.length) {
this.apiProxy_.getCallbackRouter().removeListener(
this.listenerIds_.shift());
this.listenerIds_.shift()!);
}

while (this.listElement_.firstChild) {
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/resources/download_shelf/tsconfig_base.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"allowJs": true,
"noUncheckedIndexedAccess": false,
"noUnusedLocals": false,
"strictNullChecks": false,
"strictPropertyInitialization": false
}
}
2 changes: 1 addition & 1 deletion chrome/browser/resources/tab_strip/alert_indicators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export class AlertIndicatorsElement extends CustomElement {
constructor() {
super();

this.containerEl_ = this.$('#container') as HTMLElement;
this.containerEl_ = this.$<HTMLElement>('#container')!;

const audioIndicator = new AlertIndicatorElement();
const recordingIndicator = new AlertIndicatorElement();
Expand Down
20 changes: 10 additions & 10 deletions chrome/browser/resources/tab_strip/tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,29 +61,29 @@ export class TabElement extends CustomElement {
super();

this.alertIndicatorsEl_ =
this.$('tabstrip-alert-indicators') as AlertIndicatorsElement;
// Normally, custom elements will get upgraded automatically once added to
// the DOM, but TabElement may need to update properties on
this.$<AlertIndicatorsElement>('tabstrip-alert-indicators')!;
// Normally, custom elements will get upgraded automatically once added
// to the DOM, but TabElement may need to update properties on
// AlertIndicatorElement before this happens, so upgrade it manually.
customElements.upgrade(this.alertIndicatorsEl_);

this.closeButtonEl_ = this.$('#close') as HTMLElement;
this.closeButtonEl_ = this.$<HTMLElement>('#close')!;
this.closeButtonEl_.setAttribute(
'aria-label', loadTimeData.getString('closeTab'));

this.dragImageEl_ = this.$('#dragImage') as HTMLElement;
this.dragImageEl_ = this.$<HTMLElement>('#dragImage')!;

this.tabEl_ = this.$('#tab') as HTMLElement;
this.tabEl_ = this.$<HTMLElement>('#tab')!;

this.faviconEl_ = this.$('#favicon') as HTMLElement;
this.faviconEl_ = this.$<HTMLElement>('#favicon')!;

this.thumbnailContainer_ = this.$('#thumbnail') as HTMLElement;
this.thumbnailContainer_ = this.$<HTMLElement>('#thumbnail')!;

this.thumbnail_ = this.$('#thumbnailImg') as HTMLImageElement;
this.thumbnail_ = this.$<HTMLImageElement>('#thumbnailImg')!;

this.tabsApi_ = TabsApiProxyImpl.getInstance();

this.titleTextEl_ = this.$('#titleText') as HTMLElement;
this.titleTextEl_ = this.$<HTMLElement>('#titleText')!;

/**
* Flag indicating if this TabElement can accept dragover events. This
Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/resources/tab_strip/tab_group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class TabGroupElement extends CustomElement {

this.tabsApi_ = TabsApiProxyImpl.getInstance();

this.chip_ = this.$('#chip') as HTMLElement;
this.chip_ = this.$<HTMLElement>('#chip')!;
this.chip_.addEventListener('click', () => this.onClickChip_());
this.chip_.addEventListener(
'keydown', e => this.onKeydownChip_(/** @type {!KeyboardEvent} */ (e)));
Expand All @@ -43,13 +43,13 @@ export class TabGroupElement extends CustomElement {
}

getDragImage(): HTMLElement {
return this.$('#dragImage') as HTMLElement;
return this.$<HTMLElement>('#dragImage')!;
}

getDragImageCenter(): HTMLElement {
// Since the drag handle is #dragHandle, the drag image should be
// centered relatively to it.
return this.$('#dragHandle') as HTMLElement;
return this.$<HTMLElement>('#dragHandle')!;
}

private onClickChip_() {
Expand Down Expand Up @@ -96,7 +96,7 @@ export class TabGroupElement extends CustomElement {
}

updateVisuals(visualData: TabGroupVisualData) {
(this.$('#title') as HTMLElement).innerText = visualData.title;
this.$<HTMLElement>('#title')!.innerText = visualData.title;
this.style.setProperty('--tabstrip-tab-group-color-rgb', visualData.color);
this.style.setProperty(
'--tabstrip-tab-group-text-color-rgb', visualData.textColor);
Expand Down
23 changes: 11 additions & 12 deletions chrome/browser/resources/tab_strip/tab_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,13 +388,12 @@ export class TabListElement extends CustomElement implements
}

private findTabElement_(tabId: number): TabElement|null {
return this.$(`tabstrip-tab[data-tab-id="${tabId}"]`) as TabElement | null;
return this.$<TabElement>(`tabstrip-tab[data-tab-id="${tabId}"]`);
}

private findTabGroupElement_(groupId: string): TabGroupElement|null {
return this.$(`tabstrip-tab-group[data-group-id="${groupId}"]`) as
TabGroupElement |
null;
return this.$<TabGroupElement>(
`tabstrip-tab-group[data-group-id="${groupId}"]`);
}

private fetchAndUpdateColors_() {
Expand All @@ -403,8 +402,7 @@ export class TabListElement extends CustomElement implements
}

private fetchAndUpdateGroupData_() {
const tabGroupElements =
this.$all('tabstrip-tab-group') as NodeListOf<TabGroupElement>;
const tabGroupElements = this.$all<TabGroupElement>('tabstrip-tab-group');
this.tabsApi_.getGroupVisualData().then(({data}) => {
tabGroupElements.forEach(tabGroupElement => {
tabGroupElement.updateVisuals(
Expand All @@ -420,7 +418,7 @@ export class TabListElement extends CustomElement implements
}

private getActiveTab_(): TabElement|null {
return this.$('tabstrip-tab[active]') as TabElement | null;
return this.$<TabElement>('tabstrip-tab[active]');
}

getIndexOfTab(tabElement: TabElement): number {
Expand Down Expand Up @@ -473,7 +471,7 @@ export class TabListElement extends CustomElement implements
// document. When the tab strip first gains keyboard focus, no such event
// exists yet, so the outline needs to be explicitly set to visible.
this.focusOutlineManager_.visible = true;
(this.$('tabstrip-tab') as HTMLElement).focus();
this.$<TabElement>('tabstrip-tab')!.focus();
}

private onTabActivated_(tabId: number) {
Expand All @@ -488,7 +486,7 @@ export class TabListElement extends CustomElement implements
// have updated a Tab to have an active state. For example, if a
// tab is created with an already active state, there may be 2 active
// TabElements: the newly created tab and the previously active tab.
(this.$all('tabstrip-tab[active]') as NodeListOf<TabElement>)
this.$all<TabElement>('tabstrip-tab[active]')
.forEach((previouslyActiveTab) => {
if (previouslyActiveTab.tab.id !== tabId) {
previouslyActiveTab.tab = /** @type {!Tab} */ (
Expand Down Expand Up @@ -705,7 +703,7 @@ export class TabListElement extends CustomElement implements
index++;
}

let elementAtIndex = this.$all('tabstrip-tab')[index];
let elementAtIndex = this.$all('tabstrip-tab')[index]!;
if (elementAtIndex && elementAtIndex.parentElement &&
isTabGroupElement(elementAtIndex.parentElement)) {
elementAtIndex = elementAtIndex.parentElement;
Expand Down Expand Up @@ -796,7 +794,8 @@ export class TabListElement extends CustomElement implements
element, this.pinnedTabsElement_.childNodes[index]!);
} else {
let elementToInsert: TabElement|TabGroupElement = element;
let elementAtIndex = this.$all('tabstrip-tab').item(index);
let elementAtIndex: TabElement|TabGroupElement =
this.$all<TabElement>('tabstrip-tab').item(index);
let parentElement = this.unpinnedTabsElement_;

if (groupId) {
Expand Down Expand Up @@ -824,7 +823,7 @@ export class TabListElement extends CustomElement implements
// TabElement is being sandwiched between two TabElements in a group, it
// can be assumed that the tab will eventually be inserted into the
// group as well.
elementAtIndex = elementAtIndex.parentElement;
elementAtIndex = elementAtIndex.parentElement as TabGroupElement;
}

if (elementAtIndex && elementAtIndex.parentElement === parentElement) {
Expand Down
3 changes: 1 addition & 2 deletions ui/webui/resources/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ ts_library("library") {
"cr_elements/mwb_shared_style.ts",
"cr_elements/mwb_shared_vars.ts",
"cr_elements/search_highlight_style_css.ts",
"js/custom_element.ts",
"js/i18n_mixin.ts",
"js/list_property_update_mixin.ts",
"js/web_ui_listener_mixin.ts",
Expand Down Expand Up @@ -260,7 +261,6 @@ ts_library("library") {
"$root_dir/js/cr/ui/focus_outline_manager.m.d.ts",
"$root_dir/js/cr/ui/focus_row.m.d.ts",
"$root_dir/js/cr/ui/keyboard_shortcut_list.m.d.ts",
"$root_dir/js/custom_element.d.ts",
"$root_dir/js/event_tracker.m.d.ts",
"$root_dir/js/load_time_data.m.d.ts",
"$root_dir/js/plural_string_proxy.d.ts",
Expand Down Expand Up @@ -323,7 +323,6 @@ ts_definitions("generate_definitions") {
"js/cr/ui/focus_row.m.js",
"js/cr/ui/keyboard_shortcut_list.m.js",
"js/cr/ui/store.js",
"js/custom_element.js",
"js/event_tracker.m.js",
"js/icon.js",
"js/load_time_data.m.js",
Expand Down
6 changes: 1 addition & 5 deletions ui/webui/resources/js/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ preprocess_if_expr("preprocess_src_ts") {
in_folder = "./"
out_folder = "$preprocess_folder"
in_files = [
"custom_element.ts",
"i18n_mixin.ts",
"list_property_update_mixin.ts",
"web_ui_listener_mixin.ts",
Expand All @@ -62,7 +63,6 @@ preprocess_if_expr("preprocess_src") {
"assert.js",
"color_utils.js",
"cr.m.js",
"custom_element.js",
"i18n_template_no_process.js",
"icon.js",
"load_time_data.m.js",
Expand Down Expand Up @@ -292,7 +292,6 @@ js_type_check("js_resources_modules") {
deps = [
":assert.m",
":cr.m",
":custom_element",
":event_tracker.m",
":i18n_behavior.m",
":icon",
Expand Down Expand Up @@ -328,9 +327,6 @@ js_library("cr.m") {
externs_list = [ "$externs_path/chrome_send.js" ]
}

js_library("custom_element") {
}

js_library("event_tracker.m") {
sources = [ "$root_gen_dir/ui/webui/resources/js/event_tracker.m.js" ]
extra_deps = [ ":modulize_local" ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,27 @@
* See the following file for usage:
* chrome/test/data/webui/js/custom_element_test.js
*/

export class CustomElement extends HTMLElement {
static get template(): string {
return '';
}

constructor() {
super();

this.attachShadow({mode: 'open'});
const template = document.createElement('template');
template.innerHTML = this.constructor.template || '';
this.shadowRoot.appendChild(template.content.cloneNode(true));
template.innerHTML =
(this.constructor as typeof CustomElement).template || '';
this.shadowRoot!.appendChild(template.content.cloneNode(true));
}

/**
* @param {string} query
* @return {?Element}
*/
$(query) {
return this.shadowRoot.querySelector(query);
$<E extends Element = Element>(query: string): E|null {
return this.shadowRoot!.querySelector<E>(query);
}

/**
* @param {string} query
* @return {!NodeList<!Element>}
*/
$all(query) {
return this.shadowRoot.querySelectorAll(query);
$all<E extends Element = Element>(query: string): NodeListOf<E> {
return this.shadowRoot!.querySelectorAll<E>(query);
}
}

0 comments on commit 32de615

Please sign in to comment.