From aceddf3422132292dbaa04b83888eeb4feb5b5b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Rivet?= Date: Tue, 14 Apr 2020 10:49:40 -0400 Subject: [PATCH] Issue 649 - Fix mismatched row height for fixed columns (#722) --- packages/dash-table/.circleci/config.yml | 2 +- packages/dash-table/CHANGELOG.md | 4 + packages/dash-table/demo/data.ts | 10 +- .../src/dash-table/components/Cell/index.tsx | 4 +- .../src/dash-table/components/Cell/props.ts | 2 +- .../components/CellDropdown/index.tsx | 2 +- .../components/CellMarkdown/index.tsx | 2 +- .../components/ControlledTable/index.tsx | 192 +++---- .../dash-table/components/Filter/Column.tsx | 6 +- .../dash-table/components/FilterFactory.tsx | 2 +- .../dash-table/components/Table/Table.less | 497 ++++++++++-------- .../src/dash-table/derived/cell/wrappers.tsx | 10 +- .../dash-table/derived/table/fragments.tsx | 60 ++- .../dash-table/tests/cypress/src/DashTable.ts | 64 ++- .../tests/cypress/support/commands.js | 2 +- .../cypress/tests/standalone/column_test.ts | 33 +- .../tests/standalone/delete_row_test.ts | 2 +- .../tests/standalone/edit_cell_test.ts | 71 ++- .../cypress/tests/standalone/edit_headers.ts | 52 +- .../tests/standalone/filtering_test.ts | 155 +++--- .../tests/standalone/formatting_test.ts | 21 +- .../cypress/tests/standalone/markdown_test.ts | 49 +- .../tests/standalone/navigation_test.ts | 112 ++-- .../standalone/readonly_navigation_test.ts | 15 +- .../tests/standalone/readonly_sorting_test.ts | 2 +- .../standalone/readwrite_navigation_test.ts | 37 +- .../standalone/readwrite_sorting_test.ts | 2 +- .../tests/standalone/scrolling_test.ts | 18 +- .../standalone/select_row_column_test.ts | 4 +- .../tests/standalone/selection_test.ts | 39 +- .../cypress/tests/standalone/tooltip_test.ts | 4 +- .../dash-table/tests/selenium/conftest.py | 25 +- .../tests/selenium/test_navigation.py | 187 +++++++ .../percy-storybook/MarkdownCells.percy.tsx | 241 +++++---- .../percy-storybook/Multiline.percy.tsx | 62 +++ .../visual/percy-storybook/Sizing.percy.tsx | 10 +- 36 files changed, 1212 insertions(+), 788 deletions(-) create mode 100644 packages/dash-table/tests/selenium/test_navigation.py create mode 100644 packages/dash-table/tests/visual/percy-storybook/Multiline.percy.tsx diff --git a/packages/dash-table/.circleci/config.yml b/packages/dash-table/.circleci/config.yml index bb9fafeea0..eb4ec7c5ce 100644 --- a/packages/dash-table/.circleci/config.yml +++ b/packages/dash-table/.circleci/config.yml @@ -3,7 +3,7 @@ version: 2 jobs: "server-test": docker: - - image: circleci/python:3.7.5-node-browsers + - image: circleci/python:3.7-node-browsers - image: cypress/base:10 steps: diff --git a/packages/dash-table/CHANGELOG.md b/packages/dash-table/CHANGELOG.md index b311679b78..c396cdbe5e 100644 --- a/packages/dash-table/CHANGELOG.md +++ b/packages/dash-table/CHANGELOG.md @@ -2,6 +2,10 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] +### Fixed +- [#722](https://github.com/plotly/dash-table/pull/722) Fix a bug where row height is misaligned when using fixed_columns and/or fixed_rows + ## [4.6.2] - 2020-04-01 ### Changed - [#713](https://github.com/plotly/dash-table/pull/713) Update from React 16.8.6 to 16.13.0 diff --git a/packages/dash-table/demo/data.ts b/packages/dash-table/demo/data.ts index 0a4251ee96..03def98ec4 100644 --- a/packages/dash-table/demo/data.ts +++ b/packages/dash-table/demo/data.ts @@ -176,9 +176,15 @@ export const generateMixedMarkdownMockData = (rows: number) => unpackIntoColumns name: ['Markdown'], type: ColumnType.Text, presentation: 'markdown', - data: gendata(_ => [ + data: gendata(i => [ '```javascript', - 'console.warn("this is a markdown cell")', + ...(i % 2 === 0 ? + ['console.warn("this is a markdown cell")'] : + [ + 'console.log("logging things")', + 'console.warn("this is a markdown cell")' + ] + ), '```'].join('\n'), rows) }, { diff --git a/packages/dash-table/src/dash-table/components/Cell/index.tsx b/packages/dash-table/src/dash-table/components/Cell/index.tsx index 22da2308f3..f22adb07c2 100644 --- a/packages/dash-table/src/dash-table/components/Cell/index.tsx +++ b/packages/dash-table/src/dash-table/components/Cell/index.tsx @@ -22,7 +22,7 @@ export default class Cell extends Component { render() { const { attributes, - classes, + className, onClick, onDoubleClick, onMouseEnter, @@ -35,7 +35,7 @@ export default class Cell extends Component { ref='td' children={(this as any).props.children} tabIndex={-1} - className={classes} + className={className} onClick={onClick} onDoubleClick={onDoubleClick} onMouseEnter={onMouseEnter} diff --git a/packages/dash-table/src/dash-table/components/Cell/props.ts b/packages/dash-table/src/dash-table/components/Cell/props.ts index d0ba01c465..f596e0b423 100644 --- a/packages/dash-table/src/dash-table/components/Cell/props.ts +++ b/packages/dash-table/src/dash-table/components/Cell/props.ts @@ -7,7 +7,7 @@ interface IAttributes { export interface ICellProps { active: boolean; attributes: IAttributes; - classes: string; + className: string; onClick: (e: MouseEvent) => void; onDoubleClick: (e: MouseEvent) => void; onMouseEnter: (e: MouseEvent) => void; diff --git a/packages/dash-table/src/dash-table/components/CellDropdown/index.tsx b/packages/dash-table/src/dash-table/components/CellDropdown/index.tsx index 30ae7a1842..538638ffa6 100644 --- a/packages/dash-table/src/dash-table/components/CellDropdown/index.tsx +++ b/packages/dash-table/src/dash-table/components/CellDropdown/index.tsx @@ -75,7 +75,7 @@ export default class CellDropdown extends PureComponent { if (applyFocus && dropdown && document.activeElement !== dropdown) { // Limitation. If React >= 16 --> Use React.createRef instead to pass parent ref to child const tdParent = DOM.getFirstParentOfType(dropdown.wrapper, 'td'); - if (tdParent) { + if (tdParent && tdParent.className.indexOf('phantom-cell') === -1) { tdParent.focus(); } } diff --git a/packages/dash-table/src/dash-table/components/CellMarkdown/index.tsx b/packages/dash-table/src/dash-table/components/CellMarkdown/index.tsx index 49931c830b..a3fba0e6bf 100644 --- a/packages/dash-table/src/dash-table/components/CellMarkdown/index.tsx +++ b/packages/dash-table/src/dash-table/components/CellMarkdown/index.tsx @@ -62,7 +62,7 @@ export default class CellMarkdown extends PureComponent { if (applyFocus && el && document.activeElement !== el) { // Limitation. If React >= 16 --> Use React.createRef instead to pass parent ref to child const tdParent = DOM.getFirstParentOfType(el, 'td'); - if (tdParent) { + if (tdParent && tdParent.className.indexOf('phantom-cell') !== -1) { tdParent.focus(); } } diff --git a/packages/dash-table/src/dash-table/components/ControlledTable/index.tsx b/packages/dash-table/src/dash-table/components/ControlledTable/index.tsx index c633c8e36c..85346c6455 100644 --- a/packages/dash-table/src/dash-table/components/ControlledTable/index.tsx +++ b/packages/dash-table/src/dash-table/components/ControlledTable/index.tsx @@ -48,6 +48,9 @@ const INNER_STYLE = { minWidth: '100%' }; +const WIDTH_EPSILON = 0.5; +const MAX_WIDTH_ITERATIONS = 30; + export default class ControlledTable extends PureComponent { private readonly menuRef = React.createRef(); private readonly stylesheet: Stylesheet = new Stylesheet(`#${this.props.id}`); @@ -134,7 +137,7 @@ export default class ControlledTable extends PureComponent setProps({ active_cell: selected_cells[0] }); } - this.applyStyle(); + this.updateUiViewport(); this.handleResize(); } @@ -146,7 +149,7 @@ export default class ControlledTable extends PureComponent componentDidUpdate() { this.updateStylesheet(); - this.applyStyle(); + this.updateUiViewport(); this.handleResize(); this.handleDropdown(); this.adjustTooltipPosition(); @@ -226,6 +229,8 @@ export default class ControlledTable extends PureComponent handleResize = (force: boolean = false) => { const { + fixed_columns, + fixed_rows, forcedResizeOnly, setState } = this.props; @@ -244,6 +249,7 @@ export default class ControlledTable extends PureComponent const { r0c0, r0c1, r1c0, r1c1 } = this.refs as { [key: string]: HTMLElement }; + // Adjust [fixed columns/fixed rows combo] to fixed rows height let trs = r0c1.querySelectorAll('tr'); Array.from(r0c0.querySelectorAll('tr')).forEach((tr, index) => { @@ -261,12 +267,77 @@ export default class ControlledTable extends PureComponent tr.style.height = getComputedStyle(tr2).height; }); - // Adjust fixed columns data to data height - const contentTd = r1c1.querySelector('tr > td:first-of-type'); - if (contentTd) { - const contentTr = contentTd.parentElement as HTMLElement; + if (fixed_columns) { + const r1c0Table = r1c0.querySelector('table') as HTMLElement; + const r1c1Table = r1c0.querySelector('table') as HTMLElement; + + r1c0Table.style.width = getComputedStyle(r1c1Table).width; + + const lastVisibleTd = r1c0.querySelector(`tr:first-of-type > *:nth-of-type(${fixed_columns})`); + + let it = 0; + let currentWidth = r1c0.getBoundingClientRect().width; + let lastWidth = currentWidth; + + do { + lastWidth = currentWidth + + // Force first column containers width to match visible portion of table + if (lastVisibleTd) { + const r1c0FragmentBounds = r1c0.getBoundingClientRect(); + const lastTdBounds = lastVisibleTd.getBoundingClientRect(); + currentWidth = lastTdBounds.right - r1c0FragmentBounds.left; + + const width = `${currentWidth}px`; + + r0c0.style.width = width; + r1c0.style.width = width; + } + + // Force second column containers width to match visible portion of table + const firstVisibleTd = r1c1.querySelector(`tr:first-of-type > *:nth-of-type(${fixed_columns + 1})`); + if (firstVisibleTd) { + const r1c1FragmentBounds = r1c1.getBoundingClientRect(); + const firstTdBounds = firstVisibleTd.getBoundingClientRect(); + + const width = firstTdBounds.left - r1c1FragmentBounds.left; + r0c1.style.marginLeft = `-${width}px`; + r0c1.style.marginRight = `${width}px`; + r1c1.style.marginLeft = `-${width}px`; + r1c1.style.marginRight = `${width}px`; + } - this.stylesheet.setRule('.dash-fixed-column tr', `height: ${getComputedStyle(contentTr).height};`); + it++; + } while ( + Math.abs(currentWidth - lastWidth) > WIDTH_EPSILON || + it < MAX_WIDTH_ITERATIONS + ) + } + + if (fixed_columns || fixed_rows) { + const r1c0CellWidths = Array.from( + r1c0.querySelectorAll('table.cell-table > tbody > tr:first-of-type > *') + ).map(c => c.getBoundingClientRect().width); + + const r1c1CellWidths = Array.from( + r1c1.querySelectorAll('table.cell-table > tbody > tr:first-of-type > *') + ).map(c => c.getBoundingClientRect().width); + + Array.from( + r0c0.querySelectorAll('table.cell-table > tbody > tr:first-of-type > *') + ).forEach((c, i) => this.setCellWidth(c, r1c0CellWidths[i])); + + Array.from( + r0c0.querySelectorAll('table.cell-table > tbody > tr:last-of-type > *') + ).forEach((c, i) => this.setCellWidth(c, r1c0CellWidths[i])); + + Array.from( + r0c1.querySelectorAll('table.cell-table > tbody > tr:first-of-type > *') + ).forEach((c, i) => this.setCellWidth(c, r1c1CellWidths[i])); + + Array.from( + r0c1.querySelectorAll('table.cell-table > tbody > tr:last-of-type > *') + ).forEach((c, i) => this.setCellWidth(c, r1c1CellWidths[i])); } } @@ -627,87 +698,6 @@ export default class ControlledTable extends PureComponent ) || page_action === TableAction.Custom; } - applyStyle = () => { - const { - fixed_columns, - fixed_rows, - row_deletable, - row_selectable - } = this.props; - - const { r1c0, r1c1 } = this.refs as { [key: string]: HTMLElement }; - - this.updateUiViewport(); - - if (row_deletable) { - this.stylesheet.setRule( - `.dash-spreadsheet-inner td.dash-delete-cell`, - `width: 30px; max-width: 30px; min-width: 30px;` - ); - } - - if (row_selectable) { - this.stylesheet.setRule( - `.dash-spreadsheet-inner td.dash-select-cell`, - `width: 30px; max-width: 30px; min-width: 30px;` - ); - } - - // Adjust the width of the fixed row header - if (fixed_rows) { - Array.from(r1c1.querySelectorAll('tr:first-of-type td.dash-cell, tr:first-of-type th.dash-header')).forEach(td => { - const classname = td.className.split(' ')[1]; - const style = getComputedStyle(td); - const width = style.width; - - this.stylesheet.setRule( - `.dash-fixed-row:not(.dash-fixed-column) th.${classname}`, - `width: ${width} !important; min-width: ${width} !important; max-width: ${width} !important;` - ); - }); - } - - // Adjust the width of the fixed row / fixed columns header - if (fixed_columns && fixed_rows) { - Array.from(r1c0.querySelectorAll('tr:first-of-type td.dash-cell, tr:first-of-type th.dash-header')).forEach(td => { - const classname = td.className.split(' ')[1]; - const style = getComputedStyle(td); - const width = style.width; - - this.stylesheet.setRule( - `.dash-fixed-column.dash-fixed-row th.${classname}`, - `width: ${width} !important; min-width: ${width} !important; max-width: ${width} !important;` - ); - }); - } - - // Adjust widths of row deletable/row selectable headers - const subTable = fixed_rows && !fixed_columns ? r1c1 : r1c0; - - if (row_deletable) { - Array.from(subTable.querySelectorAll('tr:first-of-type td.dash-delete-cell')).forEach(td => { - const style = getComputedStyle(td); - const width = style.width; - - this.stylesheet.setRule( - 'th.dash-delete-header', - `width: ${width} !important; min-width: ${width} !important; max-width: ${width} !important;` - ); - }); - } - if (row_selectable) { - Array.from(subTable.querySelectorAll('tr:first-of-type td.dash-select-cell')).forEach(td => { - const style = getComputedStyle(td); - const width = style.width; - - this.stylesheet.setRule( - 'th.dash-select-header', - `width: ${width} !important; min-width: ${width} !important; max-width: ${width} !important;` - ); - }); - } - } - handleDropdown = () => { const { r1c1 } = this.refs as { [key: string]: HTMLElement }; @@ -715,10 +705,13 @@ export default class ControlledTable extends PureComponent } onScroll = (ev: any) => { - const { r0c1 } = this.refs as { [key: string]: HTMLElement }; + const { r0c0, r0c1 } = this.refs as { [key: string]: HTMLElement }; Logger.trace(`ControlledTable fragment scrolled to (left,top)=(${ev.target.scrollLeft},${ev.target.scrollTop})`); - r0c1.style.marginLeft = `${-ev.target.scrollLeft}px`; + + const margin = parseFloat(ev.target.scrollLeft) + parseFloat(r0c0.style.width); + + r0c1.style.marginLeft = `${-margin}px`; this.updateUiViewport(); this.handleDropdown(); @@ -952,6 +945,25 @@ export default class ControlledTable extends PureComponent } } + private setCellWidth(cell: HTMLElement, width: number) { + cell.style.width = `${width}px`; + cell.style.minWidth = `${width}px`; + cell.style.maxWidth = `${width}px`; + cell.style.boxSizing = 'border-box'; + + /** + * Some browsers handle `th` and `td` size inconsistently. + * Checking the size delta and adjusting for it (different handling of padding and borders) + * allows the table to make sure all sections are correctly aligned. + */ + const delta = cell.getBoundingClientRect().width - width; + if (delta) { + cell.style.width = `${width - delta}px`; + cell.style.minWidth = `${width - delta}px`; + cell.style.maxWidth = `${width - delta}px`; + } + } + private get showToggleColumns(): boolean { const { columns, diff --git a/packages/dash-table/src/dash-table/components/Filter/Column.tsx b/packages/dash-table/src/dash-table/components/Filter/Column.tsx index 62084d450d..5c5bed523e 100644 --- a/packages/dash-table/src/dash-table/components/Filter/Column.tsx +++ b/packages/dash-table/src/dash-table/components/Filter/Column.tsx @@ -8,7 +8,7 @@ import TableClipboardHelper from 'dash-table/utils/TableClipboardHelper'; type SetFilter = (ev: any) => void; interface IColumnFilterProps { - classes: string; + className: string; columnId: ColumnId; isValid: boolean; setFilter: SetFilter; @@ -39,7 +39,7 @@ export default class ColumnFilter extends PureComponent diff --git a/packages/dash-table/src/dash-table/components/FilterFactory.tsx b/packages/dash-table/src/dash-table/components/FilterFactory.tsx index a56673afad..91073e2be3 100644 --- a/packages/dash-table/src/dash-table/components/FilterFactory.tsx +++ b/packages/dash-table/src/dash-table/components/FilterFactory.tsx @@ -50,7 +50,7 @@ export default class FilterFactory { return (