Skip to content

Commit

Permalink
fix: vscroll event mem leak causing unexpected behavior after filtering
Browse files Browse the repository at this point in the history 
- there was an event leak for the Virtual-Scroll which had the side effect of duplicating the scroll event everytime we were filtering, this indirectly caused weird behavior with the scroll and also items being skipped. I found the leak via the browser by investigating the events attached to the ul list element
  • Loading branch information
@ghiscoding
ghiscoding committed Mar 23, 2024
1 parent 559b161 commit 1409c0f
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 48 deletions.
4 changes: 2 additions & 2 deletions packages/demo/src/options/options36.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ export default class Example {
mount() {
const data1 = [];
const data2 = [];
for (let i = 0; i < 25; i++) {
for (let i = 1; i <= 25; i++) {
data1.push({ text: `Title ${i}`, value: i });
}
for (let i = 0; i < 2000; i++) {
for (let i = 1; i <= 2000; i++) {
data2.push({ text: `<i class="fa fa-star"></i> Task ${i}`, value: i });
}

Expand Down
35 changes: 18 additions & 17 deletions packages/multiple-select-vanilla/src/MultipleSelectInstance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,9 +460,7 @@ export class MultipleSelectInstance {
offset = -1;
}

if (this.options.virtualScroll && rows.length > Constants.BLOCK_ROWS * Constants.CLUSTER_BLOCKS) {
this.virtualScroll?.destroy();

if (rows.length > Constants.BLOCK_ROWS * Constants.CLUSTER_BLOCKS) {
const dropVisible = this.dropElm.style.display !== 'none';
if (!dropVisible) {
this.dropElm.style.left = '-10000';
Expand All @@ -480,8 +478,9 @@ export class MultipleSelectInstance {
this.updateDataStart = 0;
this._currentHighlightIndex = 0;
}
if (this.updateDataEnd > this.getDataLength()) {
this.updateDataEnd = this.getDataLength();
const dataLn = this.getDataLength();
if (this.updateDataEnd > dataLn) {
this.updateDataEnd = dataLn;
}

if (this.ulElm) {
Expand All @@ -495,18 +494,21 @@ export class MultipleSelectInstance {
};

if (this.ulElm) {
this.virtualScroll = new VirtualScroll({
rows,
scrollEl: this.ulElm,
contentEl: this.ulElm,
sanitizer: this.options.sanitizer,
callback: () => {
updateDataOffset();
this.events();
},
});
if (!this.virtualScroll) {
this.virtualScroll = new VirtualScroll({
rows,
scrollEl: this.ulElm,
contentEl: this.ulElm,
sanitizer: this.options.sanitizer,
callback: () => {
updateDataOffset();
this.events();
},
});
} else {
this.virtualScroll.reset(rows);
}
}

updateDataOffset();

if (!dropVisible) {
Expand All @@ -521,7 +523,6 @@ export class MultipleSelectInstance {
}
this.updateDataStart = 0;
this.updateDataEnd = this.updateData.length;
this.virtualScroll = null;
}

this.events();
Expand Down
39 changes: 23 additions & 16 deletions packages/multiple-select-vanilla/src/services/virtual-scroll.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,19 @@ import type { HtmlStruct, VirtualCache, VirtualScrollOption } from '../interface
import { convertItemRowToHtml, emptyElement } from '../utils';

export class VirtualScroll {
cache: VirtualCache;
clusterRows?: number;
protected clusterRows?: number;
protected cache: VirtualCache;
protected scrollEl: HTMLElement;
protected blockHeight?: number;
protected clusterHeight?: number;
protected contentEl: HTMLElement;
protected parentEl: HTMLElement | null;
protected itemHeight?: number;
protected lastCluster: number;
protected scrollTop: number;
dataStart!: number;
dataEnd!: number;
rows: HtmlStruct[];
scrollEl: HTMLElement;
blockHeight?: number;
clusterHeight?: number;
contentEl: HTMLElement;
parentEl: HTMLElement | null;
itemHeight?: number;
lastCluster: number;
scrollTop: number;
destroy: () => void;
callback: () => void;
sanitizer?: (dirtyHtml: string) => string;
Expand Down Expand Up @@ -49,7 +49,14 @@ export class VirtualScroll {
};
}

initDOM(rows: HtmlStruct[]) {
reset(rows: HtmlStruct[]) {
this.lastCluster = 0;
this.cache = {} as any;
emptyElement(this.contentEl);
this.initDOM(rows);
}

protected initDOM(rows: HtmlStruct[]) {
if (typeof this.clusterHeight === 'undefined') {
this.cache.scrollTop = this.scrollEl.scrollTop;
const firstRowElm = convertItemRowToHtml(rows[0]);
Expand Down Expand Up @@ -82,7 +89,7 @@ export class VirtualScroll {
}
}

getRowsHeight() {
protected getRowsHeight() {
if (typeof this.itemHeight === 'undefined') {
// make sure parent is not hidden before reading item list height
const prevParentDisplay = this.parentEl?.style.display || '';
Expand All @@ -101,7 +108,7 @@ export class VirtualScroll {
this.clusterHeight = this.blockHeight * Constants.CLUSTER_BLOCKS;
}

getNum() {
protected getNum() {
this.scrollTop = this.scrollEl.scrollTop;
const blockSize = (this.clusterHeight || 0) - (this.blockHeight || 0);
if (blockSize) {
Expand All @@ -110,7 +117,7 @@ export class VirtualScroll {
return 0;
}

initData(rows: HtmlStruct[], num: number) {
protected initData(rows: HtmlStruct[], num: number) {
if (rows.length < Constants.BLOCK_ROWS) {
return {
topOffset: 0,
Expand Down Expand Up @@ -143,13 +150,13 @@ export class VirtualScroll {
};
}

checkChanges<T extends keyof VirtualCache>(type: T, value: VirtualCache[T]) {
protected checkChanges<T extends keyof VirtualCache>(type: T, value: VirtualCache[T]) {
const changed = value !== this.cache[type];
this.cache[type] = value;
return changed;
}

getExtra(className: string, height: number) {
protected getExtra(className: string, height: number) {
const tag = document.createElement('li');
tag.className = `virtual-scroll-${className}`;
if (height) {
Expand Down
26 changes: 13 additions & 13 deletions playwright/e2e/options36.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,49 +9,49 @@ test.describe('Options 36 - Infinite Scroll', () => {

const ulElm1 = await page.locator('[data-test="select1"] .ms-drop ul');
const liElms1 = await page.locator('[data-test="select1"] .ms-drop ul li');
await expect(liElms1.nth(0)).toContainText('Title 0');
await expect(liElms1.nth(0)).toContainText('Title 1');
await liElms1.nth(0).click();
await expect(page.locator('[data-test=select1].ms-parent .ms-choice span')).toHaveText('Title 0');
await expect(page.locator('[data-test=select1].ms-parent .ms-choice span')).toHaveText('Title 1');

// scroll near the end of the list
await page.locator('[data-test="select1"].ms-parent').click();
await ulElm1.evaluate(e => (e.scrollTop = e.scrollHeight - 10));
await page.locator('[data-test="select1"] .ms-drop label').filter({ hasText: 'Title 24' }).click();
await page.locator('[data-test="select1"] .ms-drop label').filter({ hasText: 'Title 25' }).click();

// scroll completely to the end of the list & expect scrolling back to top
await page.locator('[data-test="select1"].ms-parent').click();
await ulElm1.evaluate(e => (e.scrollTop = e.scrollHeight));
const firstTitleLoc = await page.locator('div[data-test=select1] .ms-drop li:nth-of-type(1)');
await expect(firstTitleLoc).toContainText('Title 0');
await expect(firstTitleLoc).toContainText('Title 1');
await expect(firstTitleLoc).toHaveClass('hide-radio highlighted');
await page.keyboard.press('Enter');

// -- 2nd Select
await page.locator('[data-test=select2].ms-parent').click();
const ulElm2 = await page.locator('[data-test="select2"] .ms-drop ul');
const liElms2 = await page.locator('[data-test="select2"] .ms-drop ul li');
await expect(await liElms2.nth(4).locator('span').innerHTML()).toBe('<i class="fa fa-star"></i> Task 4');
await expect(await liElms2.nth(4).locator('span').innerHTML()).toBe('<i class="fa fa-star"></i> Task 5');
await liElms2.nth(4).click();
await expect(await liElms2.nth(5).locator('span').innerHTML()).toBe('<i class="fa fa-star"></i> Task 5');
await expect(await liElms2.nth(5).locator('span').innerHTML()).toBe('<i class="fa fa-star"></i> Task 6');
await liElms2.nth(5).click();
await page.getByRole('button', { name: '4, 5' }).click();
await page.getByRole('button', { name: '5, 6' }).click();

// scroll to the middle and click 1003
await page.locator('[data-test="select2"].ms-parent').click();
await ulElm2.evaluate(e => (e.scrollTop = e.scrollHeight / 2));
await page.locator('[data-test="select2"] .ms-drop label').filter({ hasText: '1003' }).click();
await page.getByRole('button', { name: '4, 5, 1003' });
await page.getByRole('button', { name: '5, 6, 1003' });

// scroll to near the end and select last 2 labels
await ulElm2.evaluate(e => (e.scrollTop = e.scrollHeight - 300));
await expect(await page.locator('[data-test="select2"] .ms-drop li[data-key=option_1995] label span').innerHTML()).toBe(
'<i class="fa fa-star"></i> Task 1995',
'<i class="fa fa-star"></i> Task 1996',
);
await expect(await page.locator('[data-test="select2"] .ms-drop li[data-key=option_1996] label span').innerHTML()).toBe(
'<i class="fa fa-star"></i> Task 1996',
'<i class="fa fa-star"></i> Task 1997',
);
await page.locator('[data-test="select2"] .ms-drop label').filter({ hasText: '1995' }).click();
await page.locator('[data-test="select2"] .ms-drop label').filter({ hasText: '1996' }).click();
await page.locator('[data-test="select2"] .ms-drop label').filter({ hasText: '1997' }).click();
await page.getByRole('button', { name: '5 of 2000 selected' });

// pressing arrow down until we reach the end will scroll back to top of the list
Expand All @@ -60,10 +60,10 @@ test.describe('Options 36 - Infinite Scroll', () => {
page.keyboard.press('ArrowDown');
await expect(await page.locator('[data-test="select2"] .ms-drop li[data-key=option_1999]')).toHaveClass('highlighted');

page.keyboard.press('ArrowDown'); // Task 0 (scrolled back to top)
page.keyboard.press('ArrowDown'); // Task 1 (scrolled back to top)

const firstTaskLoc = await page.locator('div[data-test=select2] .ms-drop li:nth-of-type(1)');
await expect(firstTaskLoc).toContainText('Task 0');
await expect(firstTaskLoc).toContainText('Task 1');
// await expect(await page.locator('[data-test="select2"] .ms-drop li[data-key=option_0]')).toHaveClass('highlighted');
});
});

0 comments on commit 1409c0f

Please sign in to comment.