Issue 777 - Align column headers when using fixed rows #793

Marc-Andre-Rivet · 2020-06-11T15:23:48Z

Fixes #777
Fixes #780

Under certain circumstances, the table fragments were misaligned for scenarios involving fixed_rows and fixed_columns. Also, some visual tests were already showing that the behavior was incorrect but it was hard to spot unless zooming in. For that reason I believe the tests already appropriately cover the cases for this PR and that no additional tests are required, only more caution.

A few problems were encountered:

Certain browsers require both the tr and the th rows to have their size explictly set when forcing them through styles. This happens for fixed row/columns using the data nested field. For example, in Firefox, forcing a tr to have a width of 200px can be overriden by a previous th requiring more space. The th "wins". The fix for this is to both set the tr and th cell size for the last row of each group.
The implementation was subtly wrong, using the size of the wrong fragment to coerce another. I think some of those is due to historical reasons, and were not reworked correctly when the table was reworked to allow variable row heights in Issue 649 - Fix mismatched row height for fixed columns #722.
Certain browsers, for certain tables will not be able to converge on a single table width, but will instead oscillate between two very close values. As far as I know always distant by 1px. The "iterations" code for the table now checks for cycles.

Also, a nice side-effect of this PR is that tables with fixed columns now behave more consistently vs. unfixed ones. Will need to check the documentation for any mention of incorrect width that will have to be removed.

now looks like this fixed and unfixed:

…eaders

…ually

- apply style to both the last row and the last `th` row

Marc-Andre-Rivet · 2020-06-15T21:03:56Z

src/dash-table/components/ControlledTable/index.tsx

+        // For  some reason, some browsers require the size to be set explicitly on the header cells too
+        Array.from<HTMLElement>(
+            lastTrOfThs.children as any
+        ).forEach((c, i) => this.setCellWidth(c, widths[i]));


Firefox requires th cells to be fixed alongside tr cells for the table to behave consistently.

Marc-Andre-Rivet · 2020-06-16T13:22:11Z

package.json

@@ -33,7 +33,7 @@
    "private::test.python": "python -m unittest tests/unit/format_test.py",
    "private::test.unit": "cypress run --browser chrome --spec 'tests/cypress/tests/unit/**/*'",
    "private::test.standalone": "cypress run --browser chrome --spec 'tests/cypress/tests/standalone/**/*'",
-    "build.watch": "webpack-dev-server --content-base dash_table --mode development --config webpack.dev.config.js",
+    "build.watch": "webpack-dev-server --disable-host-check --content-base dash_table --mode development --config webpack.dev.config.js",


In Browserstack, when running on iOS devices, localhost is redirected to bs-local which webpack-dev-server doesn't like, this flag, as its name indicates, makes it not care

Marc-Andre-Rivet · 2020-06-16T13:23:15Z

src/dash-table/components/ControlledTable/index.tsx

-                    r0c1.style.marginLeft = `-${width}px`;
-                    r0c1.style.marginRight = `${width}px`;
-                    r1c1.style.marginLeft = `-${width}px`;
-                    r1c1.style.marginRight = `${width}px`;


marginRight caused the table to be smaller than expected with fixed columns. Removing it for both r0c1 and r1c1 fragments ensures the table is the expected size

Marc-Andre-Rivet · 2020-06-16T13:24:11Z

src/dash-table/components/ControlledTable/index.tsx

+            // If the table was resized and isn't in a cycle, re-run `handleResize`.
+            // If the final size is the same as the starting size from the previous iteration, do not
+            // resize the main table, instead just use as is, otherwise it will oscillate.
+            if (nextWidth !== currentWidth) {


Tested these for scenarios involving decimal places and is seems to behave fine on all important targets, decimals or not. There doesn't seem to be a need for an epsilon.

- remove `forcedResizeOnly` state prop - add sizing test

Marc-Andre-Rivet · 2020-06-16T21:52:14Z

src/dash-table/components/ControlledTable/index.tsx

+            style_header,
+            style_header_conditional,
+            style_table
+        );


Only resize through React pipeline if a style prop has changed (memoized: https://github.com/plotly/dash-table/pull/793/files#diff-a6637ea69a08d602c311a574b4e4979aR324). This behaves similarly to the 🔪 forcedResizedOnly state prop, but also allows style changes to trigger recalculation. For example display:none;, changes in column widths, etc.

Marc-Andre-Rivet · 2020-06-16T21:53:44Z

src/dash-table/components/ControlledTable/index.tsx


-            tr.style.height = getComputedStyle(tr2).height;
-        });
+        this.handleResize();


If executing this, the styles have changed, the above clears up all forced width before handling the resize, allowing for the table to resize itself correctly.

Marc-Andre-Rivet · 2020-06-16T21:54:48Z

src/dash-table/components/ControlledTable/index.tsx

+        // Not pixels -> not displayed
+        if (!/\d+px/.test(currentTableWidth)) {
+            return;
+        }


