From c1de16aee74d5aa41f489360b23a0cd6811d122f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Thu, 16 Dec 2021 19:00:04 +0100 Subject: [PATCH] (core) Scrolling to the active record on search Summary: Two bugs fixed: 1. On search, when the first result is in the active record, GridView wasn't scrolling to the active record. 2. When an active record was not visible, GridView wasn't scrolling to the active record when the column index was changed. The problem was that the scrolling behavior was based only on rowIndex which isn't changed (and doesn't notify subscribers) when a column index changes or when the search highlights a cell. This diff makes the computed depend also on the fieldIndex, and is introducing a new method that can scroll to the active record on demand (which is used by the search). Test Plan: Updated tests. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3191 --- app/client/components/BaseView.js | 8 ++++++ app/client/components/GridView.js | 28 ++++++++++++++++--- app/client/declarations.d.ts | 1 + app/client/lib/koDom.js | 45 ++++++++++++++++++++----------- app/client/models/SearchModel.ts | 5 +++- 5 files changed, 67 insertions(+), 20 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 249af265..76c307c0 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -693,4 +693,12 @@ BaseView.prototype.isFiltered = function() { return this._filteredRowSource.getNumRows() < this.tableModel.tableData.numRecords(); }; +/** + * Makes sure that active record is in the view. + */ +BaseView.prototype.revealActiveRecord = function() { + // to override + return Promise.resolve(); +}; + module.exports = BaseView; diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index d6a597bf..52b93fb2 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -27,7 +27,7 @@ const {reportError} = require('app/client/models/AppModel'); const {onDblClickMatchElem} = require('app/client/lib/dblclick'); // Grist UI Components -const {Holder} = require('grainjs'); +const {Holder, Computed} = require('grainjs'); const {menu} = require('../ui2018/menus'); const {calcFieldsCondition} = require('../ui/GridViewMenus'); const {ColumnAddMenu, ColumnContextMenu, MultiColumnMenu, freezeAction} = require('../ui/GridViewMenus'); @@ -86,6 +86,24 @@ function GridView(gristDoc, viewSectionModel, isPreview = false) { return tree; })); + // Create observable holding current rowIndex that the view should be scrolled to. + // We will always notify, because we want to scroll to the row even when only the + // column is changed (in situation when the row is not visible). + this.visibleRowIndex = ko.observable(this.cursor.rowIndex()).extend({notify: 'always'}); + // Create grain's Computed with current cursor position (we need it to examine position + // before the change and after). + this.currentPosition = Computed.create(this, (use) => ({ + rowIndex : use(this.cursor.rowIndex), + fieldIndex : use(this.cursor.fieldIndex) + })); + // Add listener, and check if the cursor is indeed changed, if so, update the row + // and scroll it into view (using kd.scrollChildIntoView in buildDom function). + this.autoDispose(this.currentPosition.addListener((cur, prev) => { + if (cur.rowIndex !== prev.rowIndex || cur.fieldIndex !== prev.fieldIndex) { + this.visibleRowIndex(cur.rowIndex); + } + })); + this.autoDispose(this.cursor.fieldIndex.subscribe(idx => { const offset = this.colRightOffsets.peek().getSumTo(idx); @@ -745,7 +763,7 @@ GridView.prototype._getColStyle = function(colIndex) { }; -// TODO: for now lets just assume youre clicking on a .field, .row, or .column +// TODO: for now lets just assume you are clicking on a .field, .row, or .column GridView.prototype.domToRowModel = function(elem, elemType) { switch (elemType) { case selector.COL: @@ -859,7 +877,7 @@ GridView.prototype.buildDom = function() { self.scrollPane = dom('div.grid_view_data.gridview_data_scroll.show_scrollbar', - kd.scrollChildIntoView(self.cursor.rowIndex), + kd.scrollChildIntoView(self.visibleRowIndex), dom.onDispose(() => { // Save the previous scroll values to the section. self.viewSection.lastScrollPos = _.extend({ @@ -1410,6 +1428,10 @@ GridView.prototype.maybeSelectRow = function(elem, rowId) { } }; +GridView.prototype.revealActiveRecord = function() { + return kd.doScrollChildIntoView(this.scrollPane, this.cursor.rowIndex()); +} + // End Context Menus module.exports = GridView; diff --git a/app/client/declarations.d.ts b/app/client/declarations.d.ts index 6b44ed23..6aa36f9a 100644 --- a/app/client/declarations.d.ts +++ b/app/client/declarations.d.ts @@ -72,6 +72,7 @@ declare module "app/client/components/BaseView" { public onResize(): void; public prepareToPrint(onOff: boolean): void; public moveEditRowToCursor(): DataRowModel; + public revealActiveRecord(): Promise; } export = BaseView; } diff --git a/app/client/lib/koDom.js b/app/client/lib/koDom.js index 50a5bf67..fc3c46a4 100644 --- a/app/client/lib/koDom.js +++ b/app/client/lib/koDom.js @@ -276,34 +276,47 @@ exports.cssClass = cssClass; * whose value is the index of the child element to keep scrolled into view. */ function scrollChildIntoView(valueOrFunc) { - return makeBinding(valueOrFunc, function(elem, index) { - if (index === null) { - return; - } - var scrolly = ko.utils.domData.get(elem, "scrolly"); - if (scrolly) { - // Delay this in case it's triggered while other changes are processed (e.g. splices). - setTimeout(() => scrolly.isDisposed() || scrolly.scrollRowIntoView(index), 0); - } else { - var child = elem.children[index]; - if (!child) { - return; - } + return makeBinding(valueOrFunc, doScrollChildIntoView); +} +function doScrollChildIntoView(elem, index) { + if (index === null) { + return Promise.resolve(); + } + const scrolly = ko.utils.domData.get(elem, "scrolly"); + if (scrolly) { + // Delay this in case it's triggered while other changes are processed (e.g. splices). + return new Promise((resolve, reject) => { + setTimeout(() => { + try { + if (!scrolly.isDisposed()) { + scrolly.scrollRowIntoView(index); + } + resolve(); + } catch(err) { + reject(err); + } + }, 0); + }); + } else { + const child = elem.children[index]; + if (child) { if (index === 0) { // Scroll the container all the way if showing the first child. elem.scrollTop = 0; } - var childRect = child.getBoundingClientRect(); - var parentRect = elem.getBoundingClientRect(); + const childRect = child.getBoundingClientRect(); + const parentRect = elem.getBoundingClientRect(); if (childRect.top < parentRect.top) { child.scrollIntoView(true); // Align with top if scrolling up.. } else if (childRect.bottom > parentRect.bottom) { child.scrollIntoView(false); // ..bottom if scrolling down. } } - }); + return Promise.resolve(); + } } exports.scrollChildIntoView = scrollChildIntoView; +exports.doScrollChildIntoView = doScrollChildIntoView; /** diff --git a/app/client/models/SearchModel.ts b/app/client/models/SearchModel.ts index 0cefb235..5c760839 100644 --- a/app/client/models/SearchModel.ts +++ b/app/client/models/SearchModel.ts @@ -309,9 +309,12 @@ class FinderImpl implements IFinder { // this ad-hoc way rather than use observables, to avoid the overhead of *every* cell // depending on an additional observable. await delay(0); - const viewInstance = await waitObs(section.viewInstance); + const viewInstance = (await waitObs(section.viewInstance))!; await viewInstance.getLoadingDonePromise(); if (this._aborted) { return; } + // Make sure we are at good place. This is important when the cursor + // was already in a matched record, but the record was scrolled away. + await viewInstance.revealActiveRecord(); const cursor = viewInstance.viewPane.querySelector('.selected_cursor'); if (cursor) {