Skip to content

Commit

Permalink
RPP: fix text appearing on layout shift track
Browse files Browse the repository at this point in the history
This bug was introduced by crrev.com/c/6172797 which mistakently caused
the layout shift track to have the "Layout shift cluster" text drawn on
to it.

The reason we didn't notice this before is because the layout shifts
track was trying to add the "Layout shift" title to each diamond, but
because the diamonds are not wide enough, no text got drawn onto it. But
that is not true of the clusters.

To fix this I reintroduced the `titleForEvent` call in the appender, but
make it always return an empty string, as we never want to title any of
the events on the track itself.

Fixed: 390093886
Change-Id: Ic77ca03dd94fce5ab040aac0d2254f6b0bb6b738
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6169939
Reviewed-by: Andres Olivares <[email protected]>
Commit-Queue: Andres Olivares <[email protected]>
Auto-Submit: Jack Franklin <[email protected]>
  • Loading branch information
jackfranklin authored and Devtools-frontend LUCI CQ committed Jan 15, 2025
1 parent 326c069 commit 7298ef0
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 5 deletions.
13 changes: 8 additions & 5 deletions front_end/panels/timeline/CompatibilityTracksAppender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,11 +637,14 @@ export class CompatibilityTracksAppender {
throw new Error('Track not found for level');
}

// Historically all tracks would have a titleForEvent() method.
// However, we are working on migrating all event title logic into one place (components/EntryName)
// TODO(crbug.com/365047728):
// Once this migration is complete, no tracks will have a custom
// titleForEvent method and we can remove titleForEvent entirely.
// Historically all tracks would have a titleForEvent() method. However a
// lot of these were duplicated so we worked on removing them in favour of
// the EntryName.nameForEntry method called below (see crbug.com/365047728).
// However, sometimes an appender needs to customise the titles slightly;
// for example the LayoutShiftsTrackAppender does not show any titles as we
// use diamonds to represent layout shifts.
// So whilst we expect most appenders to not define this method, we do
// allow appenders to override it.
if (track.titleForEvent) {
return track.titleForEvent(event);
}
Expand Down
12 changes: 12 additions & 0 deletions front_end/panels/timeline/LayoutShiftsTrackAppender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,18 @@ export class LayoutShiftsTrackAppender implements TrackAppender {
return Utils.ImageCache.preload(screenshots);
}

titleForEvent(_event: Trace.Types.Events.Event): string {
/**
* This method defines the titles drawn on the track for the events in this
* appender. In the case of the Layout Shifts, we do not draw any titles. We
* draw layout shifts which are represented as diamonds, and clusters, which
* are represented as the purple lines through the diamonds. We do not want
* to put any text on top of these, hence overriding this method to return
* the empty string.
*/
return '';
}

static createShiftViz(
event: Trace.Types.Events.SyntheticLayoutShift, parsedTrace: Trace.Handlers.Types.ParsedTrace,
maxSize: UI.Geometry.Size): HTMLElement|undefined {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ describeWithEnvironment('LayoutShiftsTrackAppender', function() {
}
});

it('does not define any title for a layout shift or a cluster', async () => {
const {layoutShiftsTrackAppender, parsedTrace} = await renderTrackAppender(this, 'cls-no-nav.json.gz');
const cluster = parsedTrace.LayoutShifts.clusters.at(0);
assert.isOk(cluster);
const shift = cluster.events.at(0);
assert.isOk(shift);
assert.strictEqual(layoutShiftsTrackAppender.titleForEvent(cluster), '');
assert.strictEqual(layoutShiftsTrackAppender.titleForEvent(shift), '');
});

it('shows "Layout shift" tooltip on hover', async function() {
const {layoutShiftsTrackAppender, parsedTrace} = await renderTrackAppender(this, 'cls-no-nav.json.gz');
const shifts = parsedTrace.LayoutShifts.clusters.flatMap(c => c.events);
Expand Down

0 comments on commit 7298ef0

Please sign in to comment.