(core) Polish "Click to insert $id" tooltip

Summary:
The tooltip also broke recently on Firefox after upgrading Ace. This
includes a fix for the regression.

Test Plan: Updated test.

Reviewers: jarek

Reviewed By: jarek

Subscribers: jarek, dsagal

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

app/client/components/GridView.js

@@ -1097,7 +1097,6 @@ GridView.prototype.buildDom = function() {
                 kd.toggleClass('font-strikethrough', headerFontStrikethrough),
                 kd.style('--frozen-position', () => ko.unwrap(this.frozenPositions.at(field._index()))),
                 kd.toggleClass("frozen", () => ko.unwrap(this.frozenMap.at(field._index()))),
-                kd.toggleClass("hover-column", isTooltip),
                 dom.autoDispose(isEditingLabel),
                 dom.autoDispose(isTooltip),
                 dom.testId("GridView_columnLabel"),
@@ -1136,8 +1135,7 @@ GridView.prototype.buildDom = function() {
                     optCommands: renameCommands
                   }),
                 ),
-                dom.on("mouseenter", () => self.changeHover(field._index())),
+                self._showTooltipOnHover(field, isTooltip),
-                dom.on("mouseleave", () => self.changeHover(-1)),
                 self.isPreview ? null : menuToggle(null,
                   kd.cssClass('g-column-main-menu'),
                   kd.cssClass('g-column-menu-btn'),
@@ -1347,6 +1345,12 @@ GridView.prototype.buildDom = function() {
               ko.unwrap(self.isColSelected.at(field._index())) &&
               self.cellSelector.isRowSelected(row._index());
           });
