From 7c8db90aef9aa1035ead6eecf4afa3e8de373abc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 24 Oct 2022 12:06:24 +0200 Subject: [PATCH] (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 | 163 ++++++++++++++++----------- app/client/ui/RightPanel.ts | 5 +- app/client/widgets/CellStyle.ts | 89 +++++++-------- test/nbrowser/MultiColumn.ts | 23 ++++ test/nbrowser/gristUtils.ts | 38 ++++++- test/nbrowser/testUtils.ts | 13 +-- 6 files changed, 205 insertions(+), 126 deletions(-) diff --git a/app/client/models/ViewFieldConfig.ts b/app/client/models/ViewFieldConfig.ts index 5c138f46..4c28ff8a 100644 --- a/app/client/models/ViewFieldConfig.ts +++ b/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; /** If all selected columns have the same widget list. */ public sameWidgets: ko.Computed; - /** 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; // Rest of the options mimic the same options from ViewFieldRec. public wrap: modelUtil.KoSaveableObservable; public widget: ko.Computed; public alignment: modelUtil.KoSaveableObservable; - public textColor: modelUtil.KoSaveableObservable; - public fillColor: modelUtil.KoSaveableObservable; - public fontBold: modelUtil.KoSaveableObservable; - public fontUnderline: modelUtil.KoSaveableObservable; - public fontItalic: modelUtil.KoSaveableObservable; - public fontStrikethrough: modelUtil.KoSaveableObservable; - private _fields: ko.PureComputed; + public fields: ko.PureComputed; 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|null = null; @@ -122,18 +118,7 @@ export class ViewFieldConfig { } } } - // Add cell style options, as they are common to all widgets. - 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; + return options ?? new Set(); })); // 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. - mixed: prop => ko.pureComputed(() => !allSame(this._fields().map(f => f.widgetOptionsJson.prop(prop)()))), + // Property has mixed value, if not all options are the same. + 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'); - this.fontBold = this.options.prop('fontBold'); - this.fontUnderline = this.options.prop('fontUnderline'); - this.fontItalic = this.options.prop('fontItalic'); - this.fontStrikethrough = this.options.prop('fontStrikethrough'); + + // Style options are a bit different, as they are saved when style picker is disposed. + // By the time it happens, fields may have changed (since user might have clicked some other column). + // To support this use case we need to compute a snapshot of fields, and use it to save style. Style + // picker will be rebuild every time fields change, and it will have access to last selected fields + // 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 & { disabled(prop: string): ko.Computed, mixed(prop: string): ko.Computed, empty(prop: string): ko.Computed, } +/** + * 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 & { + mixed(prop: string): ko.Computed, + empty(prop: string): ko.Computed, + 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, options: { [key: string]: (prop: string) => ko.PureComputed } -): CommonOptions { +) { const result = obs as any; for(const key of Object.keys(options)) { const cacheKey = `__${key}`; diff --git a/app/client/ui/RightPanel.ts b/app/client/ui/RightPanel.ts index 42476f38..d65dc665 100644 --- a/app/client/ui/RightPanel.ts +++ b/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() || []); diff --git a/app/client/widgets/CellStyle.ts b/app/client/widgets/CellStyle.ts index 50ffcdcc..761bbb66 100644 --- a/app/client/widgets/CellStyle.ts +++ b/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; - private _fillColor: Observable; - private _fontBold: Observable; - private _fontUnderline: Observable; - private _fontItalic: Observable; - private _fontStrikethrough: Observable; 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( - { - textColor: new ColorOption( - { color: this._textColor, defaultColor: this._defaultTextColor, noneText: 'default'} - ), - fillColor: new ColorOption( - { color: this._fillColor, allowsNone: true, noneText: 'none'} - ), - fontBold: this._fontBold, - fontItalic: this._fontItalic, - fontUnderline: this._fontUnderline, - fontStrikethrough: this._fontStrikethrough - }, { - // Calling `field.config.options.save()` saves all options at once. - onSave: () => this._field.config.options.save(), - onOpen: () => state = this._field.config.copyStyles(), - onRevert: () => this._field.config.setStyles(state), - placeholder: use => use(hasMixedStyle) ? 'Mixed style' : 'Default cell style' - } - ) + 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: textColor, defaultColor: this._defaultTextColor, noneText: 'default'} + ), + fillColor: new ColorOption( + { color: fillColor, allowsNone: true, noneText: 'none'} + ), + fontBold: fontBold, + fontItalic: fontItalic, + fontUnderline: fontUnderline, + fontStrikethrough: fontStrikethrough + }, { + onSave: () => options.save(), + onRevert: () => options.revert(), + placeholder: use => use(hasMixedStyle) ? 'Mixed style' : 'Default cell style' + } + ); + }), ), dom.create(ConditionalStyle, "Cell Style", this._field, this._gristDoc, fromKo(this._field.config.multiselect)) ]; diff --git a/test/nbrowser/MultiColumn.ts b/test/nbrowser/MultiColumn.ts index 5daf8665..28bf1633 100644 --- a/test/nbrowser/MultiColumn.ts +++ b/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 diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 4db44f3b..bdd57bec 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -852,6 +852,9 @@ export async function waitAppFocus(yesNo: boolean = true): Promise { await driver.wait(async () => (await driver.find('.copypaste').hasFocus()) === yesNo, 5000); } +export async function waitForLabelInput(): Promise { + 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 `` 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 `` 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'); diff --git a/test/nbrowser/testUtils.ts b/test/nbrowser/testUtils.ts index 1b5a0ff9..37caa780 100644 --- a/test/nbrowser/testUtils.ts +++ b/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 () => { - 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; - } - }); + after(() => server.removeLogin()); } // If requested, clear user preferences for all test users after this suite.