[Lens] Prevent KQL Popovers From Stacking (#118258)

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.test.tsx

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import React, { MouseEventHandler } from 'react';
+import React from 'react';
 import { shallow, mount } from 'enzyme';
 import { act } from 'react-dom/test-utils';
 import { EuiPopover, EuiLink } from '@elastic/eui';
@@ -20,19 +20,6 @@ jest.mock('.', () => ({
   defaultLabel: 'label',
 }));
 
-const defaultProps = {
-  filter: {
-    input: { query: 'bytes >= 1', language: 'kuery' },
-    label: 'More than one',
-    id: '1',
-  },
-  setFilter: jest.fn(),
-  indexPattern: createMockedIndexPattern(),
-  Button: ({ onClick }: { onClick: MouseEventHandler }) => (
-    <EuiLink onClick={onClick}>trigger</EuiLink>
-  ),
-  initiallyOpen: true,
-};
 jest.mock('../../../../../../../../src/plugins/data/public', () => ({
   QueryStringInput: () => {
     return 'QueryStringInput';
@@ -40,6 +27,67 @@ jest.mock('../../../../../../../../src/plugins/data/public', () => ({
 }));
 
 describe('filter popover', () => {
+  let defaultProps: Parameters<typeof FilterPopover>[0];
+  let mockOnClick: jest.Mock;
+
+  beforeEach(() => {
+    mockOnClick = jest.fn();
+
+    defaultProps = {
+      filter: {
+        input: { query: 'bytes >= 1', language: 'kuery' },
+        label: 'More than one',
+        id: '1',
+      },
+      setFilter: jest.fn(),
+      indexPattern: createMockedIndexPattern(),
+      Button: () => <EuiLink onClick={mockOnClick}>trigger</EuiLink>,
+      isOpen: true,
+      triggerClose: () => {},
+    };
+  });
+
+  describe('interactions', () => {
+    it('should open/close according to isOpen', () => {
+      const instance = mount(<FilterPopover {...{ ...defaultProps, isOpen: true }} />);
+
+      expect(instance.find(EuiPopover).prop('isOpen')).toEqual(true);
+
+      instance.setProps({ ...defaultProps, isOpen: false });
+      instance.update();
+
+      expect(instance.find(EuiPopover).prop('isOpen')).toEqual(false);
+    });
+
+    it('should report click event', () => {
+      const instance = mount(<FilterPopover {...defaultProps} />);
+
+      expect(mockOnClick).not.toHaveBeenCalled();
+
+      instance.find(EuiPopover).find('button').simulate('click', {});
+
+      expect(mockOnClick).toHaveBeenCalledTimes(1);
+    });
+
+    it('should trigger close', () => {
+      const props = { ...defaultProps, triggerClose: jest.fn() };
+      const instance = mount(<FilterPopover {...props} />);
+      expect(instance.find(EuiPopover).prop('isOpen')).toEqual(true);
+
+      // Trigger from EuiPopover
+      act(() => {
+        instance.find(EuiPopover).prop('closePopover')!();
+      });
+      expect(props.triggerClose).toHaveBeenCalledTimes(1);
+
+      // Trigger from submit
+      act(() => {
+        instance.find(LabelInput).prop('onSubmit')!();
+      });
+      expect(props.triggerClose).toHaveBeenCalledTimes(2);
+    });
+  });
+
   it('passes correct props to QueryStringInput', () => {
     const instance = mount(<FilterPopover {...defaultProps} />);
     instance.update();
@@ -52,16 +100,7 @@ describe('filter popover', () => {
       })
     );
   });
-  it('should be open if is open by creation', () => {
-    const instance = mount(<FilterPopover {...defaultProps} />);
-    instance.update();
-    expect(instance.find(EuiPopover).prop('isOpen')).toEqual(true);
-    act(() => {
-      instance.find(EuiPopover).prop('closePopover')!();
-    });
-    instance.update();
-    expect(instance.find(EuiPopover).prop('isOpen')).toEqual(false);
-  });
+
   it('should call setFilter when modifying QueryInput', () => {
     const setFilter = jest.fn();
     const instance = shallow(<FilterPopover {...defaultProps} setFilter={setFilter} />);
@@ -78,6 +117,7 @@ describe('filter popover', () => {
       id: '1',
     });
   });
+
   it('should call setFilter when modifying LabelInput', () => {
     const setFilter = jest.fn();
     const instance = shallow(<FilterPopover {...defaultProps} setFilter={setFilter} />);

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filter_popover.tsx

@@ -7,7 +7,7 @@
 
 import './filter_popover.scss';
 
-import React, { MouseEventHandler, useEffect, useState } from 'react';
+import React from 'react';
 import { EuiPopover, EuiSpacer } from '@elastic/eui';
 import { FilterValue, defaultLabel, isQueryValid } from '.';
 import { IndexPattern } from '../../../types';
@@ -20,28 +20,40 @@ export const FilterPopover = ({
   setFilter,
   indexPattern,
   Button,
-  initiallyOpen,
+  isOpen,
+  triggerClose,
 }: {
   filter: FilterValue;
   setFilter: Function;
   indexPattern: IndexPattern;
-  Button: React.FunctionComponent<{ onClick: MouseEventHandler }>;
-  initiallyOpen: boolean;
+  Button: React.FunctionComponent;
+  isOpen: boolean;
+  triggerClose: () => void;
 }) => {
-  const [isPopoverOpen, setIsPopoverOpen] = useState(false);
   const inputRef = React.useRef<HTMLInputElement>();
 
-  // set popover open on start to work around EUI bug
-  useEffect(() => {
-    setIsPopoverOpen(initiallyOpen);
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []);
+  // The following code is to prevent an <ESCAPE> keypress
+  // from propagating.
+  //
+  // TODO - It looks like EUI should be handling this
+  // (see https://github.com/elastic/eui/commit/ad97583b0d644690379f72c7a20879cfadb16e7a)
+  const popoverRef = React.useRef<EuiPopover>(null);
+  let panelElement: HTMLDivElement;
+  const panelRefCallback = (element: HTMLDivElement) => {
+    const listener = (event: KeyboardEvent) => {
+      if (event.key === 'Escape') {
+        event.stopPropagation();
+        panelElement.removeEventListener('keydown', listener);
+        popoverRef.current?.closePopover();
+      }
+    };
 
-  const closePopover = () => {
-    if (isPopoverOpen) {
-      setIsPopoverOpen(false);
+    if (element) {
+      panelElement = element;
+      panelElement.addEventListener('keydown', listener);
     }
   };
+  // End <ESCAPE> handling code
 
   const setFilterLabel = (label: string) => setFilter({ ...filter, label });
   const setFilterQuery = (input: Query) => setFilter({ ...filter, input });
@@ -58,19 +70,15 @@ export const FilterPopover = ({
 
   return (
     <EuiPopover
+      ref={popoverRef}
+      panelRef={panelRefCallback}
       data-test-subj="indexPattern-filters-existingFilterContainer"
       anchorClassName="eui-fullWidth"
       panelClassName="lnsIndexPatternDimensionEditor__filtersEditor"
-      isOpen={isPopoverOpen}
+      isOpen={isOpen}
       ownFocus
-      closePopover={() => closePopover()}
-      button={
-        <Button
-          onClick={() => {
-            setIsPopoverOpen((open) => !open);
-          }}
-        />
-      }
+      closePopover={() => triggerClose()}
+      button={<Button />}
     >
       <QueryInput
         isInvalid={!isQueryValid(filter.input, indexPattern)}
@@ -87,7 +95,7 @@ export const FilterPopover = ({
         onChange={setFilterLabel}
         placeholder={getPlaceholder(filter.input.query)}
         inputRef={inputRef}
-        onSubmit={() => closePopover()}
+        onSubmit={() => triggerClose()}
         dataTestSubj="indexPattern-filters-label"
       />
     </EuiPopover>

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx

@@ -6,7 +6,7 @@
  */
 
 import './filters.scss';
-import React, { MouseEventHandler, useState } from 'react';
+import React, { useState } from 'react';
 import { fromKueryExpression, luceneStringToDsl, toElasticsearchQuery } from '@kbn/es-query';
 import { omit } from 'lodash';
 import { i18n } from '@kbn/i18n';
@@ -172,7 +172,7 @@ export const FilterList = ({
   indexPattern: IndexPattern;
   defaultQuery: Filter;
 }) => {
-  const [isOpenByCreation, setIsOpenByCreation] = useState(false);
+  const [activeFilterId, setActiveFilterId] = useState('');
   const [localFilters, setLocalFilters] = useState(() =>
     filters.map((filter) => ({ ...filter, id: generateId() }))
   );
@@ -183,14 +183,19 @@ export const FilterList = ({
     setLocalFilters(updatedFilters);
   };
 
-  const onAddFilter = () =>
+  const onAddFilter = () => {
+    const newFilterId = generateId();
+
     updateFilters([
       ...localFilters,
       {
         ...defaultQuery,
-        id: generateId(),
+        id: newFilterId,
       },
     ]);
+
+    setActiveFilterId(newFilterId);
+  };
   const onRemoveFilter = (id: string) =>
     updateFilters(localFilters.filter((filter) => filter.id !== id));
 
@@ -207,6 +212,14 @@ export const FilterList = ({
       )
     );
 
+  const changeActiveFilter = (filterId: string) => {
+    let newActiveFilterId = filterId;
+    if (activeFilterId === filterId) {
+      newActiveFilterId = ''; // toggle off
+    }
+    setActiveFilterId(newActiveFilterId);
+  };
+
   return (
     <>
       <DragDropBuckets
@@ -235,17 +248,18 @@ export const FilterList = ({
             >
               <FilterPopover
                 data-test-subj="indexPattern-filters-existingFilterContainer"
-                initiallyOpen={idx === localFilters.length - 1 && isOpenByCreation}
+                isOpen={filter.id === activeFilterId}
+                triggerClose={() => changeActiveFilter('')}
                 indexPattern={indexPattern}
                 filter={filter}
                 setFilter={(f: FilterValue) => {
                   onChangeValue(f.id, f.input, f.label);
                 }}
-                Button={({ onClick }: { onClick: MouseEventHandler }) => (
+                Button={() => (
                   <EuiLink
                     className="lnsFiltersOperation__popoverButton"
                     data-test-subj="indexPattern-filters-existingFilterTrigger"
-                    onClick={onClick}
+                    onClick={() => changeActiveFilter(filter.id)}
                     color={isInvalid ? 'danger' : 'text'}
                     title={i18n.translate('xpack.lens.indexPattern.filters.clickToEdit', {
                       defaultMessage: 'Click to edit',
@@ -262,7 +276,6 @@ export const FilterList = ({
       <NewBucketButton
         onClick={() => {
           onAddFilter();
-          setIsOpenByCreation(true);
         }}
         label={i18n.translate('xpack.lens.indexPattern.filters.addaFilter', {
           defaultMessage: 'Add a filter',