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

[BUG] Fix cases where manual selection includes unrendered or nonexistent rows #748

Merged
merged 6 commits into from
Jul 31, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
75 changes: 73 additions & 2 deletions addon/-private/collapse-tree.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import EmberObject, { get, set } from '@ember/object';
import EmberArray, { A as emberA, isArray } from '@ember/array';
import { assert } from '@ember/debug';
import { assert, warn } from '@ember/debug';

import { computed } from '@ember/object';
import { addObserver } from '@ember/object/observers';
Expand Down Expand Up @@ -227,7 +227,7 @@ export const TableRowMeta = EmberObject.extend({
selection.add(rowValue);
}

let rowMetas = Array.from(selection).map(r => rowMetaCache.get(r));
let rowMetas = mapSelectionToMeta(this.get('_tree'), selection, rowMetaCache);

if (selectingChildrenSelectsParent) {
let groupingCounts = new Map();
Expand Down Expand Up @@ -309,6 +309,77 @@ function setupRowMeta(tree, row, parentRow, node) {
}
}

/**
* Traverses the tree to set up row meta for every row in the tree.
* Usually row metas are lazily created as needed, but it's possible to end up in a state
* where a table's `selection` contains rows that do not have a rowMeta (for instance, if they
* have not yet been rendered due to occlusion rendering). In this state, there may not be a
* rowMeta for every row in the `selection`, so we need to explicitly set them all up at that
* time.
* This has adverse performance impact, so we lazily call this function only when we find that
* the `selection` has some rows with no corresponding rowMeta.
*
* @param {CollapseTree} tree The collapse tree for this section (body|footer) of the table
* @param {object} parentRow The parent row. Only present when called recursively
*/
function setupAllRowMeta(tree, rows, parentRow = null) {
for (let row of rows) {
setupRowMeta(tree, row, parentRow);
if (row.children && row.children.length) {
setupAllRowMeta(tree, row.children, row);
}
}
}

/**
* Maps the selection to an array of rowMetas.
*
* If any row in the selection does not have a rowMeta, calls `setupAllRowMeta`
* to materialize all rowMetas, then tries again to get the rowMeta for that
* row. This happens in rare cases where, due to occlusion rendering, a row may
* be part of the selection but not in view (and thus have no rowMeta; the
* rowMeta is lazily created when the row is rendered).
*
* If after calling `setupAllRowMeta` the row still does not have a
* corresponding rowMeta, it is likely an invalid selection, which can happen when a user
* sets the table's selection programmatically and includes a row that is not
* actually part of the table. If this happens we `warn` because of the adverse
* performance impact (the forced call to `setupAllRowMeta`) that is caused by
* spurious rows in the selection.
* @param {CollapseTree} tree The collapse tree for this section (body|footer) of the table
* @param {Set|Array} selection The selected rows
* @return {rowMeta[]} rowMeta for each of the rows in the selection
*/
function mapSelectionToMeta(tree, selection) {
let rowMetaCache = tree.get('rowMetaCache');
let rowMetas = [];
let didSetupAllRowMeta = false;

for (let item of Array.from(selection)) {
let rowMeta = rowMetaCache.get(item);
if (!rowMeta && !didSetupAllRowMeta) {
setupAllRowMeta(tree, tree.get('rows'));
didSetupAllRowMeta = true;
rowMeta = rowMetaCache.get(item);
}

if (!rowMeta && didSetupAllRowMeta) {
warn(
`[ember-table] The selection included a row that was not found in the table's rows. This should be avoided as it causes performance issues. The missing row is: ${JSON.stringify(
item
)}`,
{
id: 'ember-table.selection-invalid',
}
);
} else {
rowMetas.push(rowMeta);
}
}

return rowMetas;
}

/**
Given a list of ordered values and a target value, finds the index of
the closest value which does not exceed the target value
Expand Down
23 changes: 23 additions & 0 deletions tests/helpers/warn-handlers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { registerWarnHandler } from '@ember/debug';

let _registeredHandler = null;

export function setup() {
registerWarnHandler((message, options, next) => {
if (_registeredHandler) {
_registeredHandler(message, options);
} else {
next(message, options);
}
});
}

export function registerTestWarnHandler(callback) {
_registeredHandler = callback;
}

export function teardown() {
if (_registeredHandler) {
_registeredHandler = null;
}
}
62 changes: 62 additions & 0 deletions tests/integration/components/selection-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { generateRows } from 'dummy/utils/generators';
import { A as emberA } from '@ember/array';
import { run } from '@ember/runloop';
import { scrollTo } from 'ember-native-dom-helpers';
import { registerTestWarnHandler } from '../../helpers/warn-handlers';

let table = new TablePage({
validateSelected(...selectedIndexes) {
Expand Down Expand Up @@ -536,6 +537,67 @@ module('Integration | selection', () => {
'After scrolling to bottom, there are still no rows selected'
);
});

test('Issue 747: Programmatically select an un-rendered row', async function(assert) {
let rows = generateRows(200, 1);
await generateTable(this, { rows, bufferSize: 1 });

let renderedRowCount = table.rows.length;
assert.ok(renderedRowCount < 200, 'some rows are occluded');

// Select rows at the end that will not all have been rendered yet
this.set('selection', rows.slice(-5));

await table.rows.objectAt(0).checkbox.click();
assert.ok(true, 'no error');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also assert that the row is selected when you scroll to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add an assertion that it is selected, but this row is at the top of the table so will already be in view.

});

test('Issue 747: Programmatic selection of unrendered children plus manual selection -> selects parent', async function(assert) {
// 1 Parent row with 200 children
let children = generateRows(200, 1);
let rows = generateRows(1, 1);
rows[0].children = children;

await generateTable(this, { rows, selectingChildrenSelectsParent: true, bufferSize: 1 });

let renderedRowCount = table.rows.length;
assert.ok(renderedRowCount < 200, 'some rows are occluded');

// Select all the children but the first. Most have not yet been rendered.
this.set('selection', children.slice(1));

// Select the last un-selected child
await table.rows.objectAt(1).checkbox.click();

assert.ok(true, 'no error');
assert.ok(
table.validateSelected(...allRenderedRowIndexes(table)),
'All rendered rows are selected'
);
assert.ok(table.rows.objectAt(0).isSelected, 'Parent row is selected');
});

test('Issue 747: Programmatic selection that includes a row not part of `rows`', async function(assert) {
let capturedWarningIds = [];
registerTestWarnHandler((_message, { id }) => capturedWarningIds.push(id));

let rows = generateRows(1, 1);
await generateTable(this, { rows });

this.set('selection', [...rows, { fakeRow: true }]);
assert.ok(true, 'after setting bad selection, no error');
assert.ok(table.validateSelected(0), 'First row is selected');

// De-select selected row
await table.rows.objectAt(0).checkbox.click();
assert.ok(true, 'after un-checking, no error');
assert.ok(table.validateSelected(), 'No rows remain selected');

assert.ok(
capturedWarningIds.includes('ember-table.selection-invalid'),
'ember-table warns about invalid selection'
);
});
});
});
});
13 changes: 13 additions & 0 deletions tests/test-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,21 @@ import config from '../config/environment';
import registerRAFWaiter from 'ember-raf-scheduler/test-support/register-waiter';
import { setApplication } from '@ember/test-helpers';
import { start } from 'ember-qunit';
import QUnit from 'qunit';
import {
setup as setupWarnHandlers,
teardown as teardownWarnHandlers,
} from './helpers/warn-handlers';

registerRAFWaiter();
setApplication(Application.create(config.APP));

QUnit.testStart(() => {
setupWarnHandlers();
});

QUnit.testDone(() => {
teardownWarnHandlers();
});

start();