[select] fix(MultiSelect): better handling of key events in Tag… (#3836)

packages/select/src/common/classes.ts

@@ -20,6 +20,7 @@ const NS = Classes.getClassNamespace();
 
 export const MULTISELECT = `${NS}-multi-select`;
 export const MULTISELECT_POPOVER = `${MULTISELECT}-popover`;
+export const MULTISELECT_TAG_INPUT_INPUT = `${MULTISELECT}-tag-input-input`;
 export const OMNIBAR = `${NS}-omnibar`;
 export const OMNIBAR_OVERLAY = `${OMNIBAR}-overlay`;
 export const SELECT = `${NS}-select`;

packages/select/src/components/select/multiSelect.tsx

@@ -17,6 +17,7 @@ import classNames from "classnames";
 import * as React from "react";
 
 import {
+    Classes as CoreClasses,
     DISPLAYNAME_PREFIX,
     IPopoverProps,
     ITagInputProps,
@@ -117,6 +118,10 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
             tagInputProps.fill = true;
         }
 
+        // add our own inputProps.className so that we can reference it in event handlers
+        const { inputProps = {} } = tagInputProps;
+        inputProps.className = classNames(inputProps.className, Classes.MULTISELECT_TAG_INPUT_INPUT);
+
         const handleTagInputAdd = (values: any[], method: TagInputAddMethod) => {
             if (method === "paste") {
                 handlePaste(values);
@@ -137,21 +142,22 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
                 onOpened={this.handlePopoverOpened}
             >
                 <div
-                    onKeyDown={this.getTargetKeyDownHandler(handleKeyDown)}
-                    onKeyUp={this.state.isOpen ? handleKeyUp : undefined}
+                    onKeyDown={this.getTagInputKeyDownHandler(handleKeyDown)}
+                    onKeyUp={this.getTagInputKeyUpHandler(handleKeyUp)}
                 >
                     <TagInput
                         placeholder={placeholder}
                         {...tagInputProps}
                         className={classNames(Classes.MULTISELECT, tagInputProps.className)}
                         inputRef={this.refHandlers.input}
+                        inputProps={inputProps}
                         inputValue={listProps.query}
                         onAdd={handleTagInputAdd}
                         onInputChange={listProps.handleQueryChange}
                         values={selectedItems.map(this.props.tagRenderer)}
                     />
                 </div>
-                <div onKeyDown={this.getTargetKeyDownHandler(handleKeyDown)} onKeyUp={handleKeyUp}>
+                <div onKeyDown={handleKeyDown} onKeyUp={handleKeyUp}>
                     {listProps.itemList}
                 </div>
             </Popover>
@@ -191,11 +197,10 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
         Utils.safeInvokeMember(this.props.popoverProps, "onOpened", node);
     };
 
-    private getTargetKeyDownHandler = (
-        handleQueryListKeyDown: React.EventHandler<React.KeyboardEvent<HTMLElement>>,
-    ) => {
+    private getTagInputKeyDownHandler = (handleQueryListKeyDown: React.KeyboardEventHandler<HTMLElement>) => {
         return (e: React.KeyboardEvent<HTMLElement>) => {
             const { which } = e;
+
             if (which === Keys.ESCAPE || which === Keys.TAB) {
                 // By default the escape key will not trigger a blur on the
                 // input element. It must be done explicitly.
@@ -207,9 +212,23 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM
                 this.setState({ isOpen: true });
             }
 
-            if (this.state.isOpen) {
+            const isTargetingTagRemoveButton = (e.target as HTMLElement).closest(`.${CoreClasses.TAG_REMOVE}`) != null;
+
+            if (this.state.isOpen && !isTargetingTagRemoveButton) {
                 Utils.safeInvoke(handleQueryListKeyDown, e);
             }
         };
     };
+
+    private getTagInputKeyUpHandler = (handleQueryListKeyUp: React.KeyboardEventHandler<HTMLElement>) => {
+        return (e: React.KeyboardEvent<HTMLElement>) => {
+            const isTargetingInput = (e.target as HTMLElement).classList.contains(Classes.MULTISELECT_TAG_INPUT_INPUT);
+
+            // only handle events when the focus is on the actual <input> inside the TagInput, as that's
+            // what QueryList is designed to do
+            if (this.state.isOpen && isTargetingInput) {
+                Utils.safeInvoke(handleQueryListKeyUp, e);
+            }
+        };
+    };
 }

packages/select/test/multiSelectTests.tsx

@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 
-import { Tag } from "@blueprintjs/core";
+import { Classes as CoreClasses, Keys, Tag } from "@blueprintjs/core";
+import { dispatchTestKeyboardEventWithCode } from "@blueprintjs/test-commons";
 import { assert } from "chai";
 import { mount } from "enzyme";
 import * as React from "react";
@@ -78,6 +79,25 @@ describe("<MultiSelect>", () => {
         assert.doesNotThrow(() => multiselect({ selectedItems: undefined }));
     });
 
+    it("only triggers QueryList key up events when focus is on TagInput's <input>", () => {
+        const itemSelectSpy = sinon.spy();
+        const wrapper = multiselect({
+            onItemSelect: itemSelectSpy,
+            selectedItems: [TOP_100_FILMS[1]],
+        });
+
+        const firstTagRemoveButton = wrapper
+            .find(`.${CoreClasses.TAG_REMOVE}`)
+            .at(0)
+            .getDOMNode();
+        dispatchTestKeyboardEventWithCode(firstTagRemoveButton, "keyup", "Enter", Keys.ENTER);
+
+        // checks for the bug in https://github.com/palantir/blueprint/issues/3674
+        // where the first item in the dropdown list would get selected upon hitting Enter inside
+        // a TAG_REMOVE button
+        assert.isFalse(itemSelectSpy.calledWith(TOP_100_FILMS[0]));
+    });
+
     function multiselect(props: Partial<IMultiSelectProps<IFilm>> = {}, query?: string) {
         const wrapper = mount(
             <FilmMultiSelect {...defaultProps} {...handlers} {...props}>

packages/test-commons/src/utils.ts

@@ -27,9 +27,20 @@ import * as React from "react";
  * chrome tests.
  */
 export function dispatchTestKeyboardEvent(target: EventTarget, eventType: string, key: string, shift = false) {
-    const event = document.createEvent("KeyboardEvent");
-    const keyCode = key.charCodeAt(0);
+    return dispatchTestKeyboardEventWithCode(target, eventType, key, key.charCodeAt(0), shift);
+}
 
+/**
+ * Same as dispatchTestKeyboardEvent, but with more control over the keyCode.
+ */
+export function dispatchTestKeyboardEventWithCode(
+    target: EventTarget,
+    eventType: string,
+    key: string,
+    keyCode: number,
+    shift = false,
+) {
+    const event = document.createEvent("KeyboardEvent");
     (event as any).initKeyboardEvent(eventType, true, true, window, key, 0, false, false, shift);
 
     // Hack around these readonly properties in WebKit and Chrome