Skip to content

Commit

Permalink
fix: disable row drag and drop while loading (#2957) (#2198)
Browse files Browse the repository at this point in the history 
* fix: disable row drag and drop while loading (#2957)

* chore: fix lint issue

* test: create new instance for each test

* fix: workaround an IE11 bug

* test: remove a skipped flaky test

* Revert "test: create new instance for each test"

This reverts commit 6039fb6.

* fix drag and drop test setup

Co-authored-by: Sascha Ißbrücker <sascha.issbruecker@googlemail.com>
  • Loading branch information
@tomivirkki @sissbruecker
tomivirkki and sissbruecker authored Dec 20, 2021
1 parent bec0993 commit cd7d5bf
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 82 deletions.
33 changes: 12 additions & 21 deletions src/vaadin-grid-drag-and-drop-mixin.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@

static get observers() {
return [
'_dragDropAccessChanged(rowsDraggable, dropMode, dragFilter, dropFilter)'
'_dragDropAccessChanged(rowsDraggable, dropMode, dragFilter, dropFilter, loading)'
];
}

Expand Down Expand Up @@ -305,25 +305,15 @@
__getViewportRows() {
// Workaround an IE11 bug where the `getBoundingClientRect` for header/footer
// might return incorrect values
if (this._ie) {
this.$.header.style.outline = '0px solid transparent';
const scrollerRect = this.$.scroller.getBoundingClientRect();
const headerBottom = Math.max(this.$.header.getBoundingClientRect().bottom, scrollerRect.top);
const footerTop = Math.min(this.$.footer.getBoundingClientRect().top, scrollerRect.bottom);
this.$.header.style.outline = '0px solid transparent';
const scrollerRect = this.$.scroller.getBoundingClientRect();
const headerBottom = Math.max(this.$.header.getBoundingClientRect().bottom, scrollerRect.top);
const footerTop = Math.min(this.$.footer.getBoundingClientRect().top, scrollerRect.bottom);

return Array.from(this.$.items.children).filter((row) => {
const rowRect = row.getBoundingClientRect();
return rowRect.bottom > headerBottom && rowRect.top < footerTop;
});
}

const headerBottom = this.$.header.getBoundingClientRect().bottom;
const footerTop = this.$.footer.getBoundingClientRect().top;
return Array.from(this.$.items.children)
.filter(row => {
const rowRect = row.getBoundingClientRect();
return rowRect.bottom > headerBottom && rowRect.top < footerTop;
});
return Array.from(this.$.items.children).filter((row) => {
const rowRect = row.getBoundingClientRect();
return rowRect.bottom > headerBottom && rowRect.top < footerTop;
});
}

/** @protected */
Expand Down Expand Up @@ -402,8 +392,9 @@
* @protected
*/
_filterDragAndDrop(row, model) {
const dragDisabled = !this.rowsDraggable || (this.dragFilter && !this.dragFilter(model));
const dropDisabled = !this.dropMode || (this.dropFilter && !this.dropFilter(model));
const loading = this.loading || row.hasAttribute('loading');
const dragDisabled = !this.rowsDraggable || loading || (this.dragFilter && !this.dragFilter(model));
const dropDisabled = !this.dropMode || loading || (this.dropFilter && !this.dropFilter(model));

const draggableElements = window.ShadyDOM
? [row]
Expand Down
17 changes: 0 additions & 17 deletions test/data-provider.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -671,23 +671,6 @@
}, loadDebounceTime);
});

// FIXME: flaky
it.skip('should call dataprovider multiple times to load all items', done => {
container.dataProvider.reset();
grid.style.fontSize = '12px';
grid.pageSize = 10;
flushGrid(grid);

setTimeout(() => {
// assuming grid has about 30 items
expect(container.dataProvider.callCount).to.be.above(2);
for (var i = 0; i < container.dataProvider.callCount; i++) {
expect(container.dataProvider.getCall(i).args[0].page).to.eql(i);
}
done();
}, loadDebounceTime);
});

