[Table] Deselect cells on cmd+click only #1665

cmslewis · 2017-10-05T07:28:24Z

Fixes #1073, Addresses #1079

Checklist

Include tests

Changes proposed in this pull request:

A morning surprise for @llorca.

🔄 CHANGED Table no longer toggles selection on click; now, you must cmd + click to deselect cells, columns, or rows.
🐛 FIXED Table ctrl + click now works on Windows again.

Reviewers should focus on:

There was an edge case for rows due to the fact that selected rows become reorderable, meaning the DragReorderable component would swallow all clicks (even cmd + clicks meant for deselecting). The fix required a refactor of the disabled? prop to optionally be a function that accepts the current MouseEvent, so it can check if the metaKey is pressed.
This isn't an issue for columns, because the reorder handle is the only trigger for reordering; the rest of the column header cell is managed by the DragSelectable layer.
This isn't an issue for body cells either, because reordering isn't even a thing there.
You can still start new drag-selections from an already-selected cell. 🎉

…ng issues

blueprint-bot · 2017-10-05T07:36:14Z

Little cleanups

Preview: documentation | table
Coverage: core | datetime

llorca

feels amazing 💯

cmslewis · 2017-10-05T23:04:30Z

packages/table/src/interactions/selectable.tsx

+     * Whether the meta key should be pressed to enable deselection on click.
+     * @default false
+     */
+    requireMetaKeyToDeselect?: boolean;


This is now true in every context (body cells and header cells). Shall I get rid of it and make the true path the default behavior?

cmslewis · 2017-10-05T23:08:48Z

packages/table/src/interactions/selectable.tsx

            ignoredSelectors.some((selector: string) => element.closest(selector) != null)
        );
    }

    // Update logic
    // ============

+    /**
+     * Returns true if the component should ignore subsequent drag-move's.
+     */
    private handleUpdateExistingSelection = (selectedRegionIndex: number, event: MouseEvent) => {


If we get rid of the requireMetaKeyToDeselect prop, I can also promote the two logic paths in this function to the top-level handleActivate function (with new, nicely named helper functions to manage each logic path). That would obviate the need for a return value here.

themadcreator · 2017-10-06T17:51:22Z

packages/table/src/headers/header.tsx

+        if (event.metaKey) {
+            // if the meta key is pressed, we want to forcefully ignore reordering
+            // interactions and prioritize drag-selection interactions (e.g. to make
+            // it possible to deselect a row).


i don't think meta is the ideal key here for all platforms. meta means cmd on mac and windows-key on windows. typically i use what the hotkeys code calls mod which is cmd on mac and ctrl on windows. i think the windows-key accesses OS resources like the start menu so it's not usually used in a key combo.

maybe holding any modifier key should disable the drag?

seems like this would likely conflict with other key combos

Fixed by using DragEvents.isAdditive(event)

themadcreator · 2017-10-06T17:52:33Z

packages/table/src/tableBody.tsx

@@ -85,6 +85,7 @@ export class TableBody extends React.Component<ITableBodyProps, {}> {
                onFocus={this.props.onFocus}
                onSelection={this.props.onSelection}
                onSelectionEnd={this.handleSelectionEnd}
+                requireMetaKeyToDeselect={true}


non-configurable? why even have a prop?

Because I added it at the beginning of this work. And then realized by the end of it that I didn't need it to be configurable. Will remove.

giladgray · 2017-10-06T18:52:50Z

packages/table/src/headers/header.tsx

-    public state: IHeaderState = {
-        hasSelectionEnded: false,
-    };
+    public state: IHeaderState;


remove this if defining in constructor.

giladgray · 2017-10-06T18:53:44Z

packages/table/src/headers/header.tsx

+        // a selection is already defined, so enable reordering interactions
+        // right away if other criteria are satisfied too.
+        this.state = {
+            hasSelectionEnded: props.selectedRegions != null && props.selectedRegions.length > 0,


does this logic need to appear in componentWillReceiveProps too?

Yep, already does.

giladgray · 2017-10-06T18:54:54Z

packages/table/src/headers/header.tsx

+        if (event.metaKey) {
+            // if the meta key is pressed, we want to forcefully ignore reordering
+            // interactions and prioritize drag-selection interactions (e.g. to make
+            // it possible to deselect a row).


maybe holding any modifier key should disable the drag?

giladgray · 2017-10-06T18:57:12Z

packages/table/src/interactions/selectable.tsx

+     * Whether the meta key should be pressed to enable deselection on click.
+     * @default false
+     */
+    requireMetaKeyToDeselect?: boolean;


giladgray

(to prevent accidental merge after recent reviews)

blueprint-bot · 2017-10-06T22:02:44Z

Use DragEvents.isAdditive, fix tests

Preview: documentation | table
Coverage: core | datetime

Addressed comments

blueprint-bot · 2017-10-10T23:22:04Z

Write tests for PlatformUtils

Preview: documentation | table
Coverage: core | datetime

cmslewis · 2017-10-10T23:40:22Z

@themadcreator @giladgray @adidahiya @gscshoyru

The situation:

On non-MAC, CTRL + Click should add disjoint selections.
On Mac, CMD + Click should add disjoint selections.
On Mac, CTRL + Click triggers the context menu and shouldn't add disjoint selections.

I tried using Hotkeys in the DragSelectable component so that I could leverage its platform detection capabilities, but given that a click is involved, I would have to add some awkward flag-toggling callbacks like:

<Hotkey
    label="???"
    combo="mod"
    onKeyDown={() => (this.isModKeyPressed = true)}
    onKeyUp={() => (this.isModKeyPressed = false)}
/>

And then do something on click like if (isLeftClick && this.isModKeyPressed).

But this has some problems:

There's a click involved, so using a Hotkey feels forced.
The Hotkey label is meaningless.
I still need to know the platform somehow: if I'm on a Mac, CTRL + click requires special handling, so I need some way to write if (isLeftClick && event.ctrlKey && isMac()).

So I need some kind of platform detection. We're already doing this via the isMac helper in hotkeyParser.ts in core, but I don't particularly want to export that function for table to reuse, because that would also enable any other Blueprint consumer to use it (and it ain't the most rigorous function in the world).

For now, I've copied the isMac code into table, written an isModKeyPressed helper too, and written/copied tests for each. Everything now works as expected on both Mac and Windows.

^ Thoughts on this approach? I'd rather not copy code between packages, but nothing else seemed workable.

cmslewis · 2017-10-10T23:43:14Z

packages/table/test/common/internal/platformUtilsTests.ts

+
+            it("returns true if CTRL key pressed", () => {
+                const fakeEvent: any = { metaKey: false, ctrlKey: true };
+                expect(PlatformUtils.isModKeyPressed(fakeEvent, PLATFORM)).to.be.true;


You can't do const isMac = sinon.stub(PlatformUtils, "isMac") and expect the tests to pass, because under the hood, isModKeyPressed invokes isMac as its own free-floating function, not as a child of some PlatformUtils construct.

Thus, a platformOverride parameter.

that seems wrong... you should be able to stub the function at the module where it's exported (but not at some other module which re-exports the symbol). did you try stubbing it with a specific implementation (specify its return value)?

I'd love to be wrong, but it wouldn't work with my normal strategies. I tried stubbing the return value within the describe, but that actually stubs the isMac function before the describe("isMac") block runs, which causes all those tests to fail. When I did the stubbing in a before or in a beforeEach, it didn't work. Maybe I was just tired and missing something yesterday - I'll try some other stuff right now.

Yeah, this test fails when run on a Mac; isModKeyPressed still invokes the real isMac function in its same file:

import * as PlatformUtils from "../../../src/common/internal/platformUtils"; it("returns true if CTRL key pressed on a non-Mac", () => { const isMac = sinon.stub(PlatformUtils, "isMac").returns(false); const fakeEvent: any = { metaKey: false, ctrlKey: true }; expect(PlatformUtils.isModKeyPressed(fakeEvent)).to.be.true; isMac.restore(); });

what if the signature was isModKeyPressed(event, isMac = isMac) (callback param, defaults to real impl in the file)?
then you could simply pass a fake isMac impl () => false instead of trying to stub some module export.
also then your actual isMac impl wouldn't need to support platformOverride.

How would I test the logic in the real isMac without a platformOverride param? I'd like to just stub navigator.platform, but you can't because it's not a function.

giladgray · 2017-10-11T19:35:01Z

packages/table/src/headers/header.tsx

-        } else {
-            this.setState({ hasSelectionEnded: false });
-        }
+        this.setState({ hasSelectionEnded: this.isSelectedRegionsControlledAndNonEmpty(nextProps) });


⚠️ this will always update the state when props change; was that your intention behind this change? or does it need to be gated with an if clause?

Yep, that's intended. For row headers, hasSelectionEnded needs to be true before reordering is allowed. When you pass in selectedRegions in a controlled fashion, we want the flag to be true from the get-go so that reordering is immediately enabled.

maybe we should rename this since the boolean does not indicate anything about the selection "ending", but rather if it is a valid complete selection.

maybe hasValidSelection or hasReorderableSelection

Changed to hasValidSelection.

themadcreator

just some nits

themadcreator · 2017-10-12T18:55:42Z

packages/table/src/headers/header.tsx

-        } else {
-            this.setState({ hasSelectionEnded: false });
-        }
+        this.setState({ hasSelectionEnded: this.isSelectedRegionsControlledAndNonEmpty(nextProps) });


maybe we should rename this since the boolean does not indicate anything about the selection "ending", but rather if it is a valid complete selection.

maybe hasValidSelection or hasReorderableSelection

themadcreator · 2017-10-12T18:57:37Z

packages/table/src/headers/header.tsx

+    private wrapInDragReorderable(
+        index: number,
+        children: JSX.Element,
+        disabled: boolean | ((event: MouseEvent) => boolean) = false,


nit:

this method will only ever be called with the function type right?

also, it's another private method, so you could just omit this param and use this.isDragReorderableDisabled directly (though maybe i missed something hidden in folded code)

Actually this function is used twice: in one case, it falls back to the default false value, in the other, shown in this diff, the invoking function passes this.isDragReorderableDisabled directly.

For clarity, I'll remove the default in the first case and passed in the false value explicitly.

themadcreator · 2017-10-12T19:01:38Z

packages/table/src/interactions/selectable.tsx

        const element = event.target as HTMLElement;

        const isLeftClick = Utils.isLeftClick(event);
-        const isContextMenuTrigger = isLeftClick && event.ctrlKey;
+        const isContextMenuTrigger = isLeftClick && event.ctrlKey && PlatformUtils.isMac();
+        const isDisabled = CoreUtils.isFunction(disabled) ? CoreUtils.safeInvoke(disabled, event) : disabled;


nit: seems like a good opportunity to DRY. maybe a templatized CoreUtils method that resolves a constant or functional result and has a default constant.

Added CoreUtils.safeInvokeOrValue. 👍

blueprint-bot · 2017-10-13T18:52:45Z

Add CoreUtils.safeInvokeOrValue

Preview: documentation | table
Coverage: core | datetime

gscshoyru · 2017-10-13T19:08:14Z

packages/table/src/common/internal/platformUtils.ts

+ * Returns `true` if (1) the platform is Mac and the keypress includes the `cmd`
+ * key, or (2) the platform is non-Mac and the keypress includes the `ctrl` key.
+ */
+export const isModKeyPressed = (event: KeyboardEvent, platformOverride?: string) => {


...there's no way to do this via feature discovery, huh? Drat.

gscshoyru · 2017-10-13T19:08:49Z

packages/table/src/headers/header.tsx

+
+    private isDragSelectableDisabled = (event: MouseEvent) => {
+        if (DragEvents.isAdditive(event)) {
+            // if the meta/crtl key was pressed, we want to forcefully ignore


Nitpick: ctrl, not crtl :P

blueprint-bot · 2017-10-13T19:58:52Z

Nit: 'crtl' => 'ctrl'

Preview: documentation | table
Coverage: core | datetime

cmslewis added 10 commits October 4, 2017 20:50

Deselect body cells on cmd+click, not click

f44ef37

Fix some edge cases in TableBody

fd1b675

Delete console.logs

bf14962

Add DragSelectable tests

b43a3b7

Define Header state in constructor, not on mount

b7647ef

Apply the same interaction to headers, disentangle selection+reorderi…

ebf73d7

…ng issues

Fix tests

5b4302a

Fix test error in columnHeaderCellTests

04c797d

Add more tests

ed58e1f

Little cleanups

0b7fe09

cmslewis added Package: table Type: enhancement labels Oct 5, 2017

cmslewis requested review from llorca, themadcreator, gscshoyru and tgreenwatts October 5, 2017 07:28

llorca approved these changes Oct 5, 2017

View reviewed changes

cmslewis commented Oct 6, 2017

View reviewed changes

themadcreator reviewed Oct 6, 2017

View reviewed changes

giladgray reviewed Oct 6, 2017

View reviewed changes

giladgray previously requested changes Oct 6, 2017

View reviewed changes

cmslewis added 4 commits October 6, 2017 14:14

Simplify logic, remove prop

d4f1728

Fix tests

199473e

Delete prop from TableBody and Header

d86df4e

Use DragEvents.isAdditive, fix tests

41910df

cmslewis added the P1 label Oct 9, 2017

cmslewis added 2 commits October 10, 2017 15:29

Try to fix broken windows ctrl key

cbb5030

Write tests for PlatformUtils

310cb20

cmslewis commented Oct 10, 2017

View reviewed changes

giladgray reviewed Oct 11, 2017

View reviewed changes

themadcreator approved these changes Oct 12, 2017

View reviewed changes

cmslewis added 2 commits October 13, 2017 11:44

Code cleanups in header.tsx

1299bb7

Add CoreUtils.safeInvokeOrValue

ac156e2

gscshoyru approved these changes Oct 13, 2017

View reviewed changes

Nit: 'crtl' => 'ctrl'

fc24472

cmslewis merged commit cf3e807 into master Oct 13, 2017

cmslewis deleted the cl/table-1079-cmd-click-deselect branch October 13, 2017 20:25

cmslewis mentioned this pull request Oct 16, 2017

Prepare Release 1.32.0 #1723

Merged

alexstotsky mentioned this pull request Jul 28, 2023

(improvements) Tables usability bitfinexcom/bfx-report-ui#682

Merged

3 tasks

[Table] Deselect cells on cmd+click only #1665

[Table] Deselect cells on cmd+click only #1665

Conversation

cmslewis commented Oct 5, 2017 • edited Loading

Fixes #1073, Addresses #1079

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

blueprint-bot commented Oct 5, 2017

Little cleanups

llorca 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

giladgray left a comment

Choose a reason for hiding this comment

blueprint-bot commented Oct 6, 2017

Use DragEvents.isAdditive, fix tests

blueprint-bot commented Oct 10, 2017

Write tests for PlatformUtils

cmslewis commented Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

cmslewis Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

giladgray Oct 11, 2017 • 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

themadcreator 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

blueprint-bot commented Oct 13, 2017

Add CoreUtils.safeInvokeOrValue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Oct 13, 2017

Nit: 'crtl' => 'ctrl'

cmslewis commented Oct 5, 2017 •

edited

Loading

cmslewis commented Oct 10, 2017 •

edited

Loading

cmslewis Oct 11, 2017 •

edited

Loading

cmslewis Oct 11, 2017 •

edited

Loading

giladgray Oct 11, 2017 •

edited

Loading