[Table] TruncatedFormat popover improvements and performance tuning for dev preview #1624

themadcreator · 2017-09-26T23:04:36Z

~~Switches <TrunctedFormat> to measure text with a canvas element, which does not trigger a relayout.~~

Also adding switches to the dev preview to demonstrate how the table performs with:

Slow native layout
An isolated layout boundary

Uses off-DOM canvas text measuring to avoid forced reflows. SlowLayout component allows us to highlight potential perf issues in the dev preview.

themadcreator · 2017-09-26T23:11:06Z

packages/table/src/cell/formats/truncatedFormat.tsx


    private contentDiv: HTMLDivElement;
+    private cachedFontString: string;


we cache the font string on first use

this will be out of date if the style changes, but async font loading will not be an issue since the fontstring will still apply once the font is loaded.

I'm fine moving forward with this and letting someone file a ticket if out-of-date styles become an issue during the life of the page.

themadcreator · 2017-09-26T23:13:14Z

packages/table/src/cell/formats/truncatedFormat.tsx

+            );
+        } else {
+            return (
+                <span className={Classes.TABLE_TRUNCATED_POPOVER_TARGET} onClick={this.handlePopoverOpen}>


this reproduces <Popover>'s internal structure without calling any layout-triggering listeners

👏, but can we add a comment in Popover—or somewhere—indicating that this assumption-ridden code is here?

cmslewis

Woo woo! Left some stuff.

cmslewis · 2017-09-26T23:56:48Z

packages/table/preview/index.html

@@ -40,6 +63,7 @@
      flex: 1;
      order: 2;
      z-index: 2;
+      width: 100%;


Reasoning for this rule? Is it because of:

To be a layout boundary, the element must...Not have an implicit or auto width value

?

cmslewis · 2017-09-26T23:56:51Z

packages/table/preview/index.html

@@ -52,6 +76,7 @@
    .sidebar {
      background-color: #EBF1F5;
      flex-basis: 300px;
+      flex-shrink: 0;


And this rule?

i had some trouble getting the preview layout to work with the slowlayoutstack. i'd like to revisit these styles and solidify how we're going to recommend creating layout boundaries.

it might even be worth adding a <LayoutBoundary> react component that uses style attributes (to avoid any class overrides from breaking them)

cmslewis · 2017-09-26T23:57:54Z

packages/table/preview/mutableTable.tsx

@@ -34,6 +34,7 @@ import { RenderMode } from "../src/common/renderMode";
 import { IRegion } from "../src/regions";
 import { DenseGridMutableStore } from "./denseGridMutableStore";
 import { LocalStore } from "./localStore";
+import { SlowLayoutStack } from "./slowLayout";


would prefer to rename to ./slowLayoutStack to make the export match the filename

cmslewis · 2017-09-27T00:02:08Z

packages/table/src/cell/formats/truncatedFormat.tsx

 }

+// const CELL_FONT_PROPERTIES_STRING = `normal normal normal normal 12px / 20px -apple-system, system-ui, "Segoe UI", Roboto, Oxygen, Ubuntu, Cantarell, "Open Sans", "Helvetica Neue", Icons16, sans-serif`;


delete commented code

cmslewis · 2017-09-27T00:03:54Z

packages/table/src/cell/formats/truncatedFormat.tsx


    private contentDiv: HTMLDivElement;
+    private cachedFontString: string;


I'm fine moving forward with this and letting someone file a ticket if out-of-date styles become an issue during the life of the page.

cmslewis · 2017-09-27T00:04:51Z

packages/table/src/cell/formats/truncatedFormat.tsx

+        // <Popover> will always check the content's position on update
+        // regardless if it is open or not. This negatively affects perf due to
+        // layout thrashing. So instead we manage the popover state ourselves
+        // and mimic its popover target


eww, but so clever

really, this should just be done by popover. the dom measuring should not be done on update, but rather on interaction

cmslewis · 2017-09-27T00:05:53Z

packages/table/src/cell/formats/truncatedFormat.tsx

+            );
+        } else {
+            return (
+                <span className={Classes.TABLE_TRUNCATED_POPOVER_TARGET} onClick={this.handlePopoverOpen}>


👏, but can we add a comment in Popover—or somewhere—indicating that this assumption-ridden code is here?

cmslewis · 2017-09-27T00:10:10Z

packages/table/src/cell/formats/truncatedFormat.tsx

+        const containerWidth = parseInt(this.props.parentCellWidth, 10);
+        const availableWidth = containerWidth - CONTAINER_PADDING;
+        const isTruncated = contentWidth > availableWidth;
+        this.setState({ isTruncated } as ITruncatedFormatState);


// summoning @gscshoyru for thorough review of this new code

// appears in a puff of smoke, surrounded by chanting and candles

...um. Where'd height go? Why are we only measuring the width of the text itself? This doesn't actually work for any cases where we've got work wrap turned on. Did you test with that?

so, uh, the only thing you can measure with canvas is width because that's all that exists on TextMetrics.

the text measurement assumes no wrapping in the text and is only useful at detecting horizontal overflow.

gscshoyru

...this removes any and all measuring considerations around height. Which means this is horribly broken for any cases where we have text wrapping turned on.

gscshoyru · 2017-09-27T14:45:05Z

packages/table/preview/mutableTable.tsx

-                    renderRowHeader={this.renderRowHeader}
-                    selectionModes={this.getEnabledSelectionModes()}
-                    styledRegionGroups={this.getStyledRegionGroups()}
+                <SlowLayoutStack


..not that it really matters, and maybe I'm misunderstanding something -- but would it have been better to split the changes to truncated format and the changes to the preview into separate commits?

yeah, and might still need to do that, but i'm using this slowlayoutstack to test my changes to truncatedformat.

gscshoyru · 2017-09-27T14:45:45Z

packages/table/src/cell/formats/truncatedFormat.tsx

+        const containerWidth = parseInt(this.props.parentCellWidth, 10);
+        const availableWidth = containerWidth - CONTAINER_PADDING;
+        const isTruncated = contentWidth > availableWidth;
+        this.setState({ isTruncated } as ITruncatedFormatState);


// appears in a puff of smoke, surrounded by chanting and candles

gscshoyru · 2017-09-27T15:00:31Z

packages/table/src/cell/formats/truncatedFormat.tsx

+        const containerWidth = parseInt(this.props.parentCellWidth, 10);
+        const availableWidth = containerWidth - CONTAINER_PADDING;
+        const isTruncated = contentWidth > availableWidth;
+        this.setState({ isTruncated } as ITruncatedFormatState);


...um. Where'd height go? Why are we only measuring the width of the text itself? This doesn't actually work for any cases where we've got work wrap turned on. Did you test with that?

gscshoyru · 2017-09-27T15:01:18Z

packages/table/src/cell/formats/truncatedFormat.tsx

+// Since we measure only the `textContent` of the cell to determine the
+// truncation state, we must account for the padding that is applied via CSS to
+// the cell.
+const CONTAINER_PADDING = 20;


Why the change?

ping @themadcreator

Previously we had to consider the current truncation state when doing the calculation for content width because the element containing the text would have a different size if the "..." button was shown.

Now, since we're measuring with canvas, the size of the text content element is irrelevant. Instead we just need to consider the amount of padding between the parent element and the text content element.

…wrap to preview

blueprint-bot · 2017-09-27T20:35:31Z

fix tests for real this time

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-09-27T20:50:55Z

@gscshoyru @themadcreator @llorca

RE: not considering parent-cell height anymore:

It looks like the only side effect of that omission is that we'll occasionally show the ··· icon more often than we need to (e.g. if a long block of text fits in a cell with wrapping enabled but wouldn't with wrapping disabled). I'm okay with that slight goofiness given the huge perf benefits of this PR.

cmslewis

@themadcreator looks okay overall, but not approving yet just to make sure we have a clear story on https://github.com/palantir/blueprint/pull/1624/files#diff-820e3fef53ee313553edff37ddbf4737R19 first.

cmslewis · 2017-09-27T20:53:25Z

packages/table/preview/mutableTable.tsx

    enableMultiSelection: true,
    enableRowReordering: false,
    enableRowResizing: false,
    enableRowSelection: true,
+    enableSlowLayout: false,


@llorca FWIW the state-key names in this example have been a great testing ground for the ideas in #1625. Notice that we have both enable... and show... happening here. We really just need the former I think.

(No action required; just FYI)

cmslewis · 2017-09-27T20:59:37Z

packages/table/src/cell/formats/truncatedFormat.tsx

+// Since we measure only the `textContent` of the cell to determine the
+// truncation state, we must account for the padding that is applied via CSS to
+// the cell.
+const CONTAINER_PADDING = 20;


ping @themadcreator

blueprint-bot · 2017-09-28T20:12:30Z

Reverting canvas measuring. Keeping dev preview updates

Preview: documentation | landing | table
Coverage: core | datetime

themadcreator · 2017-09-28T22:23:38Z

some test in core is failing:

  Toaster
    with autoFocus set to true
      ✖ focuses on newly created toast

blueprint-bot · 2017-09-28T22:53:17Z

coverage

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-09-29T18:49:30Z

packages/table/preview/mutableTable.tsx

+function getRandomObject(propCount: number, depth = 0): object {
+    const childPropCount = propCount;
+    const obj: any = {};
+    while (propCount-- >= 0) {


nit: I prefer to avoid the -- and ++ operators, because they're so sneaky and tend to make lines denser.

cmslewis · 2017-09-29T18:54:20Z

packages/table/src/common/utils.ts

+     *
+     * Returns a `TextMetrics` object.
+     */
+    measureElementTextContent(element: Element, fontProperties?: string | ICssFontProperties) {


Can we revert all these utility changes?

blueprint-bot · 2017-09-29T19:35:37Z

revert utils and test changes. use for loop

Preview: documentation | table
Coverage: core | datetime

cmslewis

LGTM! I updated the PR title to help with future bookkeeping.

Reverted the TruncatedFormat measurement changes

themadcreator added 3 commits September 26, 2017 15:25

Add "SlowLayout". Make TruncatedFormat fast.

897b81a

Uses off-DOM canvas text measuring to avoid forced reflows. SlowLayout component allows us to highlight potential perf issues in the dev preview.

Add layout boundary isolation

f659039

Merge remote-tracking branch 'origin/master' into bd/table-perf-12

6874382

themadcreator commented Sep 26, 2017

View reviewed changes

cmslewis reviewed Sep 27, 2017

View reviewed changes

cmslewis changed the title ~~[Table] TrunactedFormat perf~~ [Table] Improve TruncatedFormat performance Sep 27, 2017

gscshoyru previously requested changes Sep 27, 2017

View reviewed changes

themadcreator added 5 commits September 27, 2017 11:50

PR comments. Fix tests

9bc1f31

Merge remote-tracking branch 'origin/master' into bd/table-perf-12

c7e4ffd

Use shorter string in test since linux has different fonts. Add text …

c1a248f

…wrap to preview

Remove it.only

a8a9968

fix tests for real this time

52b7326

cmslewis reviewed Sep 27, 2017

View reviewed changes

Reverting canvas measuring. Keeping dev preview updates

915085a

themadcreator changed the title ~~[Table] Improve TruncatedFormat performance~~ [Table] Performance tuning for dev preview Sep 28, 2017

Merge remote-tracking branch 'origin/master' into bd/table-perf-12

d6ee689

themadcreator added 3 commits September 28, 2017 14:00

new lint rules

c2ef73e

more lint

fbd0394

coverage

97377dd

cmslewis reviewed Sep 29, 2017

View reviewed changes

revert utils and test changes. use for loop

c7d70db

cmslewis changed the title ~~[Table] Performance tuning for dev preview~~ [Table] TruncatedFormat popover improvements and performance tuning for dev preview Sep 29, 2017

cmslewis approved these changes Sep 29, 2017

View reviewed changes

themadcreator merged commit 78cb34e into master Sep 29, 2017

themadcreator deleted the bd/table-perf-12 branch September 29, 2017 19:44

cmslewis mentioned this pull request Sep 29, 2017

Prepare Release v1.30.0 #1642

Merged


		private contentDiv: HTMLDivElement;
		private cachedFontString: string;

		}

		// const CELL_FONT_PROPERTIES_STRING = `normal normal normal normal 12px / 20px -apple-system, system-ui, "Segoe UI", Roboto, Oxygen, Ubuntu, Cantarell, "Open Sans", "Helvetica Neue", Icons16, sans-serif`;

[Table] TruncatedFormat popover improvements and performance tuning for dev preview #1624

[Table] TruncatedFormat popover improvements and performance tuning for dev preview #1624

Conversation

themadcreator commented Sep 26, 2017 • edited by cmslewis 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

cmslewis left a 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

gscshoyru left a 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

blueprint-bot commented Sep 27, 2017

fix tests for real this time

cmslewis commented Sep 27, 2017 • edited Loading

cmslewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 28, 2017

Reverting canvas measuring. Keeping dev preview updates

themadcreator commented Sep 28, 2017

blueprint-bot commented Sep 28, 2017

coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Sep 29, 2017

revert utils and test changes. use for loop

cmslewis left a comment

Choose a reason for hiding this comment

themadcreator commented Sep 26, 2017 •

edited by cmslewis

Loading

cmslewis commented Sep 27, 2017 •

edited

Loading