Skip to content

Commit d57cd34

Browse files
[EuiDataGrid] Actually fix calls to tabbable instead of moving the issue to a different part of the code (#5163)
* Don't process cells multiple times, and furthermore process cells not the entire grid (or worse 😱) * unit test * Moved and updated unit test for getParentCellContent * Refactor tests into better-explained cases * changelog
1 parent c15d456 commit d57cd34

File tree

3 files changed

+50
-5
lines changed

3 files changed

+50
-5
lines changed

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@
1313
- Added `useGeneratedHtmlId` utility, which memoizes the randomly generated ID on mount and prevents regenerated IDs on component rerender ([#5133](https://github.com/elastic/eui/pull/5133))
1414
- Fixed `z-index` styles that were causing parts of `EuiResizableContainer` to overlap `EuiHeader` ([#5164](https://github.com/elastic/eui/pull/5164))
1515

16+
**Bug fixes**
17+
18+
- Fixed [de]optimization bug in `EuiDataGrid` when cells are removed from the DOM via virtualization ([#5163](https://github.com/elastic/eui/pull/5163))
19+
1620
**Theme: Amsterdam**
1721

1822
- Deprecated `display` prop of `EuiTabs` in favor of unified styles and `bottomBorder` ([#5135](https://github.com/elastic/eui/pull/5135))

src/components/datagrid/body/data_grid_body.test.tsx

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { DataGridSortingContext } from '../data_grid_context';
1313
import { schemaDetectors } from '../data_grid_schema';
1414
import { RowHeightUtils } from '../row_height_utils';
1515

16-
import { EuiDataGridBody } from './data_grid_body';
16+
import { EuiDataGridBody, getParentCellContent } from './data_grid_body';
1717

1818
describe('EuiDataGridBody', () => {
1919
const requiredProps = {
@@ -118,4 +118,39 @@ describe('EuiDataGridBody', () => {
118118
});
119119

120120
// TODO: Test tabbing in Cypress
121+
122+
// TODO: Test column resizing in Cypress
123+
});
124+
125+
describe('getParentCellContent', () => {
126+
const doc = document.createDocumentFragment();
127+
128+
const body = document.createElement('body');
129+
doc.appendChild(body);
130+
131+
const cell = document.createElement('div');
132+
cell.setAttribute('data-datagrid-cellcontent', 'true');
133+
body.appendChild(cell);
134+
135+
const span = document.createElement('span');
136+
span.textContent = 'Here comes the text';
137+
cell.appendChild(span);
138+
139+
const text = span.childNodes[0];
140+
141+
it('locates the cell element when starting with the cell itself', () => {
142+
expect(getParentCellContent(cell)).toBe(cell);
143+
});
144+
145+
it('locates the cell element when starting with an element inside the cell', () => {
146+
expect(getParentCellContent(span!)).toBe(cell);
147+
});
148+
149+
it('locates the cell element when starting with a text node inside the cell', () => {
150+
expect(getParentCellContent(text!)).toBe(cell);
151+
});
152+
153+
it('does not locate the cell element when starting outside the cell', () => {
154+
expect(getParentCellContent(body)).toBeNull();
155+
});
121156
});

src/components/datagrid/body/data_grid_body.tsx

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,16 +248,16 @@ const IS_JEST_ENVIRONMENT = global.hasOwnProperty('_isJest');
248248
* and search its ancestors for a div[data-datagrid-cellcontent], if any,
249249
* which is a valid target for disabling tabbing within
250250
*/
251-
function getParentCellContent(_element: Node | HTMLElement) {
251+
export function getParentCellContent(_element: Node | HTMLElement) {
252252
let element: HTMLElement | null =
253253
_element.nodeType === document.ELEMENT_NODE
254254
? (_element as HTMLElement)
255255
: _element.parentElement;
256256

257257
while (
258-
element &&
259-
element.nodeName !== 'div' &&
260-
element.hasAttribute('data-datagrid-cellcontent')
258+
element && // we haven't walked off the document yet
259+
element.nodeName !== 'div' && // looking for a div
260+
!element.hasAttribute('data-datagrid-cellcontent') // that has data-datagrid-cellcontent
261261
) {
262262
element = element.parentElement;
263263
}
@@ -592,10 +592,16 @@ export const EuiDataGridBody: FunctionComponent<EuiDataGridBodyProps> = (
592592
}, [unconstrainedHeight, wrapperDimensions, isFullScreen]);
593593

594594
const preventTabbing = useCallback((records: MutationRecord[]) => {
595+
// multiple mutation records can implicate the same cell
596+
// so be sure to only check each cell once
597+
const processedCells = new Set();
598+
595599
for (let i = 0; i < records.length; i++) {
596600
const record = records[i];
597601
// find the cell content owning this mutation
598602
const cell = getParentCellContent(record.target);
603+
if (processedCells.has(cell)) continue;
604+
processedCells.add(cell);
599605

600606
if (cell) {
601607
// if we found it, disable tabbable elements

0 commit comments

Comments
 (0)