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

Ensure that table column widths are recomputed when columns change #690

Merged
merged 7 commits into from
May 20, 2019
16 changes: 11 additions & 5 deletions addon/components/ember-thead/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { notEmpty, or } from '@ember/object/computed';

import { closest } from '../../-private/utils/element';
import { sortMultiple, compareValues } from '../../-private/utils/sort';
import { scheduleOnce } from '@ember/runloop';

import ColumnTree, { RESIZE_MODE, FILL_MODE, WIDTH_CONSTRAINT } from '../../-private/column-tree';

Expand Down Expand Up @@ -195,7 +196,7 @@ export default Component.extend({
this.addObserver('reorderFunction', this._updateApi);

this.addObserver('sorts', this._updateColumnTree);
this.addObserver('columns', this._updateColumnTree);
this.addObserver('columns.[]', this._onColumnsChange);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, modification via mutation will not be noticed and _onColumnsChange is not called.

Copy link
Member

Choose a reason for hiding this comment

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

Is it a viable alternative to require the invoking code to only provide new arrays for columns? More of an immutable pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

We did discuss that @mixonic, but usage that we've seen really runs the gamut. Feels a little awkward to disallow mutation with Ember compatible mutations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be one option, yes, and that would fix the problem. It wouldn't be hard to change it in our consuming app code (I don't think). But not making ember-table's awareness of columns kvo-compliant with Ember-provided kvo-compliant methods like popObject seems to me that it would violate the implicit contract that ember-table makes.

this.addObserver('fillMode', this._updateColumnTree);
this.addObserver('resizeMode', this._updateColumnTree);
this.addObserver('widthConstraint', this._updateColumnTree);
Expand Down Expand Up @@ -225,17 +226,22 @@ export default Component.extend({
this.columnTree.set('enableReorder', this.get('enableReorder'));
},

_onColumnsChange() {
if (this.get('columns.length') === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

When would a table have 0 columns?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm relatively certain this was to address issues with the testing scenario whcih has a button that removes a column, if you continue to remove columns after you reach 0 you get a unrelated error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our app usage, sometimes the data provided to an {{ember-table}} is empty for a little while, for instance when the source data for a table is refreshed. Without this check, ET2 will throw an exception in those cases where there are (temporarily) 0 columns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewed in-person with @mixonic . We hypothesized that the issues we see in our app may be due to interim state, where a columns property is set to [] for a moment (before later on being set to the non-empty server-provided data) and the observers in ember-thead fire eagerly. If that were the case, the _onColumnsChange hook here might be firing multiple times before the columns data has "settled" to its final state in its runloop.
I tried using a counter variable in this method that increments on each call and decrements in the scheduled callback, only doing work when it decrements back to 0, but that didn't solve the issues. It may be a mistake in our app that causes this, but we certainly run into a state where columns.length is 0 when we call fillupHandler, which causes ember-table to throw (see below).

After a bit of research, it looks like the ColumnTreeNode code (in column-tree) makes the incorrect assumption that any column that isLeaf cannot be the root, and any non-isLeaf node must have subcolumnNodes under it. When we have 0 columns, though, we end up with only a single ColumnTreeNode that is both the root and isLeaf. The ensureWidthConstraint method there runs into trouble because its assumptions about leaf vs non-leaf nodes breaks down in this case. This seems like an issue that should be fixed, but the fix can be separated from this particular PR. This PR improves the situation for adding/removing columns and doesn't fix the issue with 0 columns (but doesn't make it any worse, either).

return;
}
this._updateColumnTree();
scheduleOnce('actions', this, this.fillupHandler);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the schedule, modification via mutation will not pass tests because when fillupHandler calls ensureWidthConstraint, the root's subcolumn nodes have not been updated (i.e., it doesn't yet see that a column was removed)

Copy link
Member

Choose a reason for hiding this comment

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

@bantic something in the logic of _updateColumnTree is scheduling, or causing a schedule, into actions then? Did you hunt down what this is, and can you point me at it? I looked at _updateColumnTree and don't see anything scheduling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was just observer related timing problems. Am I wrong @mixonic?

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 haven't looked into the downstream source of the problem. The code uses what seems like an intermediate "caching" layer and I am not clear on how it works — the problem that this scheduleOnce fixes is that the ensureWidthConstraint that is called (by fillupHandler) looks at a different property to get the number of columns: the column-tree's root.subcolumnNodes. That is the property that isn't in sync with columns until/unless we use the schedule.

},

didInsertElement() {
this._super(...arguments);

this._container = closest(this.element, '.ember-table');

this.columnTree.registerContainer(this._container);

this._tableResizeSensor = new ResizeSensor(
this._container,
bind(this.fillupHandler.bind(this))
);
this._tableResizeSensor = new ResizeSensor(this._container, bind(this, this.fillupHandler));
},

willDestroyElement() {
Expand Down
168 changes: 168 additions & 0 deletions tests/integration/components/headers/ember-thead-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { click, render } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import TablePage from 'ember-table/test-support/pages/ember-table';
import { A } from '@ember/array';

function tableData() {
return {
rows: [
{ A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' },
{ A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' },
{ A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' },
],
columns: [
{ name: 'A', valuePath: 'A', width: 180 },
{ name: 'B', valuePath: 'B', width: 180 },
{ name: 'C', valuePath: 'C', width: 180 },
{ name: 'D', valuePath: 'D', width: 180 },
],
newColumn: { name: 'E', valuePath: 'E', width: 180 },
};
}

function sumHeaderWidths(table) {
return table.headers.map(h => h.width).reduce((sum, w) => sum + w, 0);
}

async function renderTable() {
await render(hbs`
<button id="add-column" {{action 'addColumn'}}>Add Column</button>
<button id="remove-column" {{action 'removeColumn'}}>Remove Column</button>
{{#ember-table data-test-ember-table=true as |t|}}
{{#t.head
widthConstraint='eq-container'
columns=columns as |h|}}
{{#h.row as |r|}}
{{r.cell}}
{{/h.row}}
{{/t.head}}

{{#t.body rows=rows as |b|}}
{{#b.row as |r|}}
{{#r.cell as |cellValue|}}
{{cellValue}}
{{/r.cell}}
{{/b.row}}
{{/t.body}}
{{/ember-table}}
`);
}

async function testColumnRemovals(assert, table) {
let originalWidth = table.width;
let originalContainerWidth = table.containerWidth;

let currentColumnCount = table.headers.length;
assert.equal(currentColumnCount, 4, 'precond - 4 columns');
assert.equal(sumHeaderWidths(table), originalWidth, 'precond - headers sum to table width');
assert.equal(
originalWidth,
originalContainerWidth,
'precond - table is as wide as its container'
);

while (currentColumnCount > 1) {
await click('button#remove-column');
assert.equal(
table.headers.length,
currentColumnCount - 1,
`column count changes from ${currentColumnCount} -> ${currentColumnCount - 1}`
);
currentColumnCount -= 1;

assert.equal(
table.width,
originalWidth,
`table width is same after removal of column #${currentColumnCount}.`
);
assert.equal(
table.containerWidth,
originalContainerWidth,
`new table container is same size as original container after removal of column #${currentColumnCount}.`
);
assert.equal(
sumHeaderWidths(table),
originalWidth,
`headers sum to table width after removal of column #${currentColumnCount}`
);
assert.equal(
table.width,
table.containerWidth,
`new table width is as wide as its container after removal of column #${currentColumnCount}.`
);
}
}

async function testColumnAddition(assert, table) {
let originalWidth = table.width;
let originalContainerWidth = table.containerWidth;

let currentColumnCount = table.headers.length;
assert.equal(currentColumnCount, 4, 'precond - 4 columns');
assert.equal(sumHeaderWidths(table), originalWidth, 'precond - headers sum to table width');
assert.equal(
originalWidth,
originalContainerWidth,
'precond - table is as wide as its container'
);

await click('#add-column');
assert.equal(table.headers.length, 5, 'column is added');
assert.equal(sumHeaderWidths(table), originalWidth, 'headers sum to table width after adding');
assert.equal(table.width, originalWidth, 'table width is unchanged');
assert.equal(table.containerWidth, originalContainerWidth, 'table container width is unchanged');
}

module('ember-thead', function(hooks) {
setupRenderingTest(hooks);

test('table resizes when columns are removed', async function(assert) {
let data = tableData();
this.set('rows', data.rows);
this.set('columns', data.columns);
this.set('removeColumn', function() {
this.set('columns', this.get('columns').slice(0, -1));
});

await renderTable();
await testColumnRemovals(assert, new TablePage());
});

test('table resizes when columns are removed via mutation', async function(assert) {
let data = tableData();
this.set('rows', data.rows);
this.set('columns', A(data.columns));
this.set('removeColumn', function() {
this.get('columns').popObject();
});

await renderTable();
await testColumnRemovals(assert, new TablePage());
});

test('table resizes when columns are added', async function(assert) {
let data = tableData();
this.set('rows', data.rows);
this.set('columns', data.columns);
this.set('addColumn', function() {
this.set('columns', [...data.columns, data.newColumn]);
});

await renderTable();
await testColumnAddition(assert, new TablePage());
});

test('table resizes when columns are added via mutation', async function(assert) {
let data = tableData();
this.set('rows', data.rows);
this.set('columns', A(data.columns));
this.set('addColumn', function() {
this.get('columns').pushObject(data.newColumn);
});

await renderTable();
await testColumnAddition(assert, new TablePage());
});
});