RPP: fix CLS Culprits Insight not rendering

This CL fixes a bug in the CLS Culprits insight where it tried to read
`this.bounds` which was undefined. I think TS expects the `get` to exist
when the `set` does (we hit this a lot in our components). To fix this I
added `get bounds`.

I then added a unit test to verify the fix and to ensure it renders. I
then went down a bit of a rabbit-hole of styling; the <li> elements for
the culprits were not wrapped in a <ul> so I added that and tidied up
some of the margins.

Bug: none
Change-Id: I38dad120760f2f8bc13c63c31f07dcafe11703eb
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6074609
Commit-Queue: Connor Clark <[email protected]>
Reviewed-by: Connor Clark <[email protected]>
Auto-Submit: Jack Franklin <[email protected]>

Author: jackfranklin

front_end/panels/timeline/components/insights/BUILD.gn

@@ -69,7 +69,10 @@ devtools_entrypoint("bundle") {
 ts_library("unittests") {
   testonly = true
 
-  sources = [ "BaseInsightComponent.test.ts" ]
+  sources = [
+    "BaseInsightComponent.test.ts",
+    "CLSCulprits.test.ts",
+  ]
 
   deps = [
     ":bundle",

front_end/panels/timeline/components/insights/BaseInsightComponent.ts

@@ -112,6 +112,10 @@ export abstract class BaseInsightComponent<T extends InsightModel<{}>> extends H
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#boundRender);
   }
 
+  get bounds(): Trace.Types.Timing.TraceWindowMicroSeconds|null {
+    return this.data.bounds;
+  }
+
   set bounds(bounds: Trace.Types.Timing.TraceWindowMicroSeconds|null) {
     this.data.bounds = bounds;
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#boundRender);

front_end/panels/timeline/components/insights/CLSCulprits.test.ts

@@ -0,0 +1,45 @@
+// Copyright 2024 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.
+
+import {
+  getCleanTextContentFromElements,
+  getCleanTextContentFromSingleElement,
+  renderElementIntoDOM,
+} from '../../../../testing/DOMHelpers.js';
+import {describeWithEnvironment} from '../../../../testing/EnvironmentHelpers.js';
+import {TraceLoader} from '../../../../testing/TraceLoader.js';
+import * as Coordinator from '../../../../ui/components/render_coordinator/render_coordinator.js';
+
+import * as Insights from './insights.js';
+
+const coordinator = Coordinator.RenderCoordinator.RenderCoordinator.instance();
+describeWithEnvironment('CLSCulprits component', () => {
+  it('renders unsized image culprits', async function() {
+    const traceData = await TraceLoader.traceEngine(this, 'unsized-images.json.gz');
+    const firstNavInsights = traceData.insights?.values().next()?.value;
+    assert.isOk(firstNavInsights);
+    const clsModel = firstNavInsights.model.CLSCulprits;
+    const component = new Insights.CLSCulprits.CLSCulprits();
+    component.model = clsModel;
+    component.insightSetKey = firstNavInsights.id;
+    component.bounds = traceData.parsedTrace.Meta.traceBounds;
+    component.selected = true;
+
+    renderElementIntoDOM(component);
+    await coordinator.done();
+    assert.isOk(component.shadowRoot);
+
+    const titleText = getCleanTextContentFromSingleElement(component.shadowRoot, '.insight-title');
+    assert.strictEqual(titleText, 'Layout shift culprits');
+
+    const worstClusterText = getCleanTextContentFromSingleElement(component.shadowRoot, '.worst-cluster');
+    assert.strictEqual(worstClusterText, 'Worst cluster: Layout shift cluster @ 1.37 s');
+
+    const culpritsList = component.shadowRoot.querySelector<HTMLUListElement>('.worst-culprits');
+    assert.isOk(culpritsList);
+    const culpritsText = getCleanTextContentFromElements(culpritsList, 'li');
+    // There are two shifts hence the two culprits.
+    assert.deepEqual(culpritsText, ['Unsized Images', 'Unsized Images']);
+  });
+});

front_end/panels/timeline/components/insights/CLSCulprits.ts

@@ -150,12 +150,14 @@ export class CLSCulprits extends BaseInsightComponent<CLSCulpritsInsightModel> {
     return html`
       <div class="insight-section">
         <span class="worst-cluster">${i18nString(UIStrings.worstCluster)}: <button type="button" class="timeline-link" @click=${() => this.#clickEvent(worstCluster)}>${i18nString(UIStrings.layoutShiftCluster, {PH1: clusterTs})}</button></span>
-          <p>${i18nString(UIStrings.topCulprits)}:</p>
-              ${culprits.map(culprit => {
-                return html `
-                  <li>${culprit}</li>
-                `;
-              })}
+          <p class="list-title">${i18nString(UIStrings.topCulprits)}:</p>
+          <ul class="worst-culprits">
+            ${culprits.map(culprit => {
+              return html `
+                <li>${culprit}</li>
+              `;
+            })}
+          </ul>
       </div>`;
     // clang-format on
   }

front_end/panels/timeline/components/insights/baseInsightComponent.css

@@ -81,6 +81,17 @@
 
 .insight-body {
   padding: 0 var(--sys-size-6) var(--sys-size-5) var(--sys-size-6);
+
+  .list-title {
+    margin-top: var(--sys-size-4);
+    margin-bottom: var(--sys-size-3);
+  }
+
+  ul {
+    /* left padding to bring the list bullets to the right place */
+    padding: 0 0 0 var(--sys-size-9);
+    margin: 0;
+  }
 }
 
 .insight-section {

front_end/testing/DOMHelpers.ts

@@ -267,6 +267,16 @@ export function getCleanTextContentFromElements(el: ShadowRoot|HTMLElement, sele
   });
 }
 
+/**
+ * Returns the text content for the first element matching the given `selector` within the provided `el`.
+ * Will error if no element is found matching the selector.
+ */
+export function getCleanTextContentFromSingleElement(el: ShadowRoot|HTMLElement, selector: string): string {
+  const element = el.querySelector(selector);
+  assert.isOk(element, `Could not find element with selector ${selector}`);
+  return element.textContent ? element.textContent.trim().replace(/[ \n]{2,}/g, ' ') : '';
+}
+
 export function assertNodeTextContent(component: NodeText.NodeText.NodeText, expectedContent: string) {
   assert.isNotNull(component.shadowRoot);
   const content = Array.from(component.shadowRoot.querySelectorAll('span')).map(span => span.textContent).join('');