(core) Fix linking of new records when attachment is the first thing added.

Summary:
Fixes a bug when in a linked widget, the automatic reference wasn't being set
for a new record if attachment is the first thing that gets added to the
record.

- Move handling of 'setCursorPos' pseudo-command to GristDoc to support cross-section switching (relevant when moving attachment into a cell of a non-active page widget)
- Modernize code for AttachmentsWidget slightly (better typings, css conventions)
- Change the fix in https://phab.getgrist.com/D3796 from using isolate to using different z-index values, to avoid a change in the look of the cursor on Attachment cells.

Test Plan: Added a test case for what's possible to test with webdriver.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3811
pull/461/head
Dmitry S 1 year ago
parent 055522d374
commit 1274fe55fb

@ -51,13 +51,6 @@ export class Cursor extends Disposable {
moveToLastRecord(this: Cursor) { this.rowIndex(Infinity); },
moveToFirstField(this: Cursor) { this.fieldIndex(0); },
moveToLastField(this: Cursor) { this.fieldIndex(Infinity); },
// Command to be manually triggered on cell selection. Moves the cursor to the selected cell.
// This is overridden by the formula editor to insert "$col" variables when clicking cells.
setCursor(this: Cursor, rowModel: BaseRowModel, fieldModel: BaseRowModel) {
this.rowIndex(rowModel ? rowModel._index() : 0);
this.fieldIndex(fieldModel ? fieldModel._index()! : 0);
},
};
public viewData: LazyArrayModel<BaseRowModel>;