it('should always load visible items', done => {
grid.pageSize = 10;

Expand Down
146 changes: 102 additions & 44 deletions test/drag-and-drop.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@
describe('drag and drop', () => {
let grid;

const getTestItems = () => [
{first: 'foo', last: 'bar'},
{first: 'baz', last: 'qux'}
];

const getDraggable = (grid, rowIndex = 0) => {
const row = Array.from(grid.$.items.children).filter(row => row.index === rowIndex)[0];
const cellContent = row.querySelector('slot').assignedNodes()[0];
Expand Down Expand Up @@ -119,43 +124,18 @@
return event;
};

before(done => {
grid = fixture('default');
grid.hidden = true;
requestAnimationFrame(() => {
// Wait for the appear animation to finish
listenOnce(grid, 'animationend', () => {
flush(() => done());
});
requestAnimationFrame(() => grid.hidden = false);
});
});

beforeEach(done => {
grid = fixture('default');
grid.items = getTestItems();
dragData = {};
if (!grid.parentElement) {
document.body.appendChild(grid);
}
grid.items = [
{first: 'foo', last: 'bar'},
{first: 'baz', last: 'qux'}
];
flushGrid(grid);
if (grid._safari) {
setTimeout(() => done());
} else {
done();
}

});

afterEach(() => {
fireDragEnd(grid.$.table);
fireDrop(grid.$.table);
grid.rowsDraggable = false;
grid.dropMode = null;
grid.style.height = '';
grid.selectedItems = [];
requestAnimationFrame(() => {
if (grid._safari) {
setTimeout(() => done());
} else {
done();
}
});
});

it('should not be draggable by default', () => {
Expand Down Expand Up @@ -183,14 +163,11 @@
let dragStartSpy;
let dropSpy;

before(() => {
beforeEach(() => {
dragStartSpy = sinon.spy();
dropSpy = sinon.spy();
grid.addEventListener('grid-dragstart', dragStartSpy);
grid.addEventListener('grid-drop', dropSpy);
});

beforeEach(() => {
grid.rowsDraggable = true;
grid.dropMode = 'on-top';
dragStartSpy.reset();
Expand Down Expand Up @@ -224,8 +201,7 @@
expect(spy.called).to.be.true;
});

// TODO: flaky in IE, timeout might not be appropriate
it.skip('should only use visible items for the row count state attribute', done => {
it('should only use visible items for the row count state attribute', done => {
grid.style.height = '80px';
flushGrid(grid);
grid.notifyResize();
Expand Down Expand Up @@ -759,6 +735,44 @@
expect(row.getAttribute('dragstart')).to.equal('');
});

describe('filtering row drag - lazy loading', () => {
let finishLoadingItems;

beforeEach(() => {
grid.dataProvider = (_params, callback) => {
finishLoadingItems = (items) => callback(items || getTestItems());
};
});

it('should disable row drag while loading items', () => {
expect(getDraggable(grid, 1)).not.to.be.ok;
expect(grid.$.items.children[1].hasAttribute('drag-disabled')).to.be.true;
});

it('should enable row drag once loading has finished', () => {
finishLoadingItems();
expect(getDraggable(grid, 1)).to.be.ok;
expect(grid.$.items.children[1].hasAttribute('drag-disabled')).to.be.false;
});

it('should not run drag filter while loading items', () => {
grid.dragFilter = sinon.spy();
expect(grid.dragFilter.called).to.be.false;
});

it('should disable row drag for rows without an item', () => {
finishLoadingItems([getTestItems()[0], undefined]);
expect(getDraggable(grid, 1)).not.to.be.ok;
expect(grid.$.items.children[1].hasAttribute('drag-disabled')).to.be.true;
});

it('should enable row drag once items are available', () => {
finishLoadingItems([getTestItems()[0], undefined]);
finishLoadingItems();
expect(getDraggable(grid, 1)).to.be.ok;
expect(grid.$.items.children[1].hasAttribute('drag-disabled')).to.be.false;
});
});
});

describe('filtering row drop', () => {
Expand Down Expand Up @@ -817,6 +831,52 @@
expect(e.defaultPrevented).to.be.false;
});

describe('filtering row drop - lazy loading', () => {
let finishLoadingItems;

beforeEach(() => {
grid.dataProvider = (_params, callback) => {
finishLoadingItems = (items) => callback(items || getTestItems());
};
});

it('should disable drop on row while loading items', () => {
const row = grid.$.items.children[1];
fireDragOver(row, 'above');
expect(row.hasAttribute('dragover')).to.be.false;
expect(row.hasAttribute('drop-disabled')).to.be.true;
});

it('should enable drop on row once loading has finished', () => {
finishLoadingItems();
const row = grid.$.items.children[1];
fireDragOver(row, 'above');
expect(row.hasAttribute('dragover')).to.be.true;
expect(row.hasAttribute('drop-disabled')).to.be.false;
});

it('should not run drop filter while loading items', () => {
grid.dropFilter = sinon.spy();
expect(grid.dropFilter.called).to.be.false;
});

it('should disable drop on row for rows without an item', () => {
finishLoadingItems([getTestItems()[0], undefined]);
const row = grid.$.items.children[1];
fireDragOver(row, 'above');
expect(row.hasAttribute('dragover')).to.be.false;
expect(row.hasAttribute('drop-disabled')).to.be.true;
});

it('should enable drop on row once items are available', () => {
finishLoadingItems([getTestItems()[0], undefined]);
finishLoadingItems();
const row = grid.$.items.children[1];
fireDragOver(row, 'above');
expect(row.hasAttribute('dragover')).to.be.true;
expect(row.hasAttribute('drop-disabled')).to.be.false;
});
});
});

describe('auto scroll', () => {
Expand All @@ -842,16 +902,14 @@
expect(grid.$.table.scrollTop).to.be.within(scrollTop + 100, scrollTop + 200);
});

// TODO: flaky in Edge, magic numbers are not reliable,
it.skip('should auto scroll up', () => {
it('should auto scroll up', () => {
grid._scrollToIndex(50);
const scrollTop = grid.$.table.scrollTop;
fireDragOver(grid.__getViewportRows()[0], 'below');
expect(grid.$.table.scrollTop).to.be.within(scrollTop - 100, scrollTop - 20);
});

// TODO: flaky in Edge, magic numbers are not reliable,
it.skip('should auto scroll up fast', () => {
it('should auto scroll up fast', () => {
grid._scrollToIndex(50);
const scrollTop = grid.$.table.scrollTop;
fireDragOver(grid.__getViewportRows()[0], 'above');
Expand Down

0 comments on commit cd7d5bf

Please # to comment.