If not px, the table is not rendered, don't resize it. Lucky plotly/plotly.js#4925 happened at the same time, otherwise it would have gone unnoticed.

OIC, '100.1px' will in fact pass this test as it's not a whole string match. Worried me at first but it should work.

Marc-Andre-Rivet · 2020-06-16T21:56:23Z

tests/selenium/test_sizing.py

+
+def cells_are_same_width(target, table):
+    wait.until(lambda: abs(target.size["width"] - table.size["width"]) <= 1, 3)
+


Helper function that checks if two tables that are expected to have the same size / cell sizes are identical (or nearly identical) in size

This routine is testing the last row of cells; while we're here would it be worthwhile checking that all the rows of a single table are the same, ie rows aren't ragged? That's certainly been a problem in the past...

Jagged rows happen when we transition from one fragment to another of the DataTable. Checking the cells of any row of a fragment against the cells of any row of the .cell-1-1 fragment should be sufficient if the table doesn't have a fixed portion.

What I realize from this comment is that for szng002 I should also be testing the target against itself.

Marc-Andre-Rivet · 2020-06-16T21:57:28Z

tests/selenium/test_sizing.py

+            fixed_columns=dict(headers=True, data=1),
+            fixed_rows=dict(headers=True, data=1),
+        ),
+    ]


Create all combinations of fixed columns/rows tables and hide them.

Marc-Andre-Rivet · 2020-06-16T21:58:02Z

tests/selenium/test_sizing.py

+                style_table=dict(
+                    width=750, minWidth=750, maxWidth=750, paddingBottom=10
+                )
+            ),


These tables are used to make sure the test tables are behaving as expected when updated through the callback below

Marc-Andre-Rivet · 2020-06-16T21:58:26Z

tests/selenium/test_sizing.py

+            return [
+                dict(width=350, minWidth=350, maxWidth=350, paddingBottom=10)
+                for i in variations_range
+            ]


Change the table style to make them visible / hidden and/or resized

Beautiful test :)
Looks like you started to collect all these style_table dicts up above in styles then changed tacks and put them inline down here... could probably be simplified a little, also we don't really need n clones, could just be return [dict(...)] * variations_range... but 🤷 not important for a test, the functionality is great!

The test did change a lot between this version and the first implementation. Cleaned it up. 575aa2c

Marc-Andre-Rivet · 2020-06-16T21:59:57Z

tests/selenium/test_sizing.py

+
+    target = test.driver.find_element_by_css_selector("#table0")
+
+    for i in range(1, len(variations)):


Test various configurations of fixed columns/rows and merge_duplicate_headers to make sure they all render the same with % based column widths.

Marc-Andre-Rivet · 2020-06-16T22:01:29Z

src/dash-table/components/ControlledTable/index.tsx

+
+            ths.forEach(this.clearCellWidth);
+        });
+    }


Force all th rows with all cells to match the expected width.

src/dash-table/components/ControlledTable/index.tsx

alexcjohnson · 2020-06-17T03:33:22Z

src/dash-table/components/ControlledTable/index.tsx

-            const tr = th.parentElement as HTMLElement;
+        this.resetFragmentCells(r0c0);
+        this.resetFragmentCells(r0c1);
+        this.resetFragmentCells(r1c0);


Minor - skip reset for a table that doesn't exist? Also seems like this could be 🌴

[r0c0, r0c1, r1c0].forEach(rc=> { rcTable=...; if (rcTable) { rcTable.style.width = ''; this.resetFragmentCells(rc); } });

575aa2c#diff-a6637ea69a08d602c311a574b4e4979aR340

alexcjohnson · 2020-06-17T03:57:35Z

tests/selenium/test_sizing.py

+
+    variations_range = range(0, len(variations))
+
+    tables = [DataTable(**variation) for i, variation in enumerate(variations)]


Suggested change

tables = [DataTable(**variation) for i, variation in enumerate(variations)]

tables = [DataTable(**variation) for variation in variations]

4dcc753 and 575aa2c#diff-cd5fbde6c058807ca1b16f17f7daa4d8R110

alexcjohnson · 2020-06-17T04:19:30Z

tests/selenium/test_sizing.py

+                "name": [
+                    "A-----------------VERY LONG",
+                    "B-----------------VERY LONG",
+                    "C-----------------VERY LONG",


Would it make sense to have the last row of names all be different lengths, so we know it's not just the content match giving rise to the width match?

575aa2c#diff-cd5fbde6c058807ca1b16f17f7daa4d8R199

alexcjohnson · 2020-06-17T04:22:24Z

tests/selenium/test_sizing.py

+            {"_": 0, "a": 809, "b": 591, "c": 511},
+        ],
+        style_table=dict(width=500, minWidth=500, maxWidth=500, paddingBottom=10),
+        style_cell=dict(width="20%", minWidth="20%", maxWidth="20%"),


I only see 4 columns - is it intentional to have <100% total width?

575aa2c#diff-cd5fbde6c058807ca1b16f17f7daa4d8R213

