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
Merged

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

merged 7 commits into from
May 20, 2019

Conversation

bantic
Copy link
Contributor

@bantic bantic commented May 17, 2019

This fixes an issue where removing a column can leave a blank space in the
table because it doesn't recompute column widths. The table's ResizeSensor
is primarily responsible for noticing resizes and updating widths, but when a
column is removed, although the inner table element width changes, the
container .ember-table element does not change its width, and thus the
ResizeSensor never notices, and the column widths are not recomputed.

This modifies the ember-thead observer to have it call fillupHandler when
column count changes.

It also changes the observer in ember-thead to watch columns.[] instead
of just columns, because in the latter case, the fillupHandler will not
be called if a column was removed via mutation (e.g. popObject).

Adds tests for removal via both ways (mutation and this.set('columns', newColumns)). Also adds tests that adding columns also causes column widths
to be computed.

Co-authored-by: Jonathan Jackson

This fixes an issue where removing a column can leave a blank space in the
table because it doesn't recompute column widths. The table's `ResizeSensor`
is primarily responsible for noticing resizes and updating widths, but when a
column is removed, although the inner `table` element width changes, the
container `.ember-table` element does not change its width, and thus the
`ResizeSensor` never notices, and the column widths are not recomputed.

This modifies the `ember-thead` observer to have it call `fillupHandler` when
column count changes.

It also changes the observer in `ember-thead` to watch `columns.[]` instead
of just `columns`, because in the latter case, the `fillupHandler` will not
be called if a column was removed via mutation (e.g. `popObject`).

Adds tests for removal via both ways (mutation and `this.set('columns',
newColumns)`). Also adds tests that adding columns also causes column widths
to be computed.

Co-authored-by: Jonathan Jackson
@bantic bantic requested review from pzuraq, mixonic and rondale-sc May 17, 2019 22:05
@@ -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.

@@ -225,17 +226,19 @@ export default Component.extend({
this.columnTree.set('enableReorder', this.get('enableReorder'));
},

_onColumnsChange() {
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.

@@ -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).

@@ -225,17 +226,19 @@ export default Component.extend({
this.columnTree.set('enableReorder', this.get('enableReorder'));
},

_onColumnsChange() {
this._updateColumnTree();
scheduleOnce('actions', this, this.fillupHandler);
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.

@@ -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
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?

@@ -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

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.

@@ -225,17 +226,22 @@ export default Component.extend({
this.columnTree.set('enableReorder', this.get('enableReorder'));
},

_onColumnsChange() {
if (this.get('columns.length') === 0) {
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.

@@ -225,17 +226,19 @@ export default Component.extend({
this.columnTree.set('enableReorder', this.get('enableReorder'));
},

_onColumnsChange() {
this._updateColumnTree();
scheduleOnce('actions', this, this.fillupHandler);
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?

bantic added 5 commits May 20, 2019 11:07
This should make it so that the tests pass on Ember 1.12 and 1.13.

Use a special `requestAnimationFrame` waiter that waits 2 RAF turns to
ensure that the table's columns are done being laid out when the test
starts measuring their dimensions.
@bantic bantic merged commit ea8a46f into Addepar:master May 20, 2019
@bantic bantic deleted the rondale-sc/testing-column-removal branch May 20, 2019 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants