(core) Fixing click-away bug for the cell color widget

Summary:
After introducing multi columns operation, color picker
could save a cell style for a wrong column, if the save operation
was triggered by user clicking on one of the cells.

Test Plan: Updated

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3668

app/client/models/ViewFieldConfig.ts

@@ -1,39 +1,35 @@
 import * as modelUtil from 'app/client/models/modelUtil';
 // This is circular import, but only for types so it's fine.
 import type {DocModel, ViewFieldRec} from 'app/client/models/DocModel';
-import {Style} from 'app/client/models/Styles';
 import * as UserType from 'app/client/widgets/UserType';
 import {ifNotSet} from 'app/common/gutil';
 import * as ko from 'knockout';
 import intersection from "lodash/intersection";
 import isEqual from "lodash/isEqual";
+import zip from 'lodash/zip';
 
 export class ViewFieldConfig {
   /** If there are multiple columns selected in the viewSection */
   public multiselect: ko.Computed<boolean>;
   /** If all selected columns have the same widget list. */
   public sameWidgets: ko.Computed<boolean>;
-  /** Widget options for a field or multiple fields */
+  /** Widget options for a field or multiple fields. Doesn't contain style options */
   public options: CommonOptions;
+  /** Style options for a field or multiple fields  */
+  public style: ko.Computed<StyleOptions>;
 
   // Rest of the options mimic the same options from ViewFieldRec.
   public wrap: modelUtil.KoSaveableObservable<boolean|undefined>;
   public widget: ko.Computed<string|undefined>;
   public alignment: modelUtil.KoSaveableObservable<string|undefined>;
-  public textColor: modelUtil.KoSaveableObservable<string|undefined>;
+  public fields: ko.PureComputed<ViewFieldRec[]>;
-  public fillColor: modelUtil.KoSaveableObservable<string|undefined>;
-  public fontBold: modelUtil.KoSaveableObservable<boolean|undefined>;
-  public fontUnderline: modelUtil.KoSaveableObservable<boolean|undefined>;
-  public fontItalic: modelUtil.KoSaveableObservable<boolean|undefined>;
-  public fontStrikethrough: modelUtil.KoSaveableObservable<boolean|undefined>;
-  private _fields: ko.PureComputed<ViewFieldRec[]>;
   constructor(private _field: ViewFieldRec, private _docModel: DocModel) {
     // Everything here will belong to a _field, this class is just a builder.
     const owner = _field;
 
     // Get all selected fields from the viewSection, if there is only one field
     // selected (or the selection is empty) return it in an array.
-    this._fields = owner.autoDispose(ko.pureComputed(() => {
+    this.fields = owner.autoDispose(ko.pureComputed(() => {
       const list = this._field.viewSection().selectedFields();
       if (!list || !list.length) {
         return [_field];
@@ -46,13 +42,13 @@ export class ViewFieldConfig {
     }));
 
     // Just a helper field to see if we have multiple selected columns or not.
-    this.multiselect = owner.autoDispose(ko.pureComputed(() => this._fields().length > 1));
+    this.multiselect = owner.autoDispose(ko.pureComputed(() => this.fields().length > 1));
 
     // Calculate if all columns share the same allowed widget list (like for Numeric type
     // we have normal TextBox and Spinner). This will be used to allow the user to change
     // this type if such columns are selected.
     this.sameWidgets = owner.autoDispose(ko.pureComputed(() => {
-      const list = this._fields();
+      const list = this.fields();
       // If we have only one field selected, list is always the same.
       if (list.length <= 1) { return true; }
       // Now get all widget list and calculate intersection of the Sets.
@@ -73,7 +69,7 @@ export class ViewFieldConfig {
         }
         // If all have the same value, return it, otherwise
         // return a default value for this option "undefined"
-        const values = this._fields().map(f => f.widget());
+        const values = this.fields().map(f => f.widget());
         if (allSame(values)) {
           return values[0];
         } else {
@@ -82,7 +78,7 @@ export class ViewFieldConfig {
       },
       write: (widget) => {
         // Go through all the fields, and reset them all.
-        for(const field of this._fields.peek()) {
+        for(const field of this.fields.peek()) {
           // Reset the entire JSON, so that all options revert to their defaults.
           const previous = field.widgetOptionsJson.peek();
           // We don't need to bundle anything (actions send in the same tick, are bundled
@@ -102,7 +98,7 @@ export class ViewFieldConfig {
     // We will use this, to know which options are allowed to be changed
     // when multiple columns are selected.
     const commonOptions = owner.autoDispose(ko.pureComputed(() => {
-      const fields = this._fields();
+      const fields = this.fields();
       // Put all options of first widget in the Set, and then remove
       // them one by one, if they are not present in other fields.
       let options: Set<string>|null = null;
@@ -122,18 +118,7 @@ export class ViewFieldConfig {
           }
         }
       }
-      // Add cell style options, as they are common to all widgets.
+      return options ?? new Set();
-      const result = options ?? new Set();
-      result.add('textColor');
-      result.add('fillColor');
-      result.add('fontBold');
-      result.add('fontItalic');
-      result.add('fontUnderline');
-      result.add('fontStrikethrough');
-      result.add('rulesOptions');
-      // We are leaving rules out for this moment, as this is not supported
-      // at this moment.
-      return result;
     }));
 
     // Prepare our "multi" widgetOptionsJson, that can read and save
@@ -147,7 +132,7 @@ export class ViewFieldConfig {
         // Assemble final json object.
         const result: any = {};
         // First get all widgetOption jsons from all columns/fields.
-        const optionList = this._fields().map(f => f.widgetOptionsJson());
+        const optionList = this.fields().map(f => f.widgetOptionsJson());
         // And fill only those that are common
         const common = commonOptions();
         for(const key of common) {
@@ -173,9 +158,9 @@ export class ViewFieldConfig {
             delete value[key];
           }
         }
-        // Now update all options, for all fields, be amending the options
+        // Now update all options, for all fields, by amending the options
         // object from the field/column.
-        for(const item of this._fields.peek()) {
+        for(const item of this.fields.peek()) {
           const previous = item.widgetOptionsJson.peek();
           setter(item.widgetOptionsJson, {
             ...previous,
@@ -189,10 +174,10 @@ export class ViewFieldConfig {
     this.options = owner.autoDispose(extendObservable(modelUtil.objObservable(options), {
       // Property is not supported by set of columns if it is not a common option.
       disabled: prop => ko.pureComputed(() => !commonOptions().has(prop)),
-      // Property has mixed value, if no all options are the same.
+      // Property has mixed value, if not all options are the same.
-      mixed: prop => ko.pureComputed(() => !allSame(this._fields().map(f => f.widgetOptionsJson.prop(prop)()))),
+      mixed: prop => ko.pureComputed(() => !allSame(this.fields().map(f => f.widgetOptionsJson.prop(prop)()))),
       // Property has empty value, if all options are empty (are null, undefined, empty Array or empty Object).
-      empty: prop => ko.pureComputed(() => allEmpty(this._fields().map(f => f.widgetOptionsJson.prop(prop)()))),
+      empty: prop => ko.pureComputed(() => allEmpty(this.fields().map(f => f.widgetOptionsJson.prop(prop)()))),
     }));
 
     // This is repeated logic for wrap property in viewFieldRec,
@@ -202,15 +187,74 @@ export class ViewFieldConfig {
       () => this._field.viewSection().parentKey() !== 'record'
     );
 
-    // Some options extracted from our "virtual" widgetOptions json. Just to make
-    // them easier to use in the rest of the app.
     this.alignment = this.options.prop('alignment');
-    this.textColor = this.options.prop('textColor');
+
-    this.fillColor = this.options.prop('fillColor');
+    // Style options are a bit different, as they are saved when style picker is disposed.
-    this.fontBold = this.options.prop('fontBold');
+    // By the time it happens, fields may have changed (since user might have clicked some other column).
-    this.fontUnderline = this.options.prop('fontUnderline');
+    // To support this use case we need to compute a snapshot of fields, and use it to save style. Style
-    this.fontItalic = this.options.prop('fontItalic');
+    // picker will be rebuild every time fields change, and it will have access to last selected fields
-    this.fontStrikethrough = this.options.prop('fontStrikethrough');
+    // when it will be disposed.
+    this.style = ko.pureComputed(() => {
+      const fields = this.fields();
+      const multiSelect = fields.length > 1;
+      const savableOptions = modelUtil.savingComputed({
+        read: () => {
+          // For one column, just proxy this to the field.
+          if (!multiSelect) {
+            return this._field.widgetOptionsJson();
+          }
+          // Assemble final json object.
+          const result: any = {};
+          // First get all widgetOption jsons from all columns/fields.
+          const optionList = fields.map(f => f.widgetOptionsJson());
+          // And fill only those that are common
+          for(const key of ['textColor', 'fillColor', 'fontBold',
+                            'fontItalic', 'fontUnderline', 'fontStrikethrough']) {
+            // Setting null means that this options is there, but has no value.
+            result[key] = null;
+            // If all columns have the same value, use it.
+            if (allSame(optionList.map(v => v[key]))) {
+              result[key] = optionList[0][key] ?? null;
+            }
+          }
+          return result;
+        },
+        write: (setter, value) => {
+          if (!multiSelect) {
+            return setter(this._field.widgetOptionsJson, value);
+          }
+          // When the creator panel is saving widgetOptions, it will pass
+          // our virtual widgetObject, which has nulls for mixed values.
+          // If this option wasn't changed (set), we don't want to save it.
+          value = {...value};
+          for(const key of Object.keys(value)) {
+            if (value[key] === null) {
+              delete value[key];
+            }
+          }
+          // Now update all options, for all fields, by amending the options
+          // object from the field/column.
+          for(const item of fields) {
+            const previous = item.widgetOptionsJson.peek();
+            setter(item.widgetOptionsJson, {
+              ...previous,
+              ...value,
+            });
+          }
+        }
+      });
+      // Style picker needs to be able revert to previous value, if user cancels.
+      const state = fields.map(f => f.style.peek());
+      // We need some additional information about each property.
+      const result: StyleOptions = extendObservable(modelUtil.objObservable(savableOptions), {
+        // Property has mixed value, if not all options are the same.
+        mixed: prop => ko.pureComputed(() => !allSame(fields.map(f => f.widgetOptionsJson.prop(prop)()))),
+        // Property has empty value, if all options are empty (are null, undefined, empty Array or empty Object).
+        empty: prop => ko.pureComputed(() => allEmpty(fields.map(f => f.widgetOptionsJson.prop(prop)()))),
+      });
+      result.revert = () => { zip(fields, state).forEach(([f, s]) => f!.style(s!)); };
+      return result;
+    });
   }
 
   // Helper for Choice/ChoiceList columns, that saves widget options and renames values in a document
@@ -220,7 +264,7 @@ export class ViewFieldConfig {
     const tableId = this._field.column.peek().table.peek().tableId.peek();
     if (this.multiselect.peek()) {
       this._field.config.options.update(options);
-      const colIds = this._fields.peek().map(f => f.colId.peek());
+      const colIds = this.fields.peek().map(f => f.colId.peek());
       return this._docModel.docData.bundleActions("Update choices configuration", () => Promise.all([
         this._field.config.options.save(),
         !hasRenames ? null : this._docModel.docData.sendActions(
@@ -241,26 +285,6 @@ export class ViewFieldConfig {
     }
 
   }
-
-  // Two helper methods, to support reverting viewFields style on the style
-  // picker. Style picker is reverting options by remembering what was
-  // there previously, and setting it back when user presses the cancel button.
-  // This won't work for mixed values, as there is no previous single value.
-  // To support this reverting mechanism, we will remember all styles for
-  // selected fields, and revert them ourselves. Style picker will either use
-  // our methods or fallback with its default behavior.
-  public copyStyles() {
-    return this._fields.peek().map(f => f.style.peek());
-  }
-  public setStyles(styles: Style[]|null) {
-    if (!styles) {
-      return;
-    }
-    for(let i = 0; i < this._fields.peek().length; i++) {
-      const f = this._fields.peek()[i];
-      f.style(styles[i]);
-    }
-  }
 }
 
 /**
@@ -284,18 +308,31 @@ function allEmpty(arr: any[]) {
   return arr.every(item => ifNotSet(item, null) === null);
 }
 
+/**
+ * Extended version of widget options observable that contains information about mixed and empty values.
+ */
 type CommonOptions = modelUtil.SaveableObjObservable<any> & {
   disabled(prop: string): ko.Computed<boolean>,
   mixed(prop: string): ko.Computed<boolean>,
   empty(prop: string): ko.Computed<boolean>,
 }
 
+/**
+ * Extended version of widget options observable that contains information about mixed and empty styles, and supports
+ * reverting to a previous value.
+ */
+type StyleOptions = modelUtil.SaveableObjObservable<any> & {
+  mixed(prop: string): ko.Computed<boolean>,
+  empty(prop: string): ko.Computed<boolean>,
+  revert(): void;
+}
+
 // This is helper that adds disabled computed to an ObjObservable, it follows
 // the same pattern as `prop` helper.
 function extendObservable(
   obs: modelUtil.SaveableObjObservable<any>,
   options: { [key: string]: (prop: string) => ko.PureComputed<boolean> }
-): CommonOptions {
+) {
   const result = obs as any;
   for(const key of Object.keys(options)) {
     const cacheKey = `__${key}`;

app/client/ui/RightPanel.ts

@@ -199,7 +199,10 @@ export class RightPanel extends Disposable {
     }));
 
     owner.autoDispose(selectedColumns.subscribe(cols => {
-      this._gristDoc.viewModel.activeSection()?.selectedFields(cols || []);
+      if (owner.isDisposed() || this._gristDoc.isDisposed() || this._gristDoc.viewModel.isDisposed()) { return; }
+      const section = this._gristDoc.viewModel.activeSection();
+      if (!section || section.isDisposed()) { return; }
+      section.selectedFields(cols || []);
     }));
     this._gristDoc.viewModel.activeSection()?.selectedFields(selectedColumns.peek() || []);
 

app/client/widgets/CellStyle.ts

@@ -1,20 +1,13 @@
 import {allCommands} from 'app/client/components/commands';
 import {GristDoc} from 'app/client/components/GristDoc';
 import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec';
-import {Style} from 'app/client/models/Styles';
 import {textButton} from 'app/client/ui2018/buttons';
 import {ColorOption, colorSelect} from 'app/client/ui2018/ColorSelect';
 import {theme, vars} from 'app/client/ui2018/cssVars';
 import {ConditionalStyle} from 'app/client/widgets/ConditionalStyle';
-import {Computed, Disposable, dom, DomContents, fromKo, MultiHolder, Observable, styled} from 'grainjs';
+import {Computed, Disposable, dom, DomContents, fromKo, styled} from 'grainjs';
 
 export class CellStyle extends Disposable {
-  private _textColor: Observable<string|undefined>;
-  private _fillColor: Observable<string|undefined>;
-  private _fontBold: Observable<boolean|undefined>;
-  private _fontUnderline: Observable<boolean|undefined>;
-  private _fontItalic: Observable<boolean|undefined>;
-  private _fontStrikethrough: Observable<boolean|undefined>;
 
   constructor(
     private _field: ViewFieldRec,
@@ -22,55 +15,53 @@ export class CellStyle extends Disposable {
     private _defaultTextColor: string
   ) {
     super();
-    this._textColor = fromKo(this._field.config.textColor);
-    this._fillColor = fromKo(this._field.config.fillColor);
-    this._fontBold = fromKo(this._field.config.fontBold);
-    this._fontUnderline = fromKo(this._field.config.fontUnderline);
-    this._fontItalic = fromKo(this._field.config.fontItalic);
-    this._fontStrikethrough = fromKo(this._field.config.fontStrikethrough);
   }
 
   public buildDom(): DomContents {
-    const holder = new MultiHolder();
-    const hasMixedStyle = Computed.create(holder, use => {
-      if (!use(this._field.config.multiselect)) { return false; }
-      const commonStyle = [
-        use(this._field.config.options.mixed('textColor')),
-        use(this._field.config.options.mixed('fillColor')),
-        use(this._field.config.options.mixed('fontBold')),
-        use(this._field.config.options.mixed('fontUnderline')),
-        use(this._field.config.options.mixed('fontItalic')),
-        use(this._field.config.options.mixed('fontStrikethrough'))
-      ];
-      return commonStyle.some(Boolean);
-    });
-    let state: Style[]|null = null;
     return [
       cssLine(
-        cssLabel('CELL STYLE', dom.autoDispose(holder)),
+        cssLabel('CELL STYLE'),
         cssButton('Open row styles', dom.on('click', allCommands.viewTabOpen.run)),
       ),
       cssRow(
-        colorSelect(
+        dom.domComputedOwned(fromKo(this._field.config.style), (holder, options) => {
+          const textColor = fromKo(options.prop("textColor"));
+          const fillColor = fromKo(options.prop("fillColor"));
+          const fontBold = fromKo(options.prop("fontBold"));
+          const fontUnderline = fromKo(options.prop("fontUnderline"));
+          const fontItalic = fromKo(options.prop("fontItalic"));
+          const fontStrikethrough = fromKo(options.prop("fontStrikethrough"));
+          const hasMixedStyle = Computed.create(holder, use => {
+            if (!use(this._field.config.multiselect)) { return false; }
+            const commonStyle = [
+              use(options.mixed('textColor')),
+              use(options.mixed('fillColor')),
+              use(options.mixed('fontBold')),
+              use(options.mixed('fontUnderline')),
+              use(options.mixed('fontItalic')),
+              use(options.mixed('fontStrikethrough'))
+            ];
+            return commonStyle.some(Boolean);
+          });
+          return colorSelect(
             {
               textColor: new ColorOption(
-              { color: this._textColor, defaultColor: this._defaultTextColor, noneText: 'default'}
+                { color: textColor, defaultColor: this._defaultTextColor, noneText: 'default'}
               ),
               fillColor: new ColorOption(
-              { color: this._fillColor, allowsNone: true, noneText: 'none'}
+                { color: fillColor, allowsNone: true, noneText: 'none'}
               ),
-            fontBold: this._fontBold,
+              fontBold: fontBold,
-            fontItalic: this._fontItalic,
+              fontItalic: fontItalic,
-            fontUnderline: this._fontUnderline,
+              fontUnderline: fontUnderline,
-            fontStrikethrough: this._fontStrikethrough
+              fontStrikethrough: fontStrikethrough
             }, {
-            // Calling `field.config.options.save()` saves all options at once.
+              onSave: () => options.save(),
-            onSave: () => this._field.config.options.save(),
+              onRevert: () => options.revert(),
-            onOpen: () => state = this._field.config.copyStyles(),
-            onRevert: () => this._field.config.setStyles(state),
               placeholder: use => use(hasMixedStyle) ? 'Mixed style' : 'Default cell style'
             }
-        )
+          );
+        }),
       ),
       dom.create(ConditionalStyle, "Cell Style", this._field, this._gristDoc, fromKo(this._field.config.multiselect))
     ];

test/nbrowser/MultiColumn.ts

@@ -61,6 +61,29 @@ describe('MultiColumn', function() {
       }
     });
 
+    it('should undo color change', async () => {
+      // This is test for a bug, colors were not saved when "click outside" was done by clicking
+      // one of the cells.
+      await selectColumns('Test1', 'Test2');
+      await gu.setType('Reference');
+      await gu.getCell('Test1', 1).click();
+      await gu.enterCell('Table1', Key.ENTER);
+      await gu.getCell('Test2', 3).click();
+      await gu.enterCell('Table1', Key.ENTER);
+      await selectColumns('Test1', 'Test2');
+      await gu.openColorPicker();
+      await gu.setFillColor(blue);
+      // Clicking on one of the cell caused that the color was not saved.
+      await gu.getCell('Test2', 1).click();
+      // Test if color is set.
+      await gu.assertFillColor(await gu.getCell('Test1', 1), blue);
+      await gu.assertFillColor(await gu.getCell('Test2', 1), blue);
+      // Press undo
+      await gu.undo();
+      await gu.assertFillColor(await gu.getCell('Test1', 1), transparent);
+      await gu.assertFillColor(await gu.getCell('Test2', 1), transparent);
+    });
+
     for (const type of ['Choice', 'Text', 'Reference', 'Numeric']) {
       it(`should reset all columns to first column type for ${type}`, async () => {
         // We start with empty columns, then we will change first one

test/nbrowser/gristUtils.ts

@@ -852,6 +852,9 @@ export async function waitAppFocus(yesNo: boolean = true): Promise<void> {
   await driver.wait(async () => (await driver.find('.copypaste').hasFocus()) === yesNo, 5000);
 }
 
+export async function waitForLabelInput(): Promise<void> {
+  await driver.wait(async () => (await driver.findWait('.kf_elabel_input', 100).hasFocus()), 300);
+}
 
 /**
  * Waits for all pending comm requests from the client to the doc worker to complete. This taps into
@@ -2026,9 +2029,27 @@ export async function setFont(type: 'bold'|'underline'|'italic'|'strikethrough',
   }
 }
 
+/**
+ * Returns the rgb/hex representation of `color` if it's a name (e.g. red, blue, green, white, black, or transparent),
+ * or `color` unchanged if it's not a name.
+ */
+export function nameToHex(color: string) {
+  switch(color) {
+    case 'red': color = '#FF0000'; break;
+    case 'blue': color = '#0000FF'; break;
+    case 'green': color = '#00FF00'; break;
+    case 'white': color = '#FFFFFF'; break;
+    case 'black': color = '#000000'; break;
+    case 'transparent': color = 'rgba(0, 0, 0, 0)'; break;
+  }
+  return color;
+}
+
 //  Set the value of an `<input type="color">` element to `color` and trigger the `change`
-//  event. Accepts `color` to be of following forms `rgb(120, 10, 3)` or '#780a03'.
+//  event. Accepts `color` to be of following forms `rgb(120, 10, 3)` or '#780a03' or some predefined
+//  values (red, green, blue, white, black, transparent)
 export async function setColor(colorInputEl: WebElement, color: string) {
+  color = nameToHex(color);
   if (color.startsWith('rgb(')) {
     // the `value` of an `<input type='color'>` element must be a rgb color in hexadecimal
     // notation.
@@ -2051,11 +2072,25 @@ export function setFillColor(color: string) {
   return setColor(driver.find('.test-fill-input'), color);
 }
 
+export async function clickAway() {
+  await driver.find(".test-notifier-menu-btn").click();
+  await driver.sendKeys(Key.ESCAPE);
+}
+
 export function openColorPicker() {
   return driver.find('.test-color-select').click();
 }
 
+export async function assertCellTextColor(col: string, row: number, color: string) {
+  await assertTextColor(await getCell(col, row).find('.field_clip'), color);
+}
+
+export async function assertCellFillColor(col: string, row: number, color: string) {
+  await assertFillColor(await getCell(col, row), color);
+}
+
 export async function assertTextColor(cell: WebElement, color: string) {
+  color = nameToHex(color);
   color = color.startsWith('#') ? hexToRgb(color) : color;
   const test = async () => {
     const actual = await cell.getCssValue('color');
@@ -2065,6 +2100,7 @@ export async function assertTextColor(cell: WebElement, color: string) {
 }
 
 export async function assertFillColor(cell: WebElement, color: string) {
+  color = nameToHex(color);
   color = color.startsWith('#') ? hexToRgb(color) : color;
   const test = async () => {
     const actual = await cell.getCssValue('background-color');

test/nbrowser/testUtils.ts

@@ -88,18 +88,7 @@ export function setupTestSuite(options?: TestSuiteOptions) {
   // Also, log out, to avoid logins interacting, unless NO_CLEANUP is requested (useful for
   // debugging tests).
   if (!process.env.NO_CLEANUP) {
-    after(async () => {
+    after(() => server.removeLogin());
-      try {
-        await server.removeLogin();
-      } catch(err) {
-        // If there are any alerts open, close them as it might be blocking other tests.
-        if (err.name && err.name === 'UnexpectedAlertOpenError') {
-          await driver.switchTo().alert().accept();
-          assert.fail("Unexpected alert open");
-        }
-        throw err;
-      }
-    });
   }
 
   // If requested, clear user preferences for all test users after this suite.