Marc-Andre-Rivet · 2020-06-17T13:30:56Z

@alexcjohnson Addressed all previous comments. Also added b02a022 to fix #780 and lock behavior with new tests.

alexcjohnson

Fantastic!💃

Marc-André Rivet and others added 2 commits June 11, 2020 11:18

only use width adjustments if the table has fixed columns

54d7af0

Merge branch 'dev' into 777-headers

e4f6755

rpkyle changed the title ~~[WIP] Issue 777~~ [WIP] Align column headers when using fixed rows Jun 11, 2020

Marc-André Rivet added 4 commits June 11, 2020 15:24

make sure r0c1 table is the same size as r1c1 table

7fde333

Merge branch '777-headers' of github.com:plotly/dash-table into 777-h…

d041141

…eaders

add back width fix

dd78147

resize + adjust when all columns have been resized instead of individ…

4294d5b

…ually

Marc-Andre-Rivet changed the title ~~[WIP] Align column headers when using fixed rows~~ [WIP] Issue 777 - Align column headers when using fixed rows Jun 11, 2020

Marc-André Rivet added 20 commits June 11, 2020 16:36

noise

ff92e93

last row instead of first

cb09457

fix all fragment tables width and force refresh on mount

d773e4f

apply r1c1 fragment sizes to all other fragments

95d9f99

Simplify resizing

9eda74e

use % instead of px for !Chrome

2bf938a

- refactor

09208c7

- apply style to both the last row and the last `th` row

fix typing issue

f7bfefb

px on last tr + px on last tr of ths

bf31a03

noise

cffa5eb

limit handleResize max iterations

47a3573

resize with epsilon

fc95bc7

epsilon and 2 steps loop

2c11c5c

.

df9be45

what is percy running?!

7fd0a61

smaller epsilon?

6f3a78d

remove version information

4ffbb86

fixed if oscillating

322e3b7

fixed

dfbb4db

noise

482ae55

Marc-Andre-Rivet commented Jun 15, 2020

View reviewed changes

fixed -> cycle

d91466b

Marc-Andre-Rivet commented Jun 16, 2020

View reviewed changes

Marc-André Rivet added 2 commits June 16, 2020 16:28

- handle display: none case

350703a

- remove `forcedResizeOnly` state prop - add sizing test

improve support for merged headers & percentage based columns

4f5047e

Marc-Andre-Rivet commented Jun 16, 2020

View reviewed changes

alexcjohnson reviewed Jun 17, 2020

View reviewed changes

src/dash-table/components/ControlledTable/index.tsx Show resolved Hide resolved

alexcjohnson reviewed Jun 17, 2020

View reviewed changes

Marc-André Rivet added 3 commits June 17, 2020 06:39

clean up tests & dry up code

575aa2c

remove enumerate

4dcc753

fix and test scrolling in fixed rows/columns tables

b02a022

alexcjohnson approved these changes Jun 17, 2020

View reviewed changes

Marc-Andre-Rivet merged commit bee4a4f into dev Jun 17, 2020

Marc-Andre-Rivet deleted the 777-headers branch June 17, 2020 15:00

rpkyle mentioned this pull request Jun 17, 2020

Dash for R v0.6.0 plotly/dashR#210

Merged

This was referenced Jun 17, 2020

fixed_rows isn't working with fixed column widths #775

Closed

Takes several seconds for fixed headers to resize to column widths after mouse up #776

Closed

Marc-Andre-Rivet mentioned this pull request Jun 19, 2020

Misaligned table rows with fixed_row / fixed_column after callback #797

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 777 - Align column headers when using fixed rows #793

Issue 777 - Align column headers when using fixed rows #793

Marc-Andre-Rivet commented Jun 11, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 15, 2020

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 16, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

Marc-Andre-Rivet Jun 16, 2020

Marc-Andre-Rivet Jun 16, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

alexcjohnson Jun 17, 2020

Marc-Andre-Rivet Jun 17, 2020

Marc-Andre-Rivet commented Jun 17, 2020

alexcjohnson left a comment


		def cells_are_same_width(target, table):
		wait.until(lambda: abs(target.size["width"] - table.size["width"]) <= 1, 3)


		target = test.driver.find_element_by_css_selector("#table0")

		for i in range(1, len(variations)):


		variations_range = range(0, len(variations))

		tables = [DataTable(**variation) for i, variation in enumerate(variations)]

	tables = [DataTable(**variation) for i, variation in enumerate(variations)]
	tables = [DataTable(**variation) for variation in variations]

Issue 777 - Align column headers when using fixed rows #793

Issue 777 - Align column headers when using fixed rows #793

Conversation

Marc-Andre-Rivet commented Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marc-Andre-Rivet Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Marc-Andre-Rivet Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marc-Andre-Rivet Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marc-Andre-Rivet commented Jun 17, 2020

alexcjohnson left a comment

Choose a reason for hiding this comment

Marc-Andre-Rivet commented Jun 11, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading

Marc-Andre-Rivet Jun 16, 2020 •

edited

Loading