fix(Dashboard): Retain colors when color scheme not set

superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts

@@ -42,15 +42,29 @@ function openProperties() {
     cy.getBySel('header-actions-menu')
       .contains('Edit properties')
       .click({ force: true });
-    cy.wait(500);
+    cy.get('.ant-modal-body').should('be.visible');
   });
 }
 
+function assertMetadata(text: string) {
+  const regex = new RegExp(text);
+  cy.get('#json_metadata')
+    .should('be.visible')
+    .then(() => {
+      const metadata = cy.$$('#json_metadata')[0];
+
+      // cypress can read this locally, but not in ci
+      // so we have to use the ace module directly to fetch the value
+      expect(ace.edit(metadata).getValue()).to.match(regex);
+    });
+}
+
 function openAdvancedProperties() {
   cy.get('.ant-modal-body')
     .contains('Advanced')
     .should('be.visible')
     .click({ force: true });
+  cy.get('#json_metadata').should('be.visible');
 }
 
 function dragComponent(
@@ -120,10 +134,13 @@ function visitResetTabbedDashboard() {
 
 function selectColorScheme(color: string) {
   cy.get(
-    '[data-test="dashboard-edit-properties-form"] [aria-label="Select color scheme"]',
+    '[data-test="dashboard-edit-properties-form"] input[aria-label="Select color scheme"]',
   )
     .first()
-    .click();
+    .then($input => {
+      cy.wrap($input).click({ force: true });
+      cy.wrap($input).type(color.slice(0, 3), { force: true });
+    });
   cy.getBySel(color).click({ force: true });
 }
 
@@ -137,34 +154,34 @@ function saveChanges() {
   cy.wait('@update');
 }
 
-function assertMetadata(text: string) {
-  const regex = new RegExp(text);
-  cy.get('#json_metadata')
-    .should('be.visible')
-    .then(() => {
-      const metadata = cy.$$('#json_metadata')[0];
-
-      // cypress can read this locally, but not in ci
-      // so we have to use the ace module directly to fetch the value
-      expect(ace.edit(metadata).getValue()).to.match(regex);
-    });
-}
 function clearMetadata() {
   cy.get('#json_metadata').then($jsonmetadata => {
-    cy.wrap($jsonmetadata).find('.ace_content').click();
+    cy.wrap($jsonmetadata).find('.ace_content').click({ force: true });
     cy.wrap($jsonmetadata)
       .find('.ace_text-input')
-      .type('{selectall} {backspace}', { force: true });
+      .then($ace => {
+        cy.wrap($ace).focus();
+        cy.wrap($ace).should('have.focus');
+        cy.wrap($ace).type('{selectall}', { force: true });
+        cy.wrap($ace).type('{backspace}', { force: true });
+      });
   });
 }
 
 function writeMetadata(metadata: string) {
-  cy.get('#json_metadata').then($jsonmetadata =>
-    cy
-      .wrap($jsonmetadata)
+  cy.get('#json_metadata').then($jsonmetadata => {
+    cy.wrap($jsonmetadata).find('.ace_content').click({ force: true });
+    cy.wrap($jsonmetadata)
       .find('.ace_text-input')
-      .type(metadata, { parseSpecialCharSequences: false, force: true }),
-  );
+      .then($ace => {
+        cy.wrap($ace).focus();
+        cy.wrap($ace).should('have.focus');
+        cy.wrap($ace).type(metadata, {
+          parseSpecialCharSequences: false,
+          force: true,
+        });
+      });
+  });
 }
 
 function openExplore(chartName: string) {
@@ -469,7 +486,7 @@ describe('Dashboard edit', () => {
         .should('have.css', 'fill', 'rgb(255, 0, 0)');
     });
 
-    it.skip('should change color scheme multiple times', () => {
+    it('should change color scheme multiple times', () => {
       openProperties();
       selectColorScheme('blueToGreen');
       applyChanges();
@@ -487,7 +504,7 @@ describe('Dashboard edit', () => {
         '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
       )
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       // open 2nd main tab
       openTab(0, 1);
@@ -496,7 +513,7 @@ describe('Dashboard edit', () => {
       // label Anthony
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .eq(2)
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       editDashboard();
       openProperties();
@@ -507,7 +524,7 @@ describe('Dashboard edit', () => {
       // label Anthony
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .eq(2)
-        .should('have.css', 'fill', 'rgb(41, 105, 107)');
+        .should('have.css', 'fill', 'rgb(0, 128, 246)');
 
       // open main tab and nested tab
       openTab(0, 0);
@@ -518,10 +535,10 @@ describe('Dashboard edit', () => {
         '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
       )
         .first()
-        .should('have.css', 'fill', 'rgb(41, 105, 107)');
+        .should('have.css', 'fill', 'rgb(0, 128, 246)');
     });
 
-    it.skip('should apply the color scheme across main tabs', () => {
+    it('should apply the color scheme across main tabs', () => {
       openProperties();
       selectColorScheme('blueToGreen');
       applyChanges();
@@ -533,10 +550,10 @@ describe('Dashboard edit', () => {
 
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
     });
 
-    it.skip('should apply the color scheme across main tabs for rendered charts', () => {
+    it('should apply the color scheme across main tabs for rendered charts', () => {
       waitForChartLoad({ name: 'Treemap', viz: 'treemap_v2' });
       openProperties();
       selectColorScheme('blueToGreen');
@@ -549,7 +566,7 @@ describe('Dashboard edit', () => {
 
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .first()
-        .should('have.css', 'fill', 'rgb(41, 105, 107)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       // change scheme now that charts are rendered across the main tabs
       editDashboard();
@@ -560,10 +577,10 @@ describe('Dashboard edit', () => {
 
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(0, 128, 246)');
     });
 
-    it.skip('should apply the color scheme in nested tabs', () => {
+    it('should apply the color scheme in nested tabs', () => {
       openProperties();
       selectColorScheme('blueToGreen');
       applyChanges();
@@ -579,17 +596,17 @@ describe('Dashboard edit', () => {
         '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
       )
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       // open another nested tab
       openTab(2, 1);
       waitForChartLoad({ name: 'Growth Rate', viz: 'line' });
       cy.get('[data-test-chart-name="Growth Rate"] .line .nv-legend-symbol')
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
     });
 
-    it.skip('should apply a valid color scheme for rendered charts in nested tabs', () => {
+    it('should apply a valid color scheme for rendered charts in nested tabs', () => {
       // open the tab first time and let chart load
       openTab(1, 1);
       waitForChartLoad({
@@ -611,7 +628,7 @@ describe('Dashboard edit', () => {
         '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
       )
         .first()
-        .should('have.css', 'fill', 'rgb(234, 11, 140)');
+        .should('have.css', 'fill', 'rgb(50, 0, 167)');
     });
   });
 
@@ -625,7 +642,7 @@ describe('Dashboard edit', () => {
       openProperties();
     });
 
-    it.skip('should accept a valid color scheme', () => {
+    it('should accept a valid color scheme', () => {
       openAdvancedProperties();
       clearMetadata();
       writeMetadata('{"color_scheme":"lyftColors"}');
@@ -636,21 +653,21 @@ describe('Dashboard edit', () => {
       applyChanges();
     });
 
-    it.skip('should overwrite the color scheme when advanced is closed', () => {
+    it('should overwrite the color scheme when advanced is closed', () => {
       selectColorScheme('blueToGreen');
       openAdvancedProperties();
       assertMetadata('blueToGreen');
       applyChanges();
     });
 
-    it.skip('should overwrite the color scheme when advanced is open', () => {
+    it('should overwrite the color scheme when advanced is open', () => {
       openAdvancedProperties();
       selectColorScheme('modernSunset');
       assertMetadata('modernSunset');
       applyChanges();
     });
 
-    it.skip('should not accept an invalid color scheme', () => {
+    it('should not accept an invalid color scheme', () => {
       openAdvancedProperties();
       clearMetadata();
       // allow console error
@@ -716,7 +733,7 @@ describe('Dashboard edit', () => {
       visitEdit();
     });
 
-    it.skip('should add charts', () => {
+    it('should add charts', () => {
       cy.get('[role="checkbox"]').click();
       dragComponent();
       cy.getBySel('dashboard-component-chart-holder').should('have.length', 1);
@@ -765,7 +782,7 @@ describe('Dashboard edit', () => {
       visitEdit();
     });
 
-    it.skip('should save', () => {
+    it('should save', () => {
       cy.get('[role="checkbox"]').click();
       dragComponent();
       cy.getBySel('header-save-button').should('be.enabled');

superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts

@@ -17,7 +17,9 @@
  * under the License.
  */
 
+import { getCategoricalSchemeRegistry } from '.';
 import { makeSingleton } from '../utils';
+import CategoricalColorNamespace from './CategoricalColorNamespace';
 
 export enum LabelsColorMapSource {
   Dashboard,
@@ -38,7 +40,16 @@ export class LabelsColorMap {
     this.source = LabelsColorMapSource.Dashboard;
   }
 
-  updateColorMap(categoricalNamespace: any, colorScheme?: string) {
+  /**
+   * Wipes out the color map and updates it with the new color scheme.
+   *
+   * @param categoricalNamespace - the namespace to use for color mapping
+   * @param colorScheme - color scheme
+   */
+  updateColorMap(
+    categoricalNamespace: CategoricalColorNamespace,
+    colorScheme?: string,
+  ) {
     const newColorMap = new Map();
     this.colorMap.clear();
     this.chartsLabelsMap.forEach((chartConfig, sliceId) => {
@@ -58,6 +69,16 @@ export class LabelsColorMap {
     return this.colorMap;
   }
 
+  /**
+   *
+   * Called individually by each plugin via getColor fn.
+   *
+   * @param label - the label name
+   * @param color - the color
+   * @param sliceId - the chart id
+   * @param colorScheme - the color scheme
+   *
+   */
   addSlice(
     label: string,
     color: string,
@@ -75,12 +96,34 @@ export class LabelsColorMap {
       labels.push(label);
       this.chartsLabelsMap.set(sliceId, {
         labels,
-        scheme: colorScheme,
+        scheme: this.chartsLabelsMap.get(sliceId)?.scheme || colorScheme,
       });
     }
     this.colorMap.set(label, color);
   }
 
+  /**
+   * Keeps track of original colors of charts in their formData.
+   *
+   * @param sliceId - chart id
+   * @param colorScheme - color scheme
+   */
+  setSliceOriginColorScheme(sliceId: number, colorScheme?: string) {
+    const categoricalSchemes = getCategoricalSchemeRegistry();
+    const fallbackColorScheme =
+      categoricalSchemes.getDefaultKey()?.toString() ?? '';
+
+    this.chartsLabelsMap.set(sliceId, {
+      labels: this.chartsLabelsMap.get(sliceId)?.labels || [],
+      scheme: colorScheme || fallbackColorScheme,
+    });
+  }
+
+  /**
+   * Remove a slice from the color map.
+   *
+   * @param sliceId - the chart
+   */
   removeSlice(sliceId: number) {
     if (this.source !== LabelsColorMapSource.Dashboard) return;
 
@@ -96,6 +139,9 @@ export class LabelsColorMap {
     this.colorMap = newColorMap;
   }
 
+  /**
+   * Clear the shared labels color map.
+   */
   clear() {
     this.chartsLabelsMap.clear();
     this.colorMap.clear();