Allow markers to provide a marker color. (#5607)

fqueze · web-flow · commit 5eed56e89077 · 2025-09-09T11:16:56.000+02:00
diff --git a/src/components/marker-chart/Canvas.tsx b/src/components/marker-chart/Canvas.tsx
@@ -33,8 +33,17 @@ import type {
   MarkerTiming,
   MarkerTimingAndBuckets,
   MarkerIndex,
+  MarkerSchemaByName,
+  GraphColor,
 } from 'firefox-profiler/types';
 import { getStartEndRangeForMarker } from 'firefox-profiler/utils';
+import {
+  getStrokeColor,
+  getFillColor,
+  getDotColor,
+  isValidGraphColor,
+} from 'firefox-profiler/profile-logic/graph-color';
+import { getSchemaFromMarker } from 'firefox-profiler/profile-logic/marker-schema';
 
 import type {
   ChartCanvasScale,
@@ -59,6 +68,7 @@ type OwnProps = {
   readonly rowHeight: CssPixels;
   readonly getMarker: (param: MarkerIndex) => Marker;
   readonly getMarkerLabel: (param: MarkerIndex) => string;
+  readonly markerSchemaByName: MarkerSchemaByName;
   readonly markerListLength: number;
   readonly threadsKey: ThreadsKey;
   readonly updatePreviewSelection: WrapFunctionInDispatch<UpdatePreviewSelection>;
@@ -82,10 +92,64 @@ const TEXT_OFFSET_START = 3;
 const MARKER_DOT_RADIUS = 0.25;
 const LABEL_PADDING = 5;
 const MARKER_BORDER_COLOR = '#2c77d1';
+const DEFAULT_FILL_COLOR = '#8ac4ff'; // Light blue for non-highlighted
 
 class MarkerChartCanvasImpl extends React.PureComponent<Props> {
   _textMeasurement: TextMeasurement | null = null;
 
+  /**
+   * Get the fill and stroke colors for a marker based on its schema and data.
+   * If the marker schema has a colorField, use that field's value.
+   * Fall back to default blue if no color is specified.
+   */
+  _getMarkerColors(
+    markerIndex: MarkerIndex,
+    isHighlighted: boolean
+  ): {
+    fillColor: string;
+    strokeColor: string;
+  } {
+    const { getMarker, markerSchemaByName } = this.props;
+    const marker = getMarker(markerIndex);
+
+    let color: GraphColor | null = null;
+
+    // Try to get color from the marker schema's colorField
+    const schema = getSchemaFromMarker(markerSchemaByName, marker.data);
+
+    if (
+      schema &&
+      schema.colorField &&
+      marker.data &&
+      typeof marker.data === 'object'
+    ) {
+      // Use type assertion to safely access dynamic property
+      const fieldValue = (marker.data as any)[schema.colorField];
+      // Validate that the field value is a valid GraphColor
+      if (typeof fieldValue === 'string' && isValidGraphColor(fieldValue)) {
+        color = fieldValue as GraphColor;
+      }
+    }
+
+    if (color) {
+      if (isHighlighted) {
+        return {
+          fillColor: getStrokeColor(color),
+          strokeColor: getDotColor(color),
+        };
+      }
+      return {
+        fillColor: getFillColor(color),
+        strokeColor: getStrokeColor(color),
+      };
+    }
+    // Fall back to default blue colors
+    return {
+      fillColor: isHighlighted ? BLUE_60 : DEFAULT_FILL_COLOR,
+      strokeColor: isHighlighted ? BLUE_80 : MARKER_BORDER_COLOR,
+    };
+  }
+
   drawCanvas = (
     ctx: CanvasRenderingContext2D,
     scale: ChartCanvasScale,
@@ -232,7 +296,7 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
     isHighlighted: boolean = false
   ) {
     if (isInstantMarker) {
-      this.drawOneInstantMarker(ctx, x, y, h, isHighlighted);
+      this.drawOneInstantMarker(ctx, x, y, h, markerIndex, isHighlighted);
     } else {
       this.drawOneIntervalMarker(ctx, x, y, w, h, markerIndex, isHighlighted);
     }
@@ -248,14 +312,18 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
     isHighlighted: boolean
   ) {
     const { marginLeft, getMarkerLabel } = this.props;
+    const { fillColor, strokeColor } = this._getMarkerColors(
+      markerIndex,
+      isHighlighted
+    );
 
     if (w <= 2) {
       // This is an interval marker small enough that if we drew it as a
       // rectangle, we wouldn't see any inside part. With a width of 2 pixels,
       // the rectangle-with-borders would only be borders. With less than 2
       // pixels, the borders would collapse.
       // So let's draw it directly as a rect.
-      ctx.fillStyle = isHighlighted ? BLUE_80 : MARKER_BORDER_COLOR;
+      ctx.fillStyle = strokeColor;
 
       // w is rounded in the caller, but let's make sure it's at least 1.
       w = Math.max(w, 1);
@@ -264,8 +332,8 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
       // This is a bigger interval marker.
       const textMeasurement = this._getTextMeasurement(ctx);
 
-      ctx.fillStyle = isHighlighted ? BLUE_60 : '#8ac4ff';
-      ctx.strokeStyle = isHighlighted ? BLUE_80 : MARKER_BORDER_COLOR;
+      ctx.fillStyle = fillColor;
+      ctx.strokeStyle = strokeColor;
 
       ctx.beginPath();
 
@@ -312,10 +380,15 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> {
     x: CssPixels,
     y: CssPixels,
     h: CssPixels,
+    markerIndex: MarkerIndex,
     isHighlighted: boolean
   ) {
-    ctx.fillStyle = isHighlighted ? BLUE_60 : '#8ac4ff';
-    ctx.strokeStyle = isHighlighted ? BLUE_80 : MARKER_BORDER_COLOR;
+    const { fillColor, strokeColor } = this._getMarkerColors(
+      markerIndex,
+      isHighlighted
+    );
+    ctx.fillStyle = fillColor;
+    ctx.strokeStyle = strokeColor;
 
     // We're drawing a diamond shape, whose height is h - 2, and width is h / 2.
     ctx.beginPath();
diff --git a/src/components/marker-chart/index.tsx b/src/components/marker-chart/index.tsx
@@ -14,6 +14,7 @@ import { MarkerSettings } from 'firefox-profiler/components/shared/MarkerSetting
 import {
   getCommittedRange,
   getPreviewSelection,
+  getMarkerSchemaByName,
 } from 'firefox-profiler/selectors/profile';
 import { selectedThreadSelectors } from 'firefox-profiler/selectors/per-thread';
 import { getSelectedThreadsKey } from 'firefox-profiler/selectors/url-state';
@@ -28,6 +29,7 @@ import { ContextMenuTrigger } from 'firefox-profiler/components/shared/ContextMe
 import type {
   Marker,
   MarkerIndex,
+  MarkerSchemaByName,
   MarkerTimingAndBuckets,
   UnitIntervalOfProfileRange,
   StartEndRange,
@@ -51,6 +53,7 @@ type DispatchProps = {
 type StateProps = {
   readonly getMarker: (param: MarkerIndex) => Marker;
   readonly getMarkerLabel: (param: MarkerIndex) => string;
+  readonly markerSchemaByName: MarkerSchemaByName;
   readonly markerTimingAndBuckets: MarkerTimingAndBuckets;
   readonly maxMarkerRows: number;
   readonly markerListLength: number;
@@ -106,6 +109,7 @@ class MarkerChartImpl extends React.PureComponent<Props> {
       markerTimingAndBuckets,
       getMarker,
       getMarkerLabel,
+      markerSchemaByName,
       previewSelection,
       updatePreviewSelection,
       changeMouseTimePosition,
@@ -152,6 +156,7 @@ class MarkerChartImpl extends React.PureComponent<Props> {
                 markerTimingAndBuckets,
                 getMarker,
                 getMarkerLabel,
+                markerSchemaByName,
                 markerListLength,
                 updatePreviewSelection,
                 changeMouseTimePosition,
@@ -190,6 +195,7 @@ export const MarkerChart = explicitConnect<{}, StateProps, DispatchProps>({
     return {
       getMarker: selectedThreadSelectors.getMarkerGetter(state),
       getMarkerLabel: selectedThreadSelectors.getMarkerChartLabelGetter(state),
+      markerSchemaByName: getMarkerSchemaByName(state),
       markerTimingAndBuckets,
       maxMarkerRows: markerTimingAndBuckets.length,
       markerListLength: selectedThreadSelectors.getMarkerListLength(state),
diff --git a/src/profile-logic/graph-color.ts b/src/profile-logic/graph-color.ts
@@ -84,3 +84,22 @@ export function getDotColor(color: GraphColor) {
       throw new Error('Unexpected track color: ' + color);
   }
 }
+
+/**
+ * Check if a string is a valid GraphColor value.
+ */
+export function isValidGraphColor(value: string): value is GraphColor {
+  const validColors: GraphColor[] = [
+    'blue',
+    'green',
+    'grey',
+    'ink',
+    'magenta',
+    'orange',
+    'purple',
+    'red',
+    'teal',
+    'yellow',
+  ];
+  return validColors.includes(value as GraphColor);
+}
diff --git a/src/test/components/MarkerChart.test.tsx b/src/test/components/MarkerChart.test.tsx
@@ -4,6 +4,7 @@
 
 import { Provider } from 'react-redux';
 import { BLUE_60 } from 'photon-colors';
+import { getFillColor } from 'firefox-profiler/profile-logic/graph-color';
 
 // This module is mocked.
 import copy from 'copy-to-clipboard';
@@ -53,6 +54,9 @@ import { autoMockElementSize } from '../fixtures/mocks/element-size';
 
 import type { CssPixels, Profile } from 'firefox-profiler/types';
 
+// Constants matching those in Canvas.tsx
+const DEFAULT_FILL_COLOR = '#8ac4ff'; // Light blue for non-highlighted
+
 const MARKERS: TestDefinedMarker[] = [
   ['Marker A', 0, 10],
   ['Marker A', 0, 10],
@@ -780,4 +784,125 @@ describe('MarkerChart', function () {
       expect(container.querySelector('.EmptyReasons')).toMatchSnapshot();
     });
   });
+
+  describe('Colored markers', () => {
+    function setupColoredMarkerTest(markers: TestDefinedMarker[]) {
+      const profile = getProfileWithMarkers(markers);
+
+      // Add marker schema with colorField to the profile
+      profile.meta.markerSchema = [
+        {
+          name: 'Test',
+          display: ['marker-chart', 'marker-table'],
+          fields: [
+            { key: 'status', label: 'Status', format: 'string' },
+            {
+              key: 'statusColor',
+              label: 'Color',
+              format: 'string',
+              hidden: true,
+            },
+          ],
+          colorField: 'statusColor',
+        },
+      ];
+
+      const { flushRafCalls } = setupWithProfile(profile);
+      flushRafCalls();
+      const drawLog = flushDrawLog();
+
+      // Find fillStyle operations in the draw log
+      const fillStyleOps = drawLog.filter(
+        (op: any) => Array.isArray(op) && op[0] === 'set fillStyle'
+      );
+
+      return fillStyleOps.map((op: any) => op[1]);
+    }
+
+    it('renders markers with colors specified in schema colorField', () => {
+      const markersWithColors: TestDefinedMarker[] = [
+        [
+          'Green Test',
+          0,
+          5,
+          {
+            type: 'Test',
+            status: 'success',
+            statusColor: 'green',
+          },
+        ],
+        [
+          'Red Test',
+          6,
+          10,
+          {
+            type: 'Test',
+            status: 'failure',
+            statusColor: 'red',
+          },
+        ],
+        [
+          'Yellow Test',
+          11,
+          15,
+          {
+            type: 'Test',
+            status: 'warning',
+            statusColor: 'yellow',
+          },
+        ],
+      ];
+
+      const fillColors = setupColoredMarkerTest(markersWithColors);
+
+      // The colors should include our marker colors with transparency
+      expect(fillColors).toEqual(
+        expect.arrayContaining([
+          getFillColor('green'),
+          getFillColor('red'),
+          getFillColor('yellow'),
+        ])
+      );
+    });
+
+    it('falls back to default blue for markers without color data', () => {
+      const markersWithoutColors: TestDefinedMarker[] = [
+        [
+          'Test without color',
+          0,
+          5,
+          {
+            type: 'Test',
+            status: 'unknown',
+            // No statusColor field
+          },
+        ],
+      ];
+
+      const fillColors = setupColoredMarkerTest(markersWithoutColors);
+
+      // Should use default blue color
+      expect(fillColors).toEqual(expect.arrayContaining([DEFAULT_FILL_COLOR]));
+    });
+
+    it('ignores invalid color values', () => {
+      const markersWithInvalidColors: TestDefinedMarker[] = [
+        [
+          'Invalid color test',
+          0,
+          5,
+          {
+            type: 'Test',
+            status: 'unknown',
+            statusColor: 'not-a-valid-color',
+          },
+        ],
+      ];
+
+      const fillColors = setupColoredMarkerTest(markersWithInvalidColors);
+
+      // Should fall back to default blue since the color is invalid
+      expect(fillColors).toEqual(expect.arrayContaining([DEFAULT_FILL_COLOR]));
+    });
+  });
 });
diff --git a/src/types/markers.ts b/src/types/markers.ts
@@ -171,6 +171,11 @@ export type MarkerSchema = {
   // if present, give the marker its own local track
   graphs?: Array<MarkerGraph>;
 
+  // If present, specifies the key of a marker field that contains the marker's color.
+  // The field should contain one of the GraphColor values.
+  // This allows individual markers to have different colors based on their data.
+  colorField?: string;
+
   // If set to true, markers of this type are assumed to be well-nested with all
   // other stack-based markers on the same thread. Stack-based markers may
   // be displayed in a different part of the marker chart than non-stack-based