@ -22,7 +22,7 @@
overflow: auto;
overscroll-behavior: none;
z-index: 2; /* scrollbar should be over the overlay background */
z-index: 20; /* scrollbar should be over the overlay background */
border-top: 1px solid var(--grist-theme-table-header-border, lightgrey);
}
@ -36,7 +36,7 @@
position: -webkit-sticky;
position: sticky;
top: 0px;
z-index: 2; /* z-index must be here, doesnt work on children*/
z-index: 20; /* z-index must be here, doesnt work on children*/
}
.gridview_data_header {
@ -74,7 +74,7 @@
border-bottom: 1px solid var(--grist-theme-table-header-border-dark, var(--grist-color-dark-grey));
color: var(--grist-theme-table-header-fg, unset);
background-color: var(--grist-theme-table-header-bg, var(--grist-color-light-grey));
z-index: 2; /* goes over data cells */
z-index: 20; /* goes over data cells */
padding-top: 2px;
text-align: center;
@ -159,7 +159,7 @@
height: calc(var(--gridview-header-height) + 1px); /* matches gridview_data_header height (+border) */
top: 1px; /* go under 1px border on scrollpane */
border-bottom: 1px solid var(--grist-theme-table-header-border, lightgray);
z-index: 3;
z-index: 30;
cursor: pointer;
}
@ -182,7 +182,7 @@
/* shadow should only show to the right of it (10px should be enough) */
-webkit-clip-path: polygon(0 0, 10px 0, 10px 100%, 0 100%);
clip-path: polygon(0 0, 10px 0, 10px 100%, 0 100%);
z-index: 3;
z-index: 30;
}
/* Right shadow - normally not displayed - activated when grid has frozen columns */
@ -193,7 +193,7 @@
box-shadow: -8px 0 14px 4px var(--grist-theme-table-scroll-shadow, #444);
-webkit-clip-path: polygon(0 0, 10px 0, 10px 100%, 0 100%);
clip-path: polygon(0 0, 28px 0, 24px 100%, 0 100%);
z-index: 3;
z-index: 30;
position: absolute;
}
@ -207,7 +207,7 @@
*/
left: calc(4em + var(--frozen-width, 0) * 1px);
background-color: var(--grist-theme-table-frozen-columns-border, #999999);
z-index: 3;
z-index: 30;
user-select: none;
pointer-events: none
}
@ -222,14 +222,14 @@
/* should only show below it (10px should be enough) */
-webkit-clip-path: polygon(0 0, 0 10px, 100% 10px, 100% 0);
clip-path: polygon(0 0, 0 10px, 100% 10px, 100% 0);
z-index: 3;
z-index: 30;
}
.gridview_header_backdrop_left {
width: calc(4rem + 1px); /* Matches rowid width (+border) */
height:100%;
top: 1px; /* go under 1px border on scrollpane */
z-index: 1;
z-index: 10;
border-right: 1px solid var(--grist-theme-table-header-border, lightgray);
}
@ -237,7 +237,7 @@
position: absolute;
width: 0px; /* Matches rowid width (+border) */
height: 100%;
z-index: 3;
z-index: 30;
border-right: 1px solid var(--grist-theme-table-body-border, var(--grist-color-dark-grey)) !important;
user-select: none;
pointer-events: none
@ -248,7 +248,7 @@
height: calc(var(--gridview-header-height) + 1px); /* matches gridview_data_header height (+border) */
top: 1px; /* go under 1px border on scrollpane */
border-bottom: 1px solid var(--grist-theme-table-header-border, lightgray);
z-index: 1;
z-index: 10;
}
.gridview_data_pane > .scroll_shadow_top {
@ -269,7 +269,7 @@
height: 100%;
position: absolute;
border: 2px solid var(--grist-theme-table-drag-drop-indicator, gray);
z-index: 20;
z-index: 200;
top: 0px;
}
@ -278,7 +278,7 @@
height: 100%;
position: absolute;
border: 1px solid var(--grist-theme-table-drag-drop-indicator, gray);
z-index: 15;
z-index: 150;
top: 0px;
background-color: var(--grist-theme-table-drag-drop-shadow, #F0F0F0);
opacity: 0.5;
@ -289,7 +289,7 @@
height: 0px;
position: absolute;
border: 2px solid var(--grist-theme-table-drag-drop-indicator, gray);
z-index: 20;
z-index: 200;
left: 0px;
}
@ -298,7 +298,7 @@
height: 0px;
position: absolute;
border: 1px solid var(--grist-theme-table-drag-drop-indicator, gray);
z-index: 15;
z-index: 150;
left: 0px;
background-color: var(--grist-theme-table-drag-drop-shadow, #F0F0F0);
opacity: 0.5;
@ -311,7 +311,7 @@
.record .field.frozen {
position: sticky;
left: calc(4em + 1px + (var(--frozen-position, 0) - var(--frozen-offset, 0)) * 1px); /* 4em for row number + total width of cells + 1px for border*/
z-index: 1;
z-index: 10;
}
/* for data field we need to reuse color from record (add-row and zebra stripes) */
.gridview_row .record .field.frozen {

@ -885,7 +885,7 @@ GridView.prototype._getColStyle = function(colIndex) {
GridView.prototype.domToRowModel = function(elem, elemType) {
switch (elemType) {
case selector.COL:
return 0;
return undefined;
case selector.ROW: // row > row num: row has record model
return ko.utils.domData.get(elem.parentNode, 'itemModel');
case selector.NONE:
@ -899,7 +899,7 @@ GridView.prototype.domToRowModel = function(elem, elemType) {
GridView.prototype.domToColModel = function(elem, elemType) {
switch (elemType) {
case selector.ROW:
return 0;
return undefined;
case selector.NONE:
case selector.CELL: // cell: .field has col model
case selector.COL: // col: .column_name I think

@ -31,10 +31,11 @@ import {createSessionObs} from 'app/client/lib/sessionObs';
import {setTestState} from 'app/client/lib/testState';
import {selectFiles} from 'app/client/lib/uploads';
import {reportError} from 'app/client/models/AppModel';
import BaseRowModel from 'app/client/models/BaseRowModel';
import DataTableModel from 'app/client/models/DataTableModel';
import {DataTableModelWithDiff} from 'app/client/models/DataTableModelWithDiff';
import {DocData} from 'app/client/models/DocData';
import {DocInfoRec, DocModel, ViewRec, ViewSectionRec} from 'app/client/models/DocModel';
import {DocInfoRec, DocModel, ViewFieldRec, ViewRec, ViewSectionRec} from 'app/client/models/DocModel';
import {DocPageModel} from 'app/client/models/DocPageModel';
import {UserError} from 'app/client/models/errors';
import {urlState} from 'app/client/models/gristUrlState';
@ -361,6 +362,16 @@ export class GristDoc extends DisposableWithEvents {
undo(this: GristDoc) { this._undoStack.sendUndoAction().catch(reportError); },
redo(this: GristDoc) { this._undoStack.sendRedoAction().catch(reportError); },
reloadPlugins() { this.docComm.reloadPlugins().then(() => G.window.location.reload(false)); },
// Command to be manually triggered on cell selection. Moves the cursor to the selected cell.
// This is overridden by the formula editor to insert "$col" variables when clicking cells.
setCursor(this: GristDoc, rowModel: BaseRowModel, fieldModel?: ViewFieldRec) {
return this.setCursorPos({
rowIndex: rowModel?._index() || 0,
fieldIndex: fieldModel?._index() || 0,
sectionId: fieldModel?.viewSection().getRowId(),
});
},
}, this, true));
this.listenTo(app.comm, 'docUserAction', this.onDocUserAction);
@ -507,6 +518,21 @@ export class GristDoc extends DisposableWithEvents {
return Object.assign(pos, viewInstance ? viewInstance.cursor.getCursorPos() : {});
}
public async setCursorPos(cursorPos: CursorPos) {
if (cursorPos.sectionId) {
const desiredSection: ViewSectionRec = this.docModel.viewSections.getRowModel(cursorPos.sectionId);
if (desiredSection.view.peek().getRowId() !== this.activeViewId.get()) {
// This may be asynchronous. In other cases, the change is synchronous, and some code
// relies on it (doesn't wait for this function to resolve).
await this._switchToSectionId(cursorPos.sectionId);
} else if (desiredSection !== this.viewModel.activeSection.peek()) {
this.viewModel.activeSectionId(cursorPos.sectionId);
}
}
const viewInstance = this.viewModel.activeSection.peek().viewInstance.peek();
viewInstance?.setCursorPos(cursorPos);
}
/**
* Switch to the view/section and scroll to the record indicated by cursorPos. If cursorPos is
* null, then moves to a position best suited for optActionGroup (not yet implemented).
@ -523,10 +549,7 @@ export class GristDoc extends DisposableWithEvents {
return;
}
try {
const viewInstance = await this._switchToSectionId(cursorPos.sectionId);
if (viewInstance) {
viewInstance.setCursorPos(cursorPos);
}
await this.setCursorPos(cursorPos);
} catch(e) {
reportError(e);
}

@ -48,6 +48,12 @@ interface Attachment {
* download, add or remove attachments in the edited cell.
*/
export class AttachmentsEditor extends NewBaseEditor {
public static skipEditor(typedVal: CellValue|undefined, origVal: CellValue): CellValue|undefined {
if (Array.isArray(typedVal)) {
return typedVal;
}
}
private _attachmentsTable: MetaTableData<'_grist_Attachments'>;
private _docComm: DocComm;

@ -1,18 +0,0 @@
.attachment_hover_icon {
display: none;
}
.attachment_widget {
/* Create a new stacking context, primary for frozen columns which have z-index:1 and the icon
inside this widget (with z-index:1) is visible over the frozen column. */
isolation: isolate;
}
.attachment_widget:hover .attachment_hover_icon {
display: inline;
}
.attachment_drag_over {
outline: 2px dashed #ff9a00;
outline-offset: -2px;
}

@ -1,66 +1,21 @@
import {Computed, dom, fromKo, input, makeTestId, onElem, styled, TestId} from 'grainjs';
import {CellValue} from "app/common/DocActions";
import * as commands from 'app/client/components/commands';
import {dragOverClass} from 'app/client/lib/dom';
import {selectFiles, uploadFiles} from 'app/client/lib/uploads';
import {cssRow} from 'app/client/ui/RightPanelStyles';
import {colors, vars} from 'app/client/ui2018/cssVars';
import {colors, testId, vars} from 'app/client/ui2018/cssVars';
import {NewAbstractWidget} from 'app/client/widgets/NewAbstractWidget';
import {encodeQueryParams} from 'app/common/gutil';
import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec';
import {DataRowModel} from 'app/client/models/DataRowModel';
import {MetaTableData} from 'app/client/models/TableData';
import { SingleCell } from 'app/common/TableData';
import {KoSaveableObservable} from 'app/client/models/modelUtil';
import {UploadResult} from 'app/common/uploads';
import { GristObjCode } from 'app/plugin/GristData';
import {Computed, dom, fromKo, input, onElem, styled} from 'grainjs';
import {extname} from 'path';
import { SingleCell } from 'app/common/TableData';
const testId: TestId = makeTestId('test-pw-');
const attachmentWidget = styled('div.attachment_widget.field_clip', `
display: flex;
flex-wrap: wrap;
white-space: pre-wrap;
`);
const attachmentIcon = styled('div.attachment_icon.glyphicon.glyphicon-paperclip', `
position: absolute;
top: 2px;
left: 2px;
padding: 2px;
background-color: #D0D0D0;
color: white;
border-radius: 2px;
border: none;
cursor: pointer;
box-shadow: 0 0 0 1px white;
z-index: 1;
&:hover {
background-color: #3290BF;
}
`);
const attachmentPreview = styled('div', `
color: black;
background-color: white;
border: 1px solid #bbb;
margin: 0 2px 2px 0;
position: relative;
display: flex;
align-items: center;
justify-content: center;
z-index: 0;
&:hover {
border-color: ${colors.lightGreen};
}
`);
const sizeLabel = styled('div', `
color: ${colors.slate};
margin-right: 9px;
`);
export interface SavingObservable<T> extends ko.Observable<T> {
setAndSave(value: T): void;
}
/**
* AttachmentsWidget - A widget for displaying attachments as image previews.
@ -68,46 +23,49 @@ export interface SavingObservable<T> extends ko.Observable<T> {
export class AttachmentsWidget extends NewAbstractWidget {
private _attachmentsTable: MetaTableData<'_grist_Attachments'>;
private _height: SavingObservable<string>;
private _height: KoSaveableObservable<string>;
constructor(field: any) {
constructor(field: ViewFieldRec) {
super(field);
// TODO: the Attachments table currently treated as metadata, and loaded on open,
// but should probably be loaded on demand as it contains user data, which may be large.
this._attachmentsTable = this._getDocData().getMetaTable('_grist_Attachments');
this._height = this.options.prop('height') as SavingObservable<string>;
this._height = this.options.prop('height');
this.autoDispose(this._height.subscribe(() => {
this.field.viewSection().events.trigger('rowHeightChange');
}));
}
public buildDom(_row: any): Element {
public buildDom(row: DataRowModel) {
// NOTE: A cellValue of the correct type includes the list encoding designator 'L' as the
// first element.
const cellValue: SavingObservable<number[]> = _row[this.field.colId()];
const cellValue = row.cells[this.field.colId()];
const values = Computed.create(null, fromKo(cellValue), (use, _cellValue) =>
Array.isArray(_cellValue) ? _cellValue.slice(1) : []);
Array.isArray(_cellValue) ? _cellValue.slice(1) as number[] : []);
const colId = this.field.colId();
const tableId = this.field.column().table().tableId();
return attachmentWidget(
return cssAttachmentWidget(
dom.autoDispose(values),
dom.cls('field_clip'),
dragOverClass('attachment_drag_over'),
attachmentIcon(
dom.cls('attachment_hover_icon', (use) => use(values).length > 0),
dom.on('click', () => this._selectAndSave(cellValue))
cssAttachmentIcon(
cssAttachmentIcon.cls('-hover', (use) => use(values).length > 0),
dom.on('click', () => this._selectAndSave(row, cellValue)),
testId('attachment-icon'),
),
dom.maybe(_row.id, rowId => {
dom.maybe<number>(row.id, rowId => {
return dom.forEach(values, (value: number) =>
isNaN(value) ? null : this._buildAttachment(value, values, {
rowId, colId, tableId,
}));
}),
dom.on('drop', ev => this._uploadAndSave(cellValue, ev.dataTransfer!.files))
dom.on('drop', ev => this._uploadAndSave(row, cellValue, ev.dataTransfer!.files)),
testId('attachment-widget'),
);
}
@ -123,15 +81,17 @@ export class AttachmentsWidget extends NewAbstractWidget {
max: '96',
value: '36'
},
testId('thumbnail-size'),
testId('pw-thumbnail-size'),
// When multiple columns are selected, we can only edit height when all
// columns support it.
dom.prop('disabled', use => use(options.disabled('height'))),
);
// Save the height on change event (when the user releases the drag button)
onElem(inputRange, 'change', (ev: any) => { height.setAndSave(ev.target.value).catch(reportError); });
onElem(inputRange, 'change', (ev: Event) => {
height.setAndSave(inputRange.value).catch(reportError);
});
return cssRow(
sizeLabel('Size'),
cssSizeLabel('Size'),
inputRange
);
}
@ -144,7 +104,7 @@ export class AttachmentsWidget extends NewAbstractWidget {
const hasPreview = Boolean(height);
const ratio = hasPreview ? (width / height) : 1;
return attachmentPreview({title: filename}, // Add a filename tooltip to the previews.
return cssAttachmentPreview({title: filename}, // Add a filename tooltip to the previews.
dom.style('height', (use) => `${use(this._height)}px`),
dom.style('width', (use) => `${parseInt(use(this._height), 10) * ratio}px`),
// TODO: Update to legitimately determine whether a file preview exists.
@ -155,7 +115,7 @@ export class AttachmentsWidget extends NewAbstractWidget {
// pass in a 1-based index. Hitting a key opens the cell, and this approach allows an
// accidental feature of opening e.g. second attachment by hitting "2".
dom.on('dblclick', () => commands.allCommands.input.run(String(allValues.get().indexOf(value) + 1))),
testId('thumbnail'),
testId('pw-thumbnail'),
);
}
@ -170,27 +130,36 @@ export class AttachmentsWidget extends NewAbstractWidget {
});
}
private async _selectAndSave(value: SavingObservable<number[]>): Promise<void> {
private async _selectAndSave(row: DataRowModel, value: KoSaveableObservable<CellValue>): Promise<void> {
const uploadResult = await selectFiles({docWorkerUrl: this._getDocComm().docWorkerUrl,
multiple: true, sizeLimit: 'attachment'});
return this._save(value, uploadResult);
return this._save(row, value, uploadResult);
}
private async _uploadAndSave(value: SavingObservable<number[]>, files: FileList): Promise<void> {
private async _uploadAndSave(row: DataRowModel, value: KoSaveableObservable<CellValue>,
files: FileList): Promise<void> {
const uploadResult = await uploadFiles(Array.from(files),
{docWorkerUrl: this._getDocComm().docWorkerUrl,
sizeLimit: 'attachment'});
return this._save(value, uploadResult);
return this._save(row, value, uploadResult);
}
private async _save(value: SavingObservable<number[]>, uploadResult: UploadResult|null): Promise<void> {
private async _save(row: DataRowModel, value: KoSaveableObservable<CellValue>,
uploadResult: UploadResult|null
): Promise<void> {
if (!uploadResult) { return; }
const rowIds = await this._getDocComm().addAttachments(uploadResult.uploadId);
// Values should be saved with a leading "L" to fit Grist's list value encoding.
const formatted: any[] = value() ? value() : ["L"];
value.setAndSave(formatted.concat(rowIds));
// Trigger a row height change in case the added attachment wraps to the next line.
this.field.viewSection().events.trigger('rowHeightChange');
const formatted: CellValue = value() ? value() : [GristObjCode.List];
const newValue = (formatted as number[]).concat(rowIds) as CellValue;
// Move the cursor here (note that this may involve switching active section when dragging
// into a cell of an inactive section). Then send the 'input' command; it is normally used for
// key presses to open an editor; here the "typed text" is the new value. It is handled by
// AttachmentsEditor.skipEditor(), and makes the edit apply to editRow, which handles setting
// default values based on widget linking.
commands.allCommands.setCursor.run(row, this.field);
commands.allCommands.input.run(newValue);
}
}
@ -206,6 +175,63 @@ export function renderFileType(fileName: string, fileIdent: string, height?: ko.
);
}
const cssAttachmentWidget = styled('div', `
display: flex;
flex-wrap: wrap;
white-space: pre-wrap;
&.attachment_drag_over {
outline: 2px dashed #ff9a00;
outline-offset: -2px;
}
`);
const cssAttachmentIcon = styled('div.glyphicon.glyphicon-paperclip', `
position: absolute;
top: 2px;
left: 2px;
padding: 2px;
background-color: #D0D0D0;
color: white;
border-radius: 2px;
border: none;
cursor: pointer;
box-shadow: 0 0 0 1px white;
z-index: 1;
&:hover {
background-color: #3290BF;
}
&-hover {
display: none;
}
.${cssAttachmentWidget.className}:hover &-hover {
display: inline;
}
`);
const cssAttachmentPreview = styled('div', `
color: black;
background-color: white;
border: 1px solid #bbb;
margin: 0 2px 2px 0;
position: relative;
display: flex;
align-items: center;
justify-content: center;
z-index: 0;
&:hover {
border-color: ${colors.lightGreen};
}
`);
const cssSizeLabel = styled('div', `
color: ${colors.slate};
margin-right: 9px;
`);
const cssFileType = styled('div', `
height: 100%;
width: 100%;

@ -0,0 +1,80 @@
import {assert} from 'mocha-webdriver';
import * as gu from 'test/nbrowser/gristUtils';
import {Session} from 'test/nbrowser/gristUtils';
import {setupTestSuite} from 'test/nbrowser/testUtils';
describe('AttachmentsLinking', function() {
this.timeout(20000);
const cleanup = setupTestSuite({team: true});
let session: Session;
let docId: string;
before(async function() {
session = await gu.session().login();
docId = await session.tempNewDoc(cleanup, 'AttachmentColumns', {load: false});
// Set up a table Src, and table Items which links to Src and has an Attachments column.
const api = session.createHomeApi();
await api.applyUserActions(docId, [
['AddTable', 'Src', [{id: 'A', type: 'Text'}]],
['BulkAddRecord', 'Src', [null, null, null], {A: ['a', 'b', 'c']}],
['AddTable', 'Items', [
{id: 'A', type: 'Ref:Src'},
{id: 'Att', type: 'Attachments'},
]],
['BulkAddRecord', 'Items', [null, null, null], {A: [1, 1, 3]}],
]);
await session.loadDoc(`/doc/${docId}`);
// Set up a page with linked widgets.
await gu.addNewPage('Table', 'Src');
await gu.addNewSection('Table', 'Items', {selectBy: /Src/i});
});
it('should fill in values determined by linking when uploading to the add row', async function() {
// TODO Another good test case would be that dragging a file into a cell works, especially
// when that cell isn't in the selected widget. But this doesn't seem supported by webdriver.
// Selecting a cell in Src should show only linked values in Items.
await gu.getCell({section: 'Src', col: 'A', rowNum: 1}).click();
assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Att'], rowNums: [1, 2, 3]}), [
'Src[1]', '',
'Src[1]', '',
'', '',
]);
// Upload into an Attachments cell in the "Add Row" of Items.
assert.equal(await gu.getCell({section: 'Items', col: 0, rowNum: 4}).isPresent(), false);
let cell = await gu.getCell({section: 'Items', col: 'Att', rowNum: 3});
await gu.fileDialogUpload('uploads/file1.mov', () => cell.find('.test-attachment-icon').click());
await gu.waitToPass(async () =>
assert.lengthOf(await gu.getCell({section: 'Items', col: 'Att', rowNum: 3}).findAll('.test-pw-thumbnail'), 1));
assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Att'], rowNums: [1, 2, 3, 4]}), [
'Src[1]', '',
'Src[1]', '',
'Src[1]', 'MOV',
'', '',
]);
// Switch to another Src row; should see no attachments.
await gu.getCell({section: 'Src', col: 'A', rowNum: 2}).click();
assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Att'], rowNums: [1]}), [
'', '',
]);
cell = await gu.getCell({section: 'Items', col: 'Att', rowNum: 1});
await gu.fileDialogUpload('uploads/htmlfile.html,uploads/file1.mov',
() => cell.find('.test-attachment-icon').click());
await gu.waitToPass(async () =>
assert.lengthOf(await gu.getCell({section: 'Items', col: 'Att', rowNum: 1}).findAll('.test-pw-thumbnail'), 2));
assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Att'], rowNums: [1, 2]}), [
'Src[2]', 'HTML\nMOV',
'', '',
]);
});
});
Loading…
Cancel
Save