+
+          var isTooltip = ko.pureComputed(() =>
+            self.editingFormula() &&
+            ko.unwrap(self.hoverColumn) === field._index()
+          );
+
           return dom(
             'div.field',
             kd.style('--frozen-position', () => ko.unwrap(self.frozenPositions.at(field._index()))),
@@ -1356,10 +1360,7 @@ GridView.prototype.buildDom = function() {
             dom.autoDispose(isCellSelected),
             dom.autoDispose(isCellActive),
             dom.autoDispose(isSelected),
-            dom.on("mouseenter", () => self.changeHover(field._index())),
+            self._showTooltipOnHover(field, isTooltip),
-            kd.toggleClass("hover-column", () =>
-              self.editingFormula() &&
-              ko.unwrap(self.hoverColumn) === (field._index())),
             kd.style('width', field.widthPx),
             //TODO: Ensure that fields in a row resize when
             //a cell in that row becomes larger
@@ -1435,11 +1436,15 @@ GridView.prototype._createColSelectedObs = function(col) {
 // Callbacks for mouse events for the selector object
 
 GridView.prototype.cellMouseDown = function(elem, event) {
+  let col = this.domToColModel(elem, selector.CELL);
+  if (this.hoverColumn() === col._index()) {
+    return this._tooltipMouseDown(elem, selector.CELL);
+  }
+
   if (event.shiftKey) {
     // Change focus before running command so that the correct viewsection's cursor is moved.
     this.viewSection.hasFocus(true);
     let row = this.domToRowModel(elem, selector.CELL);
-    let col = this.domToColModel(elem, selector.CELL);
     this.cellSelector.selectArea(this.cursor.rowIndex(), this.cursor.fieldIndex(),
                                  row._index(), col._index());
   } else {
@@ -1448,12 +1453,24 @@ GridView.prototype.cellMouseDown = function(elem, event) {
 };
 
 GridView.prototype.colMouseDown = function(elem, event) {
+  let col = this.domToColModel(elem, selector.COL);
+  if (this.hoverColumn() === col._index()) {
+    return this._tooltipMouseDown(elem, selector.COL);
+  }
+
   this._colClickTime = Date.now();
   this.assignCursor(elem, selector.COL);
   // Clicking the column header selects all rows except the add row.
   this.cellSelector.row.end(this.getLastDataRowIndex());
 };
 
+GridView.prototype._tooltipMouseDown = function(elem, elemType) {
+  let row = this.domToRowModel(elem, elemType);
+  let col = this.domToColModel(elem, elemType);
+  // FormulaEditor.ts overrides this command to insert the column id of the clicked column.
+  commands.allCommands.setCursor.run(row, col);
+};
+
 GridView.prototype.rowMouseDown = function(elem, event) {
   if (event.shiftKey) {
     this.cellSelector.currentSelectType(selector.ROW);
@@ -1468,12 +1485,16 @@ GridView.prototype.rowMouseMove = function(event) {
 };
 
 GridView.prototype.colMouseMove = function(event) {
+  if (this.editingFormula()) { return; }
+
   var currentCol = Math.min(this.getMousePosCol(event.pageX),
                             this.viewSection.viewFields().peekLength - 1);
   this.cellSelector.col.end(currentCol);
 };
 
 GridView.prototype.cellMouseMove = function(event) {
+  if (this.editingFormula()) { return; }
+
   this.colMouseMove(event);
   this.rowMouseMove(event);
   // Maintain single cells cannot be selected invariant
@@ -1785,6 +1806,20 @@ GridView.prototype._clearCopySelection = function() {
   this.copySelection(null);
 };
 
+GridView.prototype._showTooltipOnHover = function(field, isShowingTooltip) {
+  return [
+    kd.toggleClass("hover-column", isShowingTooltip),
+    dom.on('mouseenter', () => {
+      this.changeHover(field._index());
+    }),
+    dom.on('mousedown', (ev) => {
+      if (isShowingTooltip()) {
+        ev.preventDefault();
+      }
+    }),
+  ];
+};
+
 function buildStyleOption(owner, computedRule, optionName) {
   return ko.computed(() => {
     if (owner.isDisposed()) { return null; }

app/client/widgets/FormulaEditor.ts

@@ -421,15 +421,13 @@ export class FormulaEditor extends NewBaseEditor {
 
     const colId = col.origCol.peek().colId.peek();
 
-    const aceObj = this._aceEditor.getEditor();
-
-    // Rect only to columns in the same table.
     if (col.tableId.peek() !== this.options.column.table.peek().tableId.peek()) {
-      // aceObj.focus();
+      // Fall back to default behavior if cursor didn't move to a column in the same table.
       this.options.gristDoc.onSetCursorPos(row, col).catch(reportError);
       return;
     }
 
+    const aceObj = this._aceEditor.getEditor();
     if (!aceObj.selection.isEmpty()) {
       // If text selected, replace whole selection
       aceObj.session.replace(aceObj.selection.getRange(), '$' + colId);

test/nbrowser/BundleActions.ts

@@ -0,0 +1,118 @@
+/**
+ * Test for action bundling, e.g. when changing column type. Before the action is finalized, the
+ * user has a chance to make further changes that are part of the bundle. If any change is
+ * attempted that doesn't belong in the bundle, the bundle should be finalized before the change
+ * is appled.
+ */
+import {assert, driver, Key} from 'mocha-webdriver';
+import * as gu from 'test/nbrowser/gristUtils';
+import {setupTestSuite} from 'test/nbrowser/testUtils';
+
+describe('BundleActions', function() {
+  this.timeout(30000);
+  const cleanup = setupTestSuite();
+
+  before(async function() {
+    const mainSession = await gu.session().login();
+    await mainSession.tempNewDoc(cleanup, 'TransformBug');
+
+    // Import a file
+    await gu.importFileDialog('./uploads/UploadedData1.csv');
+    assert.equal(await driver.findWait('.test-importer-preview', 2000).isPresent(), true);
+    await driver.find('.test-modal-confirm').click();
+    await gu.waitForServer();
+  });
+
+  it('should complete transform if column is added during it', async function() {
+    // Start a transform.
+    await gu.getCell({col: 'Name', rowNum: 1}).click();
+    // This does not include a click on the "Apply" button.
+    await gu.setType(/Reference/);
+    assert.equal(await gu.getCell({col: 'Name', rowNum: 1}).matches('.transform_field'), true);
+
+    // Add a column while inside the transform.
+    await driver.find('body').sendKeys(Key.chord(Key.ALT, Key.SHIFT, '='));
+    await gu.waitForServer();
+
+    // Check there are no user-visible errors at this point.
+    assert.equal(await driver.find('.test-notifier-toast-message').isPresent(), false);
+    await gu.checkForErrors();
+
+    // Close column-rename textbox.
+    await driver.sendKeys(Key.ESCAPE);
+
+    // Check the Name column is no longer being transformed. Cells are invalid because it's now an
+    // invalid reference.
+    let cell = gu.getCell({col: 'Name', rowNum: 1});
+    assert.equal(await cell.matches('.transform_field'), false);
+    assert.equal(await cell.find('.field_clip').matches('.invalid'), true);
+
+    // Do something with the new column.
+    await driver.sendKeys("HELLO", Key.ENTER);
+    await gu.waitForServer();
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'A', rowNums: [1, 2, 3]}), ['HELLO', '', '']);
+    await gu.enterFormula('str($Name).upper()');
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'A', rowNums: [1, 2, 3]}), ['LILY', 'KATHY', 'KAREN']);
+
+    await gu.checkForErrors();
+
+    // Undo the column changes and the column addition.
+    await gu.undo(3);
+
+    // The transform result is still applied
+    cell = gu.getCell({col: 'Name', rowNum: 1});
+    assert.equal(await cell.find('.field_clip').matches('.invalid'), true);
+    assert.equal(await cell.matches('.transform_field'), false);
+    assert.equal(await driver.find('.test-fbuilder-type-select').getText(), "Reference");
+
+    // Undo the transform now.
+    await gu.undo();
+
+    cell = gu.getCell({col: 'Name', rowNum: 1});
+    assert.equal(await cell.find('.field_clip').matches('.invalid'), false);
+    assert.equal(await cell.matches('.transform_field'), false);
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'Name', rowNums: [1, 2, 3]}), ['Lily', 'Kathy', 'Karen']);
+    assert.equal(await driver.find('.test-fbuilder-type-select').getText(), "Text");
+    await gu.checkForErrors();
+
+    // For good measure, check that REDO works too.
+    await gu.redo(4);
+    assert.deepEqual(await gu.getVisibleGridCells({col: 'A', rowNums: [1, 2, 3]}), ['LILY', 'KATHY', 'KAREN']);
+    cell = gu.getCell({col: 'Name', rowNum: 1});
+    assert.equal(await cell.find('.field_clip').matches('.invalid'), true);
+    assert.equal(await cell.matches('.transform_field'), false);
+    await cell.click();
+    assert.equal(await driver.find('.test-fbuilder-type-select').getText(), "Reference");
+    await gu.checkForErrors();
+
+    // And back to where we started.
+    await gu.undo(4);
+  });
+
+  it('should complete transform if a page widget is added during it', async function() {
+    // Start a transform.
+    await gu.getCell({col: 'Name', rowNum: 1}).click();
+    await gu.setType(/Reference/);     // This does not include a click on the "Apply" button.
+    assert.equal(await gu.getCell({col: 'Name', rowNum: 1}).matches('.transform_field'), true);
+
+    await gu.addNewSection(/Table/, /New Table/);
+
+    // Check there are no user-visible errors at this point.
+    assert.equal(await driver.find('.test-notifier-toast-message').isPresent(), false);
+    await gu.checkForErrors();
+
+    // Check that we see two sections.
+    assert.deepEqual(await gu.getSectionTitles(), ['UPLOADEDDATA1', 'TABLE2']);
+    await gu.getCell({col: 'Name', rowNum: 1, section: "UPLOADEDDATA1"}).click();
+    assert.equal(await driver.find('.test-fbuilder-type-select').getText(), "Reference");
+
+    // Undo both actions.
+    await gu.undo(2);
+    assert.deepEqual(await gu.getSectionTitles(), ['UPLOADEDDATA1']);
+    await gu.getCell({col: 'Name', rowNum: 1, section: "UPLOADEDDATA1"}).click();
+    assert.equal(await driver.find('.test-fbuilder-type-select').getText(), "Text");
+
+    assert.equal(await driver.find('.test-notifier-toast-message').isPresent(), false);
+    await gu.checkForErrors();
+  });
+});