diff --git a/packages/components/src/components/hds/table/index.ts b/packages/components/src/components/hds/table/index.ts index e087abd4509..0e7f9dcf434 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,6 +124,9 @@ export default class HdsTable extends Component { if (this.args.identityKey === 'none') { return undefined; } else { + // 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'; } } @@ -289,7 +292,19 @@ 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): string => row.selectionKey) + ); + 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(); @@ -297,9 +312,13 @@ export default class HdsTable extends Component { @action willDestroyRowCheckbox(selectionKey?: string): void { - this._selectableRows = this._selectableRows.filter( - (row) => row.selectionKey !== selectionKey + // TEST: + console.log( + `[${this.args.caption}] Will destroy row checkbox`, + selectionKey, + this._selectableRows.map((row): string => row.selectionKey) ); + this.setSelectAllState(); } @@ -308,7 +327,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..edfb82bda41 --- /dev/null +++ b/showcase/app/controllers/components/table-multi-select-bug.js @@ -0,0 +1,49 @@ +/** + * 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() { + 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); + } + + @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/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..a9e7d90f531 --- /dev/null +++ b/showcase/app/templates/components/table-multi-select-bug.hbs @@ -0,0 +1,76 @@ +{{! + 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 diff --git a/showcase/tests/integration/components/hds/table/index-test.js b/showcase/tests/integration/components/hds/table/index-test.js index dd7718f14ee..4d4c5c40c9e 100644 --- a/showcase/tests/integration/components/hds/table/index-test.js +++ b/showcase/tests/integration/components/hds/table/index-test.js @@ -74,43 +74,70 @@ const setSelectableTableData = (context) => { ]); }; -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');