review-ui-widgets

# Chrome DevTools frontend reviewer skill: ui-and-widgets This skill provides review guidance for Chrome DevTools frontend changes matching the `ui-and-widgets` path group (^front_end/(ui|panels)/.*\.(ts|css)$). It focuses specifically on code review verification for UI and widget changes, complementing authoring guidance in `ui-widgets` and [`docs/ui_engineering.md`](../../../docs/ui_engineering.md), and integrating with the general review checklist in `code-review`. Every rule below recurred in at least 5 CLs from 2 distinct reviewers in historical reviews; cite the rule id in findings. ## Path group `ui-and-widgets` (17 rules) ### `UI_MODERN_COMPONENT_AND_ENCAPSULATION_STANDARDS`: Use modern DevTools UI primitives (`devtools-button`, `devtools-data-grid`, `devtools-icon`), #private fields, and module entrypoint imports - **Severity**: WARNING - **Why**: Legacy `UI.Fragment`, `DataGridImpl`, `LitElement`, `new Icon()`, and TypeScript `private` fields are superseded by standard `<devtools-*>` custom elements (imported via module entrypoints such as `import * as Buttons from '../../ui/components/buttons/buttons.js'` so custom tags register at runtime) and ECMAScript `#private` fields. - **Look for**: `UI.Fragment.build`, legacy `DataGridImpl`, `extends LitElement`, `private ` keyword instead of `#` in new UI classes, `<devtools-*>` tags in Lit templates without importing the corresponding component module entrypoint (e.g. `import * as Buttons from '../../ui/components/buttons/buttons.js'`), or formatting dates or timestamps with `toLocaleString()` inside sortable `<devtools-data-grid>` cells without a numeric/ISO sort value (sorts alphabetically instead of chronologically; use `toISOString()` or a numeric sort value). - **Do not flag**: Overriding inherited `protected` lifecycle methods on framework base classes (e.g. extending `UI.Widget.Widget`). ### `UI_TYPE_SAFETY_AND_EXHAUSTIVENESS`: Model multi-state UI workflows with discriminated unions, Platform.assertNever exhaustiveness checks, and strict domain types - **Severity**: WARNING - **Why**: Representing mutually exclusive UI states with loose optional boolean flags or `any` types permits impossible states and silently misses new enum variants. Discriminated unions paired with `Platform.assertNever` in `default:` branches enforce compile-time exhaustiveness. - **Look for**: `switch` statements over union/enum types missing `Platform.assertNever(x)` in `default:`, `any` or `declare` field modifiers in UI classes, or multiple boolean flags (`isLoading`, `isError`, `isEmpty`) representing mutually exclusive states. - **Do not flag**: Switch statements that intentionally handle only a small subset of a large external protocol enum. ### `UI_PURE_VIEW_AND_PRESENTER_SEPARATION`: Keep UI.Widget view functions pure and stateless by computing presentation data in presenters, passing decoupled ViewInput data structures, and preserving helper topology - **Severity**: WARNING - **Why**: In DevTools' MVP architecture, `DEFAULT_VIEW` functions must be pure projections of `ViewInput` to Lit templates. Querying SDK models, reading global settings, mutating presenter state, or calling host/clipboard APIs inside the view couples rendering to global state and breaks isolated view testing. Additionally, deleting existing standalone/module-level DOM helper functions or async class update helpers and inlining them into `performUpdate()` or `DEFAULT_VIEW` during Lit migrations unnecessarily flattens the abstraction topology and inflates diffs. - **Look for**: `DEFAULT_VIEW` or view helper functions calling `Common.Settings.Settings.instance()`, `UI.Context.Context.instance()`, `SDK.TargetManager.TargetManager.instance()`, `Host.*`, or mutating model or `input.*` state in place; `ViewInput` interfaces carrying pre-created DOM `Element`/`HTMLElement` nodes or `UI.Widget` instances (`ListWidget`, `TabbedPane`, `Linkifier` elements); presenters calling `DEFAULT_VIEW(...)` or auxiliary `*_VIEW(...)` constants directly instead of injecting `view: View = DEFAULT_VIEW` via the constructor (`this.#view(...)`) and exporting `ViewInput` and `View`; or Lit migrations that delete existing standalone/class DOM helper functions and inline their logic into `performUpdate()` or `DEFAULT_VIEW` instead of converting them in place to return `LitTemplate` (or `Promise<LitTemplate|null>`). - **Do not flag**: Legacy `UI.Widget` subclasses that have not yet migrated to a `DEFAULT_VIEW` function, or passing read-only `Protocol.*`, `SDK.*`, or `Models.*` objects in `ViewInput` when the view only reads properties/getters from them or forwards them to child widgets/linkifiers without mutating model state or calling side-effecting `SDK` methods. ### `UI_LIT_DECLARATIVE_TEMPLATES_AND_DIRECTIVES`: Use Lit.nothing, repeat with keys, classMap/styleMap, and ifDefined in declarative templates while preserving modular helper functions - **Severity**: WARNING - **Why**: Mutating DOM elements via `appendChild`, `classList.toggle`, or `innerHTML` alongside Lit templates, or returning `''`/`null` instead of `Lit.nothing` in conditional branches, corrupts Lit's part tracking and creates unnecessary text nodes or wrapper `<div>`s. Conversely, collapsing existing standalone DOM helper functions (e.g., `linkifyIcon`, `maybeCreateLinkToElementsPanel`) or class rendering helpers into a single monolithic `DEFAULT_VIEW` template harms readability and reviewability compared to converting those helpers in place to return `LitTemplate`. - **Look for**: Ternary branches returning `''` or `undefined` in `html` templates instead of `Lit.nothing`, unkeyed `.map()` on dynamic stateful lists instead of `repeat()`, `ifDefined` omissions on optional attributes, multiple `Lit.render()` calls mutating the same container, `Lit.*` namespace calls instead of direct `{Directives, html, nothing, render}` imports from `ui/lit/lit.js`, Lit migrations that flatten/inline existing DOM helper functions and class helper methods instead of preserving them in their original locations returning `LitTemplate`, or `super(..., {classes: [...]})` / `super(..., {jslog: ...})` in Light DOM (`!useShadowDom`) `UI.Widget` constructors instead of passing `{container: {classes: [...], attributes: {jslog: ...}}}` as the third argument to every `render(template, target, {container: {classes: [...]}})` branch in the view function. - **Do not flag**: Static arrays of primitive labels where item identity never reorders. Omitting Lit's `live()` directive on `<devtools-checkbox>` (`UI.UIUtils.CheckboxLabel`) `.checked` bindings (`live()` breaks `CheckboxLabel`'s host `checked` attribute synchronization when toggled externally), listening to `ObjectWrapper` events via Lit `@event=${...}` bindings on `UI.Widget` elements (`ObjectWrapper` instances set up via `eventMixin` on `Widget` dispatch on the DOM), interpolating boolean values directly inside Lit text/`<td>` positions (Lit renders booleans as `'true'`/`'false'`), or `super(element, {useShadowDom: true, classes: [...]})` where `classes` must remain on the outer shadow host element (`this.element`) rather than the inner shadow root `this.contentElement`. Newlines and indentation spaces between expressions and child elements in multi-line `html` templates (DevTools' custom `html` tag in `front_end/ui/lit/strip-whitespace.ts` strips `\n\s*` outside of `<...>` tags at runtime, so multi-line formatting does not introduce whitespace text nodes). ### `UI_WIDGET_DOM_AND_LISTENER_LIFECYCLE`: Attach listeners to external models and managers in wasShown and detach them symmetrically in willHide - **Severity**: ERROR - **Why**: Subscribing to long-lived SDK `TargetManager`, `Settings`, or model events in a widget's constructor without unsubscribing in `willHide()` (or `onDetach()` / `onunbind()`) causes hidden panels to process background updates and leaks detached widget instances. - **Look for**: `addEventListener`, `addModelListener`, or `addChangeListener` on singletons, models, or settings inside `UI.Widget` constructors or `wasShown()` without a matching `remove*` call in `willHide()` using the same function reference, scheduled or asynchronous `performUpdate()` methods on `SDKModelObserver` widgets that access or re-register a model without checking that `this.#model` is still active after `modelRemoved()` has run, or calling `removeChildren()` on a DOM or `TreeOutline` container that hosts imperatively mounted `UI.Widget` children without first detaching them (`this.detachChildWidgets()` or `UI.Widget.Widget.get(el)?.detach()`). - **Do not flag**: Listeners attached to the widget's own internal DOM elements (`this.contentElement`), which are garbage-collected together with the widget. Model or target listeners on background recording/overview widgets (such as `NetworkOverview`) that must continuously record events while the widget is hidden or collapsed (especially when rendering is already gated on `this.isShowing()`), or pre-existing constructor listeners untouched by a DOM-to-Lit view migration. ### `UI_ACCESSIBILITY_AND_SEMANTIC_HTML`: Use native semantic HTML controls with accurate ARIA attributes, keyboard handlers, and !event.isComposing guards - **Severity**: WARNING - **Why**: Clickable `<div>`/`<span>` elements and nested interactive controls (`<button>` inside `<a>` or `<button>`) break keyboard navigation and screen readers. Interactive elements require semantic tags (`<button>`, `<details>`/`<summary>`, `<thead>`/`<tbody>`), synchronized ARIA state (`aria-expanded`, `aria-details` for rich tooltips vs `aria-describedby` for plain text), and `!event.isComposing` checks on `Enter` keydown handlers. - **Look for**: `@click` on `<div>`/`<span>` without button semantics, `<button>` nested inside `<button>`/`<x-link>`, `Enter` keydown handlers missing `!event.isComposing`, or `aria-describedby` pointing to interactive `<devtools-tooltip>` elements. - **Do not flag**: Delegating click listeners on container elements that handle bubbled events from child buttons. Rendering `<tr>` rows directly into `this.affectedResources` (`<table class="affected-resource-list">`) in `AffectedResourcesView` subclasses without a `<tbody>` wrapper (`AffectedResourcesView` creates the `<table>` container without a `<tbody>` and subclasses render `<tr>` rows directly into `this.affectedResources`). ### `UI_PERFORMANCE_AND_FLAMECHART_RENDERING`: Precompute expensive metrics outside render loops, use Set/Map for O(1) lookups, and debounce high-frequency UI updates - **Severity**: WARNING - **Why**: Performing linear `Array.includes`/`find` scans, sorting, or regex parsing inside FlameChart canvas draw loops, `ElementsTreeOutline` mutation callbacks, or Lit render passes causes frame drops and UI jank on large traces and DOM trees. - **Look for**: `.includes()` / `.find()` / `.sort()` inside canvas draw or event iteration loops, repeated `ResizeObserver` registrations on individual child items instead of the parent container, or un-debounced filter/mutation handlers. - **Do not flag**: One-time initialization passes over bounded small arrays (less than 20 items). ### `UI_LOCALIZATION_AND_UISTRINGS_HYGIENE`: Define user-facing strings in sentence-case UIStrings using complete sentences and i18nTemplate for embedded elements, and prune unused entries - **Severity**: WARNING - **Why**: Concatenating translated fragments breaks word order in non-English locales, title-Cased strings violate Chromium UX standards, translating English-only experimental strings wastes localization quota, and unused `UIStrings` entries bloat locale bundles. - **Look for**: String concatenation around `i18nString(...)`, HTML/DOM elements interpolated without `i18nTemplate` (prefer `i18nTemplate(str_, UIStrings.foo, {PH1: html`...`})` from `ui/lit/lit.js` and declarative `<devtools-link>` inside Lit view functions over `uiI18n.getFormatLocalizedStringTemplate` and imperative `Link.create`), Title Case labels in `UIStrings`, unlocalized English literals in templates, or unused keys in `UIStrings`. - **Do not flag**: Technical protocol identifiers, MIME types, or strings in `UIStringsNotTranslate` / `i18n.i18n.lockedString(...)`. Pre-existing `UIStrings` entries (such as legacy Title Case strings) or pre-existing unlocalized string literals that are moved verbatim from imperative DOM creation into a Lit template during a refactoring CL. ### `UI_REACTIVE_WIDGET_UPDATE_LIFECYCLE`: Schedule widget re-renders asynchronously via this.requestUpdate() and guard property setters with equality checks - **Severity**: WARNING - **Why**: Calling `this.performUpdate()` or invoking `this.#view(...)` synchronously inside property setters causes redundant synchronous DOM renders when multiple properties change in the same tick. Setters should short-circuit if the value is unchanged and call `this.requestUpdate()`. - **Look for**: Widget property setters calling `this.performUpdate()` or `this.#view(...)` directly, setters calling `this.requestUpdate()` without checking `if (this.#prop === value) return;`, monolithic `set data(...)` object setters on `UI.Widget` subclasses instead of individual property setters, or synchronous `this.performUpdate()` calls inside a `UI.Widget` `constructor()` (creates DOM and reads `UI.Context` before the widget is attached or visible; use `this.requestUpdate()` or `wasShown()` instead). - **Do not flag**: Internal test helpers or synchronous initialization paths explicitly documented as requiring immediate flush. Synchronous `this.performUpdate()` calls in a constructor when the constructor or methods callable before `wasShown()` depend on `ViewOutput`, `widgetRef`, or `ref` fields populated by the initial render. ### `UI_VISUAL_LOGGING_INSTRUMENTATION`: Instrument interactive UI elements and containers with kebab-case VisualLogging contexts registered in KnownContextValues - **Severity**: WARNING - **Why**: DevTools relies on Visual Elements (`jslog`) telemetry to measure feature usage. Omitting `jslog` on new buttons/toggles/menus, using localized or camelCase context strings, adding redundant panel prefixes inside already-logged containers, or dropping `jslog` during Lit refactors breaks analytics. - **Look for**: Interactive elements (`<button>`, `<devtools-button>`, `<input>`, menus, drawers) missing `jslog` or `jslogContext`, non-kebab-case context names, or new context values missing from `KnownContextValues.ts`. - **Do not flag**: Purely decorative icons or internal wrapper `<div>`s that represent neither a distinct visual section nor an interactive control. ### `UI_CSS_SCOPING_AND_FLEX_LAYOUT`: Scope light-DOM stylesheets and align elements using flexbox or grid rather than competing margin and position offsets - **Severity**: INFO - **Why**: Unscoped class or tag selectors in light-DOM widget stylesheets leak across the DevTools document, while combining `vertical-align`, negative margins, and `position: relative` offsets creates brittle layouts that break under font scaling or zoom. - **Look for**: Unscoped generic selectors in light-DOM `.css` files (or `:host` inside `@scope` blocks instead of `:scope`), redundant `@scope to (devtools-widget > *)` wrappers on `useShadowDom: true` widgets that have no child `<devtools-widget>` elements, or CSS rules combining `vertical-align`, negative `margin-top`, and pixel offsets to center icons and buttons. - **Do not flag**: Shadow DOM component stylesheets where `:host` scoping is native. `<style>${styles}</style>` interpolations of `*.css.js` default imports in Lit templates (generated `.css.js` files export plain CSS strings, not `CSSStyleSheet` objects, and `<style>${...}</style>` is the standard replacement for deprecated `registerRequiredCSS`). ### `UI_VIEW_OUTPUT_AND_CALLBACK_CONTRACTS`: Encapsulate imperative DOM and child widget actions in ViewOutput callbacks and pass user actions via semantic ViewInput callbacks instead of raw DOM or Widget references - **Severity**: WARNING - **Why**: Exposing raw `HTMLElement` or `UI.Widget` references on `ViewOutput` (e.g. `output.childWidget = w`) or passing raw `MouseEvent`/`KeyboardEvent` objects to presenter callbacks leaks view implementation details into presenters. Supply all configuration through setters/inputs on the child widget, or if imperative calls are strictly necessary, populate narrow functional closures on `ViewOutput` (e.g. `output.focusInput = () => ...`, `output.scrollWidgetToSomething = () => widgetRef.scrollToSomething()`), and unpack event values before calling `input.onSelect(value)`.