Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not update element with the same index more than once #2910

Merged
merged 1 commit into from
Oct 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/combo-box/src/vaadin-combo-box-scroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ export class ComboBoxScroller extends PolymerElement {
* calling `scrollIntoView` does not have any effect.
*/
opened: {
type: Boolean
type: Boolean,
observer: '__openedChanged'
},

/**
Expand Down Expand Up @@ -134,6 +135,12 @@ export class ComboBoxScroller extends PolymerElement {
this.__boundOnItemClick = this.__onItemClick.bind(this);
}

__openedChanged(opened) {
if (this.__virtualizer && opened) {
this.__virtualizer.update();
}
}

/** @protected */
ready() {
super.ready();
Expand Down Expand Up @@ -226,6 +233,7 @@ export class ComboBoxScroller extends PolymerElement {
this.__virtualizer.flush();
// Ensure the total count of items is properly announced.
this.setAttribute('aria-setsize', items.length);
this.__virtualizer.update();
}
}

Expand All @@ -239,6 +247,7 @@ export class ComboBoxScroller extends PolymerElement {
/** @private */
__focusedIndexChanged(index) {
if (this.__virtualizer && index >= 0) {
this.__virtualizer.update();
this.scrollIntoView(index);
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/combo-box/test/lazy-loading.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -734,11 +734,13 @@ describe('lazy loading', () => {
expect(selectedRenderedItemElements[0].item).to.eql({ id: 0, value: 'value 0', label: 'label 0' });
});

it('should select value matching selectedItem when items are loaded', () => {
it('should select value matching selectedItem when items are loaded', async () => {
comboBox.opened = true;
comboBox.selectedItem = { id: 0, value: 'value 0', label: 'label 0' };
expect(comboBox.value).to.equal('value 0');
flushComboBox(comboBox);
// Wait for the timeout in __loadingChanged to finish
await aTimeout(0);
const selectedRenderedItemElements = getAllItems(comboBox).filter((itemEl) => itemEl.selected);
// doesn't work when run on SauceLabs, work locally
// expect(selectedRenderedItemElements).to.have.lengthOf(1);
Expand Down
7 changes: 4 additions & 3 deletions packages/component-base/src/virtualizer-iron-list-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,19 +118,20 @@ export class IronListAdapter {
update(startIndex = 0, endIndex = this.size - 1) {
this.__getVisibleElements().forEach((el) => {
if (el.__virtualIndex >= startIndex && el.__virtualIndex <= endIndex) {
this.__updateElement(el, el.__virtualIndex);
this.__updateElement(el, el.__virtualIndex, true);
}
});
}

__updateElement(el, index) {
__updateElement(el, index, forceSameIndexUpdates) {
// Clean up temporary min height
if (el.style.minHeight) {
el.style.minHeight = '';
}

if (!this.__preventElementUpdates) {
if (!this.__preventElementUpdates && (el.__lastUpdatedIndex !== index || forceSameIndexUpdates)) {
this.updateElement(el, index);
el.__lastUpdatedIndex = index;
}

if (el.offsetHeight === 0) {
Expand Down
18 changes: 13 additions & 5 deletions packages/component-base/test/virtualizer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,22 +172,19 @@ describe('virtualizer', () => {
expect(item.getBoundingClientRect().top).to.equal(scrollTarget.getBoundingClientRect().top - 10);
});

it('should not request a different set of items on size increase', () => {
it('should not request item updates on size increase', () => {
const updateElement = sinon.spy((el, index) => (el.textContent = `item-${index}`));
init({ size: 100, updateElement });

// Scroll halfway down the list
updateElement.resetHistory();
virtualizer.scrollToIndex(50);
const updatedIndexes = updateElement.getCalls().map((call) => call.args[1]);

// Increase the size so it shouldn't affect the current viewport items
updateElement.resetHistory();
virtualizer.size = 200;
const postResizeUpdatedIndexes = updateElement.getCalls().map((call) => call.args[1]);

expect(postResizeUpdatedIndexes).to.eql(updatedIndexes);
expect(postResizeUpdatedIndexes).not.to.include(0);
expect(updateElement.called).to.be.false;
});

it('should request a different set of items on size decrease', () => {
Expand All @@ -208,6 +205,17 @@ describe('virtualizer', () => {
expect(postResizeUpdatedIndexes).to.include(0);
});

it('should request an index only once', async () => {
const updateElement = sinon.spy((el, index) => (el.textContent = `item-${index}`));
init({ size: 100, updateElement });

// Wait for a possible resize observer flush
await aTimeout(100);

const firstIndexUpdatesCount = updateElement.getCalls().filter((call) => call.args[1] === 0).length;
expect(firstIndexUpdatesCount).to.equal(1);
});

it('should have physical items once visible', async () => {
init({ size: 0 });
// Wait for possibly active resize observers to flush
Expand Down
3 changes: 2 additions & 1 deletion packages/grid/src/vaadin-grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,6 @@ class Grid extends ElementMixin(

this._a11yUpdateRowLevel(row, model.level);
this._a11yUpdateRowSelected(row, model.selected);
this._a11yUpdateRowExpanded(row, model.expanded);
this._a11yUpdateRowDetailsOpened(row, model.detailsOpened);

row.toggleAttribute('expanded', model.expanded);
Expand All @@ -902,6 +901,8 @@ class Grid extends ElementMixin(
});

this._updateDetailsCellHeight(row);

this._a11yUpdateRowExpanded(row, model.expanded);
}

/** @private */
Expand Down
7 changes: 2 additions & 5 deletions packages/virtual-list/src/vaadin-virtual-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,8 @@ class VirtualList extends ElementMixin(ThemableMixin(PolymerElement)) {
const hasRenderedItems = this.childElementCount > 0;

if ((renderer || hasRenderedItems) && virtualizer) {
if (items.length === virtualizer.size) {
virtualizer.update();
} else {
virtualizer.size = items.length;
}
virtualizer.size = items.length;
virtualizer.update();
}
}

Expand Down