Skip to content

Commit

Permalink
fix: prevent overriding column hidden state when attaching column gro…
Browse files Browse the repository at this point in the history 
…up (#3805) (#3821)
  • Loading branch information
@sissbruecker
sissbruecker committed May 11, 2022
1 parent b278188 commit ee87eb9
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 40 deletions.
65 changes: 34 additions & 31 deletions packages/grid/src/vaadin-grid-column-group.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,8 @@ class GridColumnGroup extends ColumnBaseMixin(PolymerElement) {

static get observers() {
return [
'_updateVisibleChildColumns(_childColumns)',
'_childColumnsChanged(_childColumns)',
'_groupFrozenChanged(frozen, _rootColumns)',
'_groupHiddenChanged(hidden, _rootColumns)',
'_visibleChildColumnsChanged(_visibleChildColumns)',
'_groupHiddenChanged(hidden)',
'_colSpanChanged(_colSpan, _headerCell, _footerCell)',
'_groupOrderChanged(_order, _rootColumns)',
'_groupReorderStatusChanged(_reorderStatus, _rootColumns)',
Expand Down Expand Up @@ -117,9 +114,12 @@ class GridColumnGroup extends ColumnBaseMixin(PolymerElement) {
*/
_columnPropChanged(path, value) {
if (path === 'hidden') {
this._preventHiddenCascade = true;
// Prevent synchronization of the hidden state to child columns.
// If the group is currently auto-hidden, and one column is made visible,
// we don't want the other columns to become visible as well.
this._preventHiddenSynchronization = true;
this._updateVisibleChildColumns(this._childColumns);
this._preventHiddenCascade = false;
this._preventHiddenSynchronization = false;
}

if (/flexGrow|width|hidden|_childColumns/.test(path)) {
Expand Down Expand Up @@ -192,14 +192,8 @@ class GridColumnGroup extends ColumnBaseMixin(PolymerElement) {
/** @private */
_updateVisibleChildColumns(childColumns) {
this._visibleChildColumns = Array.prototype.filter.call(childColumns, (col) => !col.hidden);
}

/** @private */
_childColumnsChanged(childColumns) {
if (!this._autoHidden && this.hidden) {
Array.prototype.forEach.call(childColumns, (column) => (column.hidden = true));
this._updateVisibleChildColumns(childColumns);
}
this._colSpan = this._visibleChildColumns.length;
this._updateAutoHidden();
}

/** @protected */
Expand Down Expand Up @@ -240,26 +234,31 @@ class GridColumnGroup extends ColumnBaseMixin(PolymerElement) {
}

/** @private */
_groupHiddenChanged(hidden, rootColumns) {
if (rootColumns && !this._preventHiddenCascade) {
this._ignoreVisibleChildColumns = true;
rootColumns.forEach((column) => (column.hidden = hidden));
this._ignoreVisibleChildColumns = false;
_groupHiddenChanged(hidden) {
// When initializing the hidden property, only sync hidden state to columns
// if group is actually hidden. Otherwise, we could override a hidden column
// to be visible.
// We always want to run this though if the property is actually changed.
if (hidden || this.__groupHiddenInitialized) {
this._synchronizeHidden();
}
this.__groupHiddenInitialized = true;
}

this._columnPropChanged('hidden');
/** @private */
_updateAutoHidden() {
const wasAutoHidden = this._autoHidden;
this._autoHidden = (this._visibleChildColumns || []).length === 0;
// Only modify hidden state if group was auto-hidden, or becomes auto-hidden
if (wasAutoHidden || this._autoHidden) {
this.hidden = this._autoHidden;
}
}

/** @private */
_visibleChildColumnsChanged(visibleChildColumns) {
this._colSpan = visibleChildColumns.length;

if (!this._ignoreVisibleChildColumns) {
if (visibleChildColumns.length === 0) {
this._autoHidden = this.hidden = true;
} else if (this.hidden && this._autoHidden) {
this._autoHidden = this.hidden = false;
}
_synchronizeHidden() {
if (this._childColumns && !this._preventHiddenSynchronization) {
this._childColumns.forEach((column) => (column.hidden = this.hidden));
}
}

Expand Down Expand Up @@ -291,10 +290,14 @@ class GridColumnGroup extends ColumnBaseMixin(PolymerElement) {
info.addedNodes.filter(this._isColumnElement).length > 0 ||
info.removedNodes.filter(this._isColumnElement).length > 0
) {
this._preventHiddenCascade = true;
// Prevent synchronization of the hidden state to child columns.
// If the group is currently auto-hidden, and a visible column is added,
// we don't want the other columns to become visible as well.
this._preventHiddenSynchronization = true;
this._rootColumns = this._getChildColumns(this);
this._childColumns = this._rootColumns;
this._preventHiddenCascade = false;
this._updateVisibleChildColumns(this._childColumns);
this._preventHiddenSynchronization = false;

// Update the column tree with microtask timing to avoid shady style scope issues
microTask.run(() => {
Expand Down
30 changes: 21 additions & 9 deletions packages/grid/test/column-group.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,21 +81,21 @@ describe('column group', () => {
expect(columns[1].frozen).to.be.true;
});

it('should hide group column', () => {
it('should hide when all columns are hidden', () => {
columns[0].hidden = true;
columns[1].hidden = true;

expect(group.hidden).to.be.true;
});

it('should unhide group column', () => {
it('should unhide when making child column visible', () => {
group.hidden = true;
columns[0].hidden = false;

expect(group.hidden).to.be.false;
});

it('should not unhide other columns', () => {
it('should not unhide other columns when making a column visible', () => {
group.hidden = true;
columns[0].hidden = false;

Expand All @@ -109,18 +109,15 @@ describe('column group', () => {
expect(columns[0].hidden).to.be.true;
expect(columns[1].hidden).to.be.true;
expect(group.hidden).to.be.true;
});

it('should propagate hidden to child columns 2', () => {
group.hidden = true;
group.hidden = false;

expect(columns[0].hidden).to.be.false;
expect(columns[1].hidden).to.be.false;
expect(group.hidden).to.be.false;
});

it('should hide the group', () => {
it('should hide when removing all child columns', () => {
group.removeChild(columns[0]);
group.removeChild(columns[1]);
flush();
Expand All @@ -129,7 +126,7 @@ describe('column group', () => {
expect(group.hidden).to.be.true;
});

it('should unhide the group', () => {
it('should unhide when adding a visible column', () => {
group.removeChild(columns[0]);
group.removeChild(columns[1]);
group._observer.flush();
Expand All @@ -140,7 +137,7 @@ describe('column group', () => {
expect(group.hidden).to.be.false;
});

it('should not unhide the group', () => {
it('should not unhide when adding a hidden column', () => {
group.removeChild(columns[0]);
group.removeChild(columns[1]);
group._observer.flush();
Expand All @@ -152,6 +149,21 @@ describe('column group', () => {
expect(group.hidden).to.be.true;
});

// Regression test for https://github.com/vaadin/flow-components/issues/2959
it('should not unhide columns when attached to DOM', () => {
const group = document.createElement('vaadin-grid-column-group');
const visibleColumn = document.createElement('vaadin-grid-column');
const hiddenColumn = document.createElement('vaadin-grid-column');
hiddenColumn.hidden = true;

group.appendChild(visibleColumn);
group.appendChild(hiddenColumn);
document.body.appendChild(group);
group._observer.flush();

expect(hiddenColumn.hidden).to.be.true;
});

it('should calculate column group width after hiding a column', () => {
columns[0].hidden = true;

Expand Down

0 comments on commit ee87eb9

Please sign in to comment.