feat(platform): Improve keypress UX for SearchDropdown (#15185)

* Improve behavior for `Enter` and `Escape` 
* Update tests to reflect new UX

src/sentry/static/sentry/app/components/autoComplete.jsx

@@ -226,7 +226,7 @@ class AutoComplete extends React.Component {
     this.setState(newState);
   };
 
-  moveHighlightedIndex = (step, e) => {
+  moveHighlightedIndex = (step, _e) => {
     let newIndex = this.state.highlightedIndex + step;
 
     // when this component is in virtualized mode, only a subset of items will be passed

src/sentry/static/sentry/app/components/smartSearchBar/index.jsx

@@ -222,7 +222,7 @@ class SmartSearchBar extends React.Component {
 
       searchTerm: '',
       searchItems: [],
-      activeSearchItem: 0,
+      activeSearchItem: -1,
 
       tags: {},
 
@@ -323,9 +323,107 @@ class SmartSearchBar extends React.Component {
     callIfFunction(this.props.onChange, evt.target.value, evt);
   };
 
+  onInputClick = () => {
+    this.updateAutoCompleteItems();
+  };
+
+  onKeyDown = evt => {
+    if (!this.state.searchItems.length) {
+      return;
+    }
+
+    const {key} = evt;
+    const isSelectingDropdownItems = this.state.activeSearchItem !== -1;
+
+    if (key === 'ArrowDown' || key === 'ArrowUp') {
+      evt.preventDefault();
+
+      const {searchItems, flatSearchItems, activeSearchItem} = this.state;
+      const [groupIndex, childrenIndex] = isSelectingDropdownItems
+        ? findSearchItemByIndex(searchItems, activeSearchItem)
+        : [];
+
+      // Remove the previous 'active' property
+      if (typeof groupIndex !== 'undefined') {
+        if (
+          searchItems[groupIndex] &&
+          searchItems[groupIndex].children &&
+          searchItems[groupIndex].children[childrenIndex]
+        ) {
+          delete searchItems[groupIndex].children[childrenIndex].active;
+        }
+      }
+
+      const currIndex = isSelectingDropdownItems ? activeSearchItem : 0;
+      const totalItems = flatSearchItems.length;
+
+      // Move the selected index up/down
+      const nextActiveSearchItem =
+        key === 'ArrowUp'
+          ? (currIndex - 1 + totalItems) % totalItems
+          : isSelectingDropdownItems
+          ? (currIndex + 1) % totalItems
+          : 0;
+
+      const [nextGroupIndex, nextChildrenIndex] = findSearchItemByIndex(
+        searchItems,
+        nextActiveSearchItem
+      );
+
+      // Make sure search items exist (e.g. both groups could be empty) and
+      // attach the 'active' property to the item
+      if (searchItems[nextGroupIndex] && searchItems[nextGroupIndex].children) {
+        searchItems[nextGroupIndex].children[nextChildrenIndex] = {
+          ...searchItems[nextGroupIndex].children[nextChildrenIndex],
+          active: true,
+        };
+      }
+
+      this.setState({
+        activeSearchItem: nextActiveSearchItem,
+        searchItems: searchItems.slice(0),
+      });
+    } else if ((key === 'Tab' || key === 'Enter') && isSelectingDropdownItems) {
+      evt.preventDefault();
+
+      const {activeSearchItem, searchItems} = this.state;
+      const [groupIndex, childrenIndex] = findSearchItemByIndex(
+        searchItems,
+        activeSearchItem
+      );
+      const item = searchItems[groupIndex].children[childrenIndex];
+
+      if (item && !this.isDefaultDropdownItem(item)) {
+        this.onAutoComplete(item.value, item);
+      }
+    }
+  };
+
   onKeyUp = evt => {
-    if (evt.key === 'Escape' || evt.keyCode === 27) {
-      // blur handler should additionally hide dropdown
+    // Other keys are managed at onKeyDown function
+    if (evt.key !== 'Escape') {
+      return;
+    }
+
+    evt.preventDefault();
+    const isSelectingDropdownItems = this.state.activeSearchItem > -1;
+
+    if (isSelectingDropdownItems) {
+      const {searchItems, activeSearchItem} = this.state;
+      const [groupIndex, childrenIndex] = isSelectingDropdownItems
+        ? findSearchItemByIndex(searchItems, activeSearchItem)
+        : [];
+
+      if (groupIndex !== undefined && childrenIndex !== undefined) {
+        delete searchItems[groupIndex].children[childrenIndex].active;
+      }
+
+      this.setState({
+        activeSearchItem: -1,
+        searchItems: this.state.searchItems.slice(0),
+      });
+    } else {
+      // Blur handler should additionally hide dropdown
       this.blur();
     }
   };
@@ -503,10 +601,6 @@ class SmartSearchBar extends React.Component {
     return [];
   };
 
-  onInputClick = () => {
-    this.updateAutoCompleteItems();
-  };
-
   updateAutoCompleteItems = async () => {
     if (this.blurTimeout) {
       clearTimeout(this.blurTimeout);
@@ -630,7 +724,7 @@ class SmartSearchBar extends React.Component {
     return;
   };
 
-  isDefaultDropdownItem = item => item.type === 'default';
+  isDefaultDropdownItem = item => item && item.type === 'default';
 
   /**
    * Updates autocomplete dropdown items and autocomplete index state
@@ -711,71 +805,6 @@ class SmartSearchBar extends React.Component {
     });
   };
 
-  onKeyDown = evt => {
-    if (!this.state.searchItems.length) {
-      return;
-    }
-
-    const {key} = evt;
-
-    if (key === 'ArrowDown' || key === 'ArrowUp') {
-      evt.preventDefault();
-
-      this.setState(state => {
-        const {searchItems, flatSearchItems, activeSearchItem} = state;
-        const [groupIndex, childrenIndex] =
-          findSearchItemByIndex(searchItems, activeSearchItem) || [];
-
-        if (typeof groupIndex !== 'undefined') {
-          if (
-            searchItems[groupIndex] &&
-            searchItems[groupIndex].children &&
-            searchItems[groupIndex].children[childrenIndex]
-          ) {
-            delete searchItems[groupIndex].children[childrenIndex].active;
-          }
-        }
-
-        const totalItems = flatSearchItems.length;
-
-        // Move active selection up/down
-        const nextActiveSearchItem =
-          key === 'ArrowDown'
-            ? (activeSearchItem + 1) % totalItems
-            : (activeSearchItem - 1 + totalItems) % totalItems;
-
-        const [nextGroupIndex, nextChildrenIndex] =
-          findSearchItemByIndex(searchItems, nextActiveSearchItem) || [];
-
-        // Make sure search items exist (e.g. both groups could be empty)
-        if (searchItems[nextGroupIndex] && searchItems[nextGroupIndex].children) {
-          searchItems[nextGroupIndex].children[nextChildrenIndex] = {
-            ...searchItems[nextGroupIndex].children[nextChildrenIndex],
-            active: true,
-          };
-        }
-
-        return {
-          activeSearchItem: nextActiveSearchItem,
-          searchItems: searchItems.slice(0),
-        };
-      });
-    } else if (key === 'Tab') {
-      evt.preventDefault();
-
-      const {activeSearchItem, searchItems} = this.state;
-      const [groupIndex, childrenIndex] = findSearchItemByIndex(
-        searchItems,
-        activeSearchItem
-      );
-      const item = searchItems[groupIndex].children[childrenIndex];
-
-      if (!this.isDefaultDropdownItem(item)) {
-        this.onAutoComplete(item.value, item);
-      }
-    }
-  };
-
   onAutoComplete = (replaceText, item) => {
     if (item.type === 'recent-search') {
       analytics('search.searched', {
@@ -1246,14 +1275,13 @@ function createSearchGroups(
   if (searchGroup.children && !!searchGroup.children.length) {
     searchGroup.children[activeSearchItem] = {
       ...searchGroup.children[activeSearchItem],
-      active: true,
     };
   }
 
   return {
     searchItems: [searchGroup, ...(recentSearchItems ? [recentSearchGroup] : [])],
     flatSearchItems: [...searchItems, ...(recentSearchItems ? recentSearchItems : [])],
-    activeSearchItem,
+    activeSearchItem: -1,
   };
 }
 
@@ -1266,7 +1294,8 @@ function createSearchGroups(
  */
 function findSearchItemByIndex(items, index, _total) {
   let _index = index;
-  let foundSearchItem;
+  let foundSearchItem = [undefined, undefined];
+
   items.find(({children}, i) => {
     if (!children || !children.length) {
       return false;

tests/js/spec/components/smartSearchBar.spec.jsx

@@ -261,7 +261,7 @@ describe('SmartSearchBar', function() {
         const instance = wrapper.instance();
         jest.spyOn(instance, 'blur');
 
-        wrapper.find('input').simulate('keyup', {key: 'Escape', keyCode: '27'});
+        wrapper.find('input').simulate('keyup', {key: 'Escape'});
 
         expect(instance.blur).toHaveBeenCalledTimes(1);
       });
@@ -348,7 +348,7 @@ describe('SmartSearchBar', function() {
       searchBar.updateAutoCompleteItems();
       expect(searchBar.state.searchTerm).toEqual('');
       expect(searchBar.state.searchItems).toEqual([]);
-      expect(searchBar.state.activeSearchItem).toEqual(0);
+      expect(searchBar.state.activeSearchItem).toEqual(-1);
     });
 
     it('sets state when incomplete tag', async function() {
@@ -367,7 +367,7 @@ describe('SmartSearchBar', function() {
       expect(searchBar.state.searchItems).toEqual([
         expect.objectContaining({children: []}),
       ]);
-      expect(searchBar.state.activeSearchItem).toEqual(0);
+      expect(searchBar.state.activeSearchItem).toEqual(-1);
     });
 
     it('sets state when incomplete tag has negation operator', async function() {
@@ -386,7 +386,7 @@ describe('SmartSearchBar', function() {
       expect(searchBar.state.searchItems).toEqual([
         expect.objectContaining({children: []}),
       ]);
-      expect(searchBar.state.activeSearchItem).toEqual(0);
+      expect(searchBar.state.activeSearchItem).toEqual(-1);
     });
 
     it('sets state when incomplete tag as second input', async function() {
@@ -406,7 +406,7 @@ describe('SmartSearchBar', function() {
       expect(searchBar.state.searchTerm).toEqual('fu');
       // 1 items because of headers ("Tags")
       expect(searchBar.state.searchItems).toHaveLength(1);
-      expect(searchBar.state.activeSearchItem).toEqual(0);
+      expect(searchBar.state.activeSearchItem).toEqual(-1);
     });
 
     it('does not request values when tag is environments', function() {

tests/js/spec/views/issueList/searchBar.spec.jsx

@@ -194,8 +194,17 @@ describe('IssueListSearchBar', function() {
 
       wrapper.find('input').simulate('change', {target: {value: 'is:'}});
       await tick();
+
       wrapper.update();
+      expect(
+        wrapper
+          .find('SearchItem')
+          .at(0)
+          .find('li')
+          .prop('className')
+      ).not.toContain('active');
 
+      wrapper.find('input').simulate('keyDown', {key: 'ArrowDown'});
       expect(
         wrapper
           .find('SearchItem')
@@ -204,8 +213,17 @@ describe('IssueListSearchBar', function() {
           .prop('className')
       ).toContain('active');
 
-      wrapper.find('input').simulate('keyDown', {key: 'ArrowUp'});
+      wrapper.find('input').simulate('keyDown', {key: 'ArrowDown'});
+      expect(
+        wrapper
+          .find('SearchItem')
+          .at(1)
+          .find('li')
+          .prop('className')
+      ).toContain('active');
 
+      wrapper.find('input').simulate('keyDown', {key: 'ArrowUp'});
+      wrapper.find('input').simulate('keyDown', {key: 'ArrowUp'});
       expect(
         wrapper
           .find('SearchItem')