Skip to content

Commit e4c5ae6

Browse files
authored
Merge pull request #837 from Addepar/jiayingxu/table-sticky-footer-offset-fix
The `TableStickyPolyfill` is used to position header and footer rows. The individual `th` and `td` offset positions are calculated for the footer rows so they remain “sticky”. There is a bug in the logic for calculating the bottom offset for table footer rows where the `rows` were being iterated backwards while the `heights` were iterated forwards. This bug manifests when table footer rows have varying heights and the list of heights is not a palindrome. As shown in the image below, the table has 7 footer rows where the height of the 1st footer row is 74px and the height of all other footer rows is 26px. The bottom offset of the 6th row takes into account the height of the 1st row and not the 7th row due to the loop iteration direction bug. The list `heights: [74, 26, 26, 26, 26, 26, 26]` is errantly being read in reverse order as `heights: [26, 26, 26, 26, 26, 26, 74]`. The ember-twiddle demonstrates the issue [here](https://ember-twiddle.com/35a7259172fd509c4da030ac803fa937 ). ![image](https://user-images.githubusercontent.com/42778466/97247629-6c290580-17d6-11eb-90d2-c8fd66faeced.png)
2 parents 72a2b78 + 50d74f4 commit e4c5ae6

File tree

2 files changed

+110
-7
lines changed

2 files changed

+110
-7
lines changed

addon/-private/sticky/table-sticky-polyfill.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ class TableStickyPolyfill {
9292
add row 0's height (25px) to its current offset and move on to row 1,
9393
where it will set each of that row's `th` cells' `top` to the current
9494
offset of `25px`.
95-
* For the tfoot TableStickyPolyfill, its `respositionStickyElements` will
95+
* For the tfoot TableStickyPolyfill, its `repositionStickyElements` will
9696
start at the bottom-most row, row 1, and set each of its `td` cells'
9797
`bottom` value to `0px`, then add row 1's height (20px) to its current
9898
offset and move on to the next row, row 0, where it will set each of that
@@ -187,8 +187,9 @@ class TableStickyPolyfill {
187187
for (let i = 0; i < rows.length; i++) {
188188
// Work top-down (index order) for 'top', bottom-up (reverse index
189189
// order) for 'bottom' rows
190-
let row = rows[this.side === 'top' ? i : rows.length - 1 - i];
191-
let height = heights[i];
190+
let index = this.side === 'top' ? i : rows.length - 1 - i;
191+
let row = rows[index];
192+
let height = heights[index];
192193

193194
for (let child of row.children) {
194195
child.style.position = '-webkit-sticky';

tests/unit/-private/table-sticky-polyfill-test.js

+106-4
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,23 @@ const standardTemplate = hbs`
5252
</div>
5353
`;
5454

55-
function constructMatrix(n, m, prefix = '') {
55+
/**
56+
* Constructs a matrix m by n
57+
* @param m Rows
58+
* @param n Columns
59+
* @param prefix Prefix string
60+
* @param skipPrefixOnRowIndices Skip adding the prefix string to the row (m) indices specified in the list
61+
* @returns {Array} matrix
62+
*/
63+
function constructMatrix(m, n, prefix = '', skipPrefixOnRowIndices = []) {
5664
let rows = emberA();
5765

58-
for (let i = 0; i < n; i++) {
66+
for (let i = 0; i < m; i++) {
5967
let cols = emberA();
6068

61-
for (let j = 0; j < m; j++) {
62-
cols.pushObject(`${m}${prefix}`);
69+
for (let j = 0; j < n; j++) {
70+
let skipPrefix = skipPrefixOnRowIndices.includes(i);
71+
cols.pushObject(skipPrefix ? n : `${n}${prefix}`);
6372
}
6473

6574
rows.pushObject(cols);
@@ -100,6 +109,60 @@ function verifyFooter(assert) {
100109
});
101110
}
102111

112+
/**
113+
* Verifies multi line header when scrolled to the bottom of the table
114+
* @param assert
115+
*/
116+
function verifyMultiLineHeader(assert) {
117+
let firstTableCellOfEachHeaderRow = findAll('thead > tr > th:first-child');
118+
let tableHeaderCellHeights = firstTableCellOfEachHeaderRow.map(
119+
cell => cell.getBoundingClientRect().height
120+
);
121+
let isAllHeaderCellsIdenticalHeights = tableHeaderCellHeights.every(function(cell, i, array) {
122+
return i === 0 || cell === array[i - 1];
123+
});
124+
let firstCellRect = find('thead tr:first-child th:first-child').getBoundingClientRect();
125+
let expectedOffset = firstCellRect.top;
126+
127+
assert.notOk(
128+
isAllHeaderCellsIdenticalHeights,
129+
'precond - header table rows have varying heights'
130+
);
131+
132+
firstTableCellOfEachHeaderRow.forEach(cell => {
133+
let firstCellRect = cell.getBoundingClientRect();
134+
expectedOffset += firstCellRect.height;
135+
assert.equal(expectedOffset, firstCellRect.bottom);
136+
});
137+
}
138+
139+
/**
140+
* Verifies multi line footer when scrolled to the top of the table
141+
* @param assert
142+
*/
143+
function verifyMultiLineFooter(assert) {
144+
let firstTableCellOfEachFooterRow = findAll('tfoot > tr > td:first-child');
145+
let tableFooterCellHeights = firstTableCellOfEachFooterRow.map(
146+
cell => cell.getBoundingClientRect().height
147+
);
148+
let isAllFooterCellsIdenticalHeights = tableFooterCellHeights.every(function(cell, i, array) {
149+
return i === 0 || cell === array[i - 1];
150+
});
151+
let firstCellRect = find('tfoot tr:first-child td:first-child').getBoundingClientRect();
152+
let expectedOffset = firstCellRect.top;
153+
154+
assert.notOk(
155+
isAllFooterCellsIdenticalHeights,
156+
'precond - footer table rows have varying heights'
157+
);
158+
159+
firstTableCellOfEachFooterRow.forEach(cell => {
160+
let firstCellRect = cell.getBoundingClientRect();
161+
expectedOffset += firstCellRect.height;
162+
assert.equal(expectedOffset, firstCellRect.bottom);
163+
});
164+
}
165+
103166
componentModule('Unit | Private | TableStickyPolyfill', function() {
104167
test('it works', async function(assert) {
105168
this.set('headerRows', constructMatrix(3, 3, 'thead'));
@@ -293,4 +356,43 @@ componentModule('Unit | Private | TableStickyPolyfill', function() {
293356
'the bottom of the last header cell is close to 50% of the way down the table'
294357
);
295358
});
359+
360+
test('when the header has rows with varying heights', async function(assert) {
361+
this.set(
362+
'headerRows',
363+
constructMatrix(2, 3, 'table header has multiple lines of content', [1])
364+
);
365+
this.set('bodyRows', constructMatrix(20, 3, 'body'));
366+
this.set('footerRows', constructMatrix(1, 3, 'footer'));
367+
368+
await this.render(standardTemplate);
369+
370+
setupTableStickyPolyfill(find('thead'));
371+
setupTableStickyPolyfill(find('tfoot'));
372+
373+
await wait();
374+
375+
let container = find('.ember-table');
376+
await scrollTo('.ember-table', 0, container.scrollHeight);
377+
378+
verifyMultiLineHeader(assert);
379+
});
380+
381+
test('when the footer has rows with varying heights', async function(assert) {
382+
this.set('headerRows', constructMatrix(1, 3, 'header'));
383+
this.set('bodyRows', constructMatrix(20, 3, 'body'));
384+
this.set(
385+
'footerRows',
386+
constructMatrix(2, 3, 'table footer has multiple lines of content', [1])
387+
);
388+
389+
await this.render(standardTemplate);
390+
391+
setupTableStickyPolyfill(find('thead'));
392+
setupTableStickyPolyfill(find('tfoot'));
393+
394+
await wait();
395+
396+
verifyMultiLineFooter(assert);
397+
});
296398
});

0 commit comments

Comments
 (0)