From dd8d1fe75a543d5915c1b7dc9d2a40bf3bdc62af Mon Sep 17 00:00:00 2001 From: Kristin Bradley Date: Tue, 14 Jan 2025 15:25:50 -0800 Subject: [PATCH 1/4] HDS-4273 Add temporary test code for testing bug, add missing types, add default value for identityKey --- .../src/components/hds/table/index.ts | 24 +++++- .../components/table-multi-select-bug.js | 37 ++++++++++ showcase/app/router.ts | 1 + .../components/table-multi-select-bug.hbs | 73 +++++++++++++++++++ 4 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 showcase/app/controllers/components/table-multi-select-bug.js create mode 100644 showcase/app/templates/components/table-multi-select-bug.hbs diff --git a/packages/components/src/components/hds/table/index.ts b/packages/components/src/components/hds/table/index.ts index e087abd4509..8989b219296 100644 --- a/packages/components/src/components/hds/table/index.ts +++ b/packages/components/src/components/hds/table/index.ts @@ -105,7 +105,7 @@ export default class HdsTable extends Component { get getSortCriteria(): string | HdsTableSortingFunction { // get the current column const currentColumn = this.args?.columns?.find( - (column) => column.key === this.sortBy + (column): boolean => column.key === this.sortBy ); if ( // check if there is a custom sorting function associated with the current `sortBy` column (we assume the column has `isSortable`) @@ -124,7 +124,9 @@ export default class HdsTable extends Component { if (this.args.identityKey === 'none') { return undefined; } else { - return this.args.identityKey ?? '@identity'; + // We return a default key to cause an update operation instead of replacement when the model changes to side-step + // a bug in Ember which would cause the table to lose its selection state + return this.args.identityKey ?? 'hds-table-key'; } } @@ -289,6 +291,13 @@ export default class HdsTable extends Component { checkbox: HdsFormCheckboxBaseSignature['Element'], selectionKey?: string ): void { + // TEST: + console.log( + `[${this.args.caption}] Did insert row checkbox`, + selectionKey, + this._selectableRows.map((row) => row.selectionKey) + ); + if (selectionKey) { this._selectableRows.push({ selectionKey, checkbox }); } @@ -297,8 +306,15 @@ export default class HdsTable extends Component { @action willDestroyRowCheckbox(selectionKey?: string): void { + // TEST: + console.log( + `[${this.args.caption}] Will destroy row checkbox`, + selectionKey, + this._selectableRows.map((row) => row.selectionKey) + ); + this._selectableRows = this._selectableRows.filter( - (row) => row.selectionKey !== selectionKey + (row): boolean => row.selectionKey !== selectionKey ); this.setSelectAllState(); } @@ -308,7 +324,7 @@ export default class HdsTable extends Component { if (this._selectAllCheckbox) { const selectableRowsCount = this._selectableRows.length; const selectedRowsCount = this._selectableRows.filter( - (row) => row.checkbox.checked + (row): boolean => row.checkbox.checked ).length; this._selectAllCheckbox.checked = diff --git a/showcase/app/controllers/components/table-multi-select-bug.js b/showcase/app/controllers/components/table-multi-select-bug.js new file mode 100644 index 00000000000..ccb6c446c21 --- /dev/null +++ b/showcase/app/controllers/components/table-multi-select-bug.js @@ -0,0 +1,37 @@ +/** + * Copyright (c) HashiCorp, Inc. + * SPDX-License-Identifier: MPL-2.0 + */ + +import Controller from '@ember/controller'; +import { tracked } from '@glimmer/tracking'; +import { action } from '@ember/object'; + +// prettier-ignore +const daysOfChristmas = [ + { key: 'one', quantity: 1, type: 'bird', line: 'A partridge in a pear tree' }, + { key: 'two', quantity: 2, type: 'bird', line: 'Two turtle doves' }, + { key: 'three', quantity: 3, type: 'bird', line: 'Three french hens' }, + { key: 'four', quantity: 4, type: 'bird', line: 'Four calling birds' }, + { key: 'five', quantity: 5, type: 'jewelry', line: 'Five gold rings' }, + { key: 'six', quantity: 6, type: 'more bird', line: 'Six geese a-laying' }, + { key: 'seven', quantity: 7, type: 'bird again', line: 'Seven swans a-swimming' }, + { key: 'eight', quantity: 8, type: 'servant', line: 'Eight maids a-milking' }, + { key: 'nine', quantity: 9, type: 'maybe royalty', line: 'Nine ladies dancing' }, + { key: 'ten', quantity: 10, type: 'definitely royalty', line: 'Ten lords a-leaping' }, + { key: 'eleven', quantity: 11, type: 'musician', line: 'Eleven pipers piping' }, + { key: 'twelve', quantity: 12, type: 'musician, technically', line: 'Twelve drummers drumming' }, +]; + +export default class extends Controller { + @tracked showTable = false; + @tracked lyrics = daysOfChristmas.slice(); + + @action dirtyModel() { + // Deep copy to destroy references + this.lyrics = this.lyrics.map((r) => ({ ...r })); + } + + printSelection = (selection) => + console.log('Printing selection:', selection.selectedRowsKeys); +} diff --git a/showcase/app/router.ts b/showcase/app/router.ts index 6a16b695a32..b06e57290d6 100644 --- a/showcase/app/router.ts +++ b/showcase/app/router.ts @@ -63,6 +63,7 @@ Router.map(function () { this.route('side-nav'); this.route('stepper'); this.route('table'); + this.route('table-multi-select-bug'); this.route('tag'); this.route('text'); this.route('time'); diff --git a/showcase/app/templates/components/table-multi-select-bug.hbs b/showcase/app/templates/components/table-multi-select-bug.hbs new file mode 100644 index 00000000000..afea41fdb08 --- /dev/null +++ b/showcase/app/templates/components/table-multi-select-bug.hbs @@ -0,0 +1,73 @@ +{{! + Copyright (c) HashiCorp, Inc. + SPDX-License-Identifier: MPL-2.0 +}} + +{{page-title "Table multi-select bug"}} + +Table multi-select bug + +
+

When a table isn't immediately rendered, a bug occurs where table rows render twice. Normally this is not a big + deal, but due to the way that row-level checkboxes are registered, we get this unfortunate sequence of events:

+
    +
  1. didInsertRowCheckbox: Insert selection keys to _selectableRows
    2. +
  2. didInsertRowCheckbox: Insert selection keys to + _selectableRows + a second time. This results in an array that's 2x of every selection key.
    3. +
  3. willDestroyRowCheckbox: Filters + _selectableRows + by removed selection key, except this removes the first and second insertions resulting in an empty array.
    4. +
+ + This will work until you click the "Force new data" button + + + + + <:body as |B|> + + {{B.data.quantity}} + {{B.data.type}} + {{B.data.line}} + + + + + This will always work because + @identityKey + performs an update not a replace. + + + + + <:body as |B|> + + {{B.data.quantity}} + {{B.data.type}} + {{B.data.line}} + + + +
\ No newline at end of file From 67753bf66504e4430a5835487e0b620055f8d418 Mon Sep 17 00:00:00 2001 From: Kristin Bradley Date: Thu, 16 Jan 2025 08:43:06 -0800 Subject: [PATCH 2/4] HDS-4273 Fix issue with selectableRows state when model updates --- .../src/components/hds/table/index.ts | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/components/src/components/hds/table/index.ts b/packages/components/src/components/hds/table/index.ts index 8989b219296..3e189f1c736 100644 --- a/packages/components/src/components/hds/table/index.ts +++ b/packages/components/src/components/hds/table/index.ts @@ -126,7 +126,7 @@ export default class HdsTable extends Component { } else { // We return a default key to cause an update operation instead of replacement when the model changes to side-step // a bug in Ember which would cause the table to lose its selection state - return this.args.identityKey ?? 'hds-table-key'; + return this.args.identityKey ?? '@identity'; } } @@ -295,7 +295,7 @@ export default class HdsTable extends Component { console.log( `[${this.args.caption}] Did insert row checkbox`, selectionKey, - this._selectableRows.map((row) => row.selectionKey) + this._selectableRows.map((row): string => row.selectionKey) ); if (selectionKey) { @@ -310,12 +310,25 @@ export default class HdsTable extends Component { console.log( `[${this.args.caption}] Will destroy row checkbox`, selectionKey, - this._selectableRows.map((row) => row.selectionKey) + this._selectableRows.map((row): string => row.selectionKey) ); - this._selectableRows = this._selectableRows.filter( - (row): boolean => row.selectionKey !== selectionKey + // Fix for selectableRows state not being maintained properly when model updates: + // Delete the first row with the given selction key you find from the selectableRows array + // Search for the index of the row to delete. If found, delete it. + // (Fixes issue of not rows not being properly selectable including "select all" functionality) + const rowToDeleteIndex = this._selectableRows.findIndex( + (row): boolean => row.selectionKey === selectionKey ); + + if (rowToDeleteIndex > -1) { + this._selectableRows.splice(rowToDeleteIndex, 1); + } + + // ORIGINAL: + // this._selectableRows = this._selectableRows.filter( + // (row): boolean => row.selectionKey !== selectionKey + // ); this.setSelectAllState(); } From cd194ac090beec9c37b59468f5f40cdf70fd6945 Mon Sep 17 00:00:00 2001 From: Kristin Bradley Date: Fri, 17 Jan 2025 10:57:06 -0800 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Cristiano Rastelli --- packages/components/src/components/hds/table/index.ts | 7 ++++--- .../app/controllers/components/table-multi-select-bug.js | 7 +++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/components/src/components/hds/table/index.ts b/packages/components/src/components/hds/table/index.ts index 3e189f1c736..765deb16b20 100644 --- a/packages/components/src/components/hds/table/index.ts +++ b/packages/components/src/components/hds/table/index.ts @@ -124,8 +124,9 @@ export default class HdsTable extends Component { if (this.args.identityKey === 'none') { return undefined; } else { - // We return a default key to cause an update operation instead of replacement when the model changes to side-step - // a bug in Ember which would cause the table to lose its selection state + // We return `@identity` as default to cause an update operation instead of replacement when the model changes to avoid unexpected side effects + // see: "Specifying Keys" here: https://api.emberjs.com/ember/6.1/classes/Ember.Templates.helpers/methods/each?anchor=each + // see also: https://github.com/hashicorp/design-system/pull/1160 + https://github.com/hashicorp/cloud-ui/pull/5178 + https://hashicorp.atlassian.net/browse/HDS-4273 return this.args.identityKey ?? '@identity'; } } @@ -314,7 +315,7 @@ export default class HdsTable extends Component { ); // Fix for selectableRows state not being maintained properly when model updates: - // Delete the first row with the given selction key you find from the selectableRows array + // Delete the first row with the given selection key you find from the selectableRows array // Search for the index of the row to delete. If found, delete it. // (Fixes issue of not rows not being properly selectable including "select all" functionality) const rowToDeleteIndex = this._selectableRows.findIndex( diff --git a/showcase/app/controllers/components/table-multi-select-bug.js b/showcase/app/controllers/components/table-multi-select-bug.js index ccb6c446c21..a157ba1da37 100644 --- a/showcase/app/controllers/components/table-multi-select-bug.js +++ b/showcase/app/controllers/components/table-multi-select-bug.js @@ -28,8 +28,11 @@ export default class extends Controller { @tracked lyrics = daysOfChristmas.slice(); @action dirtyModel() { - // Deep copy to destroy references - this.lyrics = this.lyrics.map((r) => ({ ...r })); + this.lyrics = this.lyrics + // Deep copy to destroy references + .map((r) => ({ ...r })) + // Randomly sort to see what happens in this case + .sort(() => Math.random() - 0.5); } printSelection = (selection) => From e0460c192c831b061b261adc123a4f8127f6036b Mon Sep 17 00:00:00 2001 From: Kristin Bradley Date: Fri, 17 Jan 2025 14:49:23 -0800 Subject: [PATCH 4/4] HDS-4273 Add integration test for testing select all on multiselect after model update, add example of updating model with fewer rows, refactor Table component code to update rows instead of deleting duplicates --- .../src/components/hds/table/index.ts | 21 +- .../components/table-multi-select-bug.js | 9 + .../components/table-multi-select-bug.hbs | 5 +- .../components/hds/table/index-test.js | 458 ++++++++++-------- 4 files changed, 262 insertions(+), 231 deletions(-) diff --git a/packages/components/src/components/hds/table/index.ts b/packages/components/src/components/hds/table/index.ts index 765deb16b20..0e7f9dcf434 100644 --- a/packages/components/src/components/hds/table/index.ts +++ b/packages/components/src/components/hds/table/index.ts @@ -300,6 +300,11 @@ export default class HdsTable extends Component { ); if (selectionKey) { + // Remove any existing item with the same `selectionKey` identifier (this may occur if the model is updated and the rows re-rendered) + this._selectableRows = this._selectableRows.filter( + (row): boolean => row.selectionKey !== selectionKey + ); + // Add the new item this._selectableRows.push({ selectionKey, checkbox }); } this.setSelectAllState(); @@ -314,22 +319,6 @@ export default class HdsTable extends Component { this._selectableRows.map((row): string => row.selectionKey) ); - // Fix for selectableRows state not being maintained properly when model updates: - // Delete the first row with the given selection key you find from the selectableRows array - // Search for the index of the row to delete. If found, delete it. - // (Fixes issue of not rows not being properly selectable including "select all" functionality) - const rowToDeleteIndex = this._selectableRows.findIndex( - (row): boolean => row.selectionKey === selectionKey - ); - - if (rowToDeleteIndex > -1) { - this._selectableRows.splice(rowToDeleteIndex, 1); - } - - // ORIGINAL: - // this._selectableRows = this._selectableRows.filter( - // (row): boolean => row.selectionKey !== selectionKey - // ); this.setSelectAllState(); } diff --git a/showcase/app/controllers/components/table-multi-select-bug.js b/showcase/app/controllers/components/table-multi-select-bug.js index a157ba1da37..edfb82bda41 100644 --- a/showcase/app/controllers/components/table-multi-select-bug.js +++ b/showcase/app/controllers/components/table-multi-select-bug.js @@ -35,6 +35,15 @@ export default class extends Controller { .sort(() => Math.random() - 0.5); } + @action dirtyModelWithLessRows() { + this.lyrics = this.lyrics + // Deep copy to destroy references + .map((r) => ({ ...r })) + // Randomly sort to see what happens in this case + .sort(() => Math.random() - 0.5) + .filter((row) => row.key !== 'five'); + } + printSelection = (selection) => console.log('Printing selection:', selection.selectedRowsKeys); } diff --git a/showcase/app/templates/components/table-multi-select-bug.hbs b/showcase/app/templates/components/table-multi-select-bug.hbs index afea41fdb08..a9e7d90f531 100644 --- a/showcase/app/templates/components/table-multi-select-bug.hbs +++ b/showcase/app/templates/components/table-multi-select-bug.hbs @@ -22,7 +22,10 @@ This will work until you click the "Force new data" button - +

+ + +

{ ]); }; -const hbsSortableTable = hbs` - - <:body as |B|> - - {{B.data.artist}} - {{B.data.album}} - {{B.data.year}} - - - -`; - -const hbsSelectableTable = hbs` - - <:body as |B|> - - {{B.data.artist}} - {{B.data.album}} - {{B.data.year}} - - - -`; +const setNewSelectableTableData = (context) => { + context.set('model', [ + { + id: '1', + type: 'folk', + artist: 'Nick Drake', + album: 'Pink Moon', + year: '1972', + }, + { + id: '2', + type: 'folk', + artist: 'The Beatles', + album: 'Abbey Road', + year: '1969', + }, + { + id: '3', + type: 'folk', + artist: 'Melanie', + album: 'Candles in the Rain', + year: '1971', + }, + ]); + context.set('columns', [ + { key: 'artist', label: 'Artist' }, + { key: 'album', label: 'Album' }, + { key: 'year', label: 'Year' }, + ]); +}; + +const hbsSortableTable = hbs` + <:body as |B|> + + {{B.data.artist}} + {{B.data.album}} + {{B.data.year}} + + +`; + +const hbsSelectableTable = hbs` + <:body as |B|> + + {{B.data.artist}} + {{B.data.album}} + {{B.data.year}} + + +`; // Basic tests @@ -118,33 +145,33 @@ module('Integration | Component | hds/table/index', function (hooks) { setupRenderingTest(hooks); test('it should render the component with a CSS class that matches the component name', async function (assert) { - await render(hbs``); + await render(hbs``); assert.dom('#data-test-table').hasClass('hds-table'); }); test('it should render with a CSS class appropriate for the @density value', async function (assert) { - await render(hbs``); + await render(hbs``); assert.dom('#data-test-table').hasClass('hds-table--density-short'); }); test('it should render with a CSS class appropriate if no @density value is set', async function (assert) { - await render(hbs``); + await render(hbs``); assert.dom('#data-test-table').hasClass('hds-table--density-medium'); }); test('it should render with a CSS class appropriate for the @valign value', async function (assert) { - await render(hbs``); + await render(hbs``); assert.dom('#data-test-table').hasClass('hds-table--valign-middle'); }); test('it should render with a CSS class appropriate if no @valign value is set', async function (assert) { - await render(hbs``); + await render(hbs``); assert.dom('#data-test-table').hasClass('hds-table--valign-top'); }); test('it should support splattributes', async function (assert) { await render( - hbs`` + hbs`` ); assert .dom('#data-test-table') @@ -152,34 +179,32 @@ module('Integration | Component | hds/table/index', function (hooks) { }); test('it should render the table with manual data passed and no model defined', async function (assert) { - await render(hbs` - - <:head as |H|> - - Cell Header 1 - Cell Header 2 - Cell Header 3 - - - <:body as |B|> - - Cell Content 1 1 - Cell Content 1 2 - Cell Content 1 3 - - - Cell Content 2 1 - Cell Content 2 2 - Cell Content 2 3 - - - Cell Content 3 1 - Cell Content 3 2 - Cell Content 3 3 - - - - `); + await render(hbs` + <:head as |H|> + + Cell Header 1 + Cell Header 2 + Cell Header 3 + + + <:body as |B|> + + Cell Content 1 1 + Cell Content 1 2 + Cell Content 1 3 + + + Cell Content 2 1 + Cell Content 2 2 + Cell Content 2 3 + + + Cell Content 3 1 + Cell Content 3 2 + Cell Content 3 3 + + +`); assert.dom('#data-test-table tr th:first-of-type').hasText('Cell Header 1'); assert @@ -194,21 +219,23 @@ module('Integration | Component | hds/table/index', function (hooks) { { key: 'year', name: 'Test 3', description: 'Test 3 description' }, ]); - await render(hbs` - - <:body as |B|> - - {{B.data.key}} - {{B.data.name}} - {{B.data.description}} - - - - `); + await render(hbs` + <:body as |B|> + + {{B.data.key}} + {{B.data.name}} + {{B.data.description}} + + +`); assert.dom('#data-test-table tr:nth-child(3)').hasProperty('id', '2'); @@ -227,24 +254,26 @@ module('Integration | Component | hds/table/index', function (hooks) { { id: 3, name: 'Test 3', description: 'Test 3 description' }, ]); - await render(hbs` - - <:head as |H|> - - Id - Name - Description - - - <:body as |B|> - - {{B.data.id}} - {{B.data.name}} - {{B.data.description}} - - - - `); + await render(hbs` + <:head as |H|> + + Id + Name + Description + + + <:body as |B|> + + {{B.data.id}} + {{B.data.name}} + {{B.data.description}} + + +`); assert.dom('#data-test-table caption').hasText('a test caption'); }); @@ -438,30 +467,22 @@ module('Integration | Component | hds/table/index', function (hooks) { } }); - await render(hbs` - - <:body as |B|> - - {{B.data.name}} - {{B.data.age}} - - - - `); + await render(hbs` + <:body as |B|> + + {{B.data.name}} + {{B.data.age}} + + +`); assert.dom(sortBySelectedSelector).exists(); assert @@ -496,34 +517,32 @@ module('Integration | Component | hds/table/index', function (hooks) { }); test('it renders a multi-select table when isSelectable is set to true for a table without a model', async function (assert) { - await render(hbs` - - <:head as |H|> - - Cell Header 1 - Cell Header 2 - Cell Header 3 - - - <:body as |B|> - - Cell Content 1 1 - Cell Content 1 2 - Cell Content 1 3 - - - Cell Content 2 1 - Cell Content 2 2 - Cell Content 2 3 - - - Cell Content 3 1 - Cell Content 3 2 - Cell Content 3 3 - - - - `); + await render(hbs` + <:head as |H|> + + Cell Header 1 + Cell Header 2 + Cell Header 3 + + + <:body as |B|> + + Cell Content 1 1 + Cell Content 1 2 + Cell Content 1 3 + + + Cell Content 2 1 + Cell Content 2 2 + Cell Content 2 3 + + + Cell Content 3 1 + Cell Content 3 2 + Cell Content 3 3 + + +`); assert.dom(selectAllCheckboxSelector).exists({ count: 1 }); assert.dom(rowCheckboxesSelector).exists({ count: 3 }); }); @@ -569,29 +588,31 @@ module('Integration | Component | hds/table/index', function (hooks) { 'onSelectionChange', ({ selectedRowsKeys }) => (keys = selectedRowsKeys) ); - await render(hbs` - - <:head as |H|> - - Cell Header 1 - Cell Header 2 - Cell Header 3 - - - <:body as |B|> - - Cell Content 1 1 - Cell Content 1 2 - Cell Content 1 3 - - - Cell Content 2 1 - Cell Content 2 2 - Cell Content 2 3 - - - - `); + await render(hbs` + <:head as |H|> + + Cell Header 1 + Cell Header 2 + Cell Header 3 + + + <:body as |B|> + + Cell Content 1 1 + Cell Content 1 2 + Cell Content 1 3 + + + Cell Content 2 1 + Cell Content 2 2 + Cell Content 2 3 + + +`); const rowCheckboxes = this.element.querySelectorAll(rowCheckboxesSelector); const firstRowCheckbox = rowCheckboxes[0]; await click(firstRowCheckbox); @@ -602,28 +623,39 @@ module('Integration | Component | hds/table/index', function (hooks) { assert.deepEqual(keys, []); }); + // test that select all functionality works after model updates + test('select all functionality should work after model updates', async function (assert) { + setSelectableTableData(this); + await render(hbsSelectableTable); + // Force a model update + setNewSelectableTableData(this); + + // test that the select all functionality still works + await click(selectAllCheckboxSelector); + assert.dom(selectAllCheckboxSelector).isChecked(); + assert.dom(rowCheckboxesSelector).isChecked().exists({ count: 3 }); + }); + // multi-select options // aria-labels test('it renders the expected `aria-label` values for "select all" and rows by default', async function (assert) { setSelectableTableData(this); - await render(hbs` - - <:body as |B|> - - {{B.data.artist}} - {{B.data.album}} - {{B.data.year}} - - - - `); + await render(hbs` + <:body as |B|> + + {{B.data.artist}} + {{B.data.album}} + {{B.data.year}} + + +`); assert.dom(selectAllCheckboxSelector).hasAria('label', 'Select all rows'); assert.dom(rowCheckboxesSelector).hasAria('label', 'Select row'); @@ -637,25 +669,23 @@ module('Integration | Component | hds/table/index', function (hooks) { test('it renders the expected `aria-label` for rows with `@selectionAriaLabelSuffix`', async function (assert) { setSelectableTableData(this); - await render(hbs` - - <:body as |B|> - - {{B.data.artist}} - {{B.data.album}} - {{B.data.year}} - - - - `); + await render(hbs` + <:body as |B|> + + {{B.data.artist}} + {{B.data.album}} + {{B.data.year}} + + +`); assert.dom(rowCheckboxesSelector).hasAria('label', 'Select custom suffix');