fix(dashboard): exit markdown edit mode when clicking outside of element (#35336)

(cherry picked from commit 553204e613)

superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx

@@ -132,6 +132,7 @@ class Markdown extends PureComponent {
     this.handleDeleteComponent = this.handleDeleteComponent.bind(this);
     this.handleResizeStart = this.handleResizeStart.bind(this);
     this.setEditor = this.setEditor.bind(this);
+    this.shouldFocusMarkdown = this.shouldFocusMarkdown.bind(this);
   }
 
   componentDidMount() {
@@ -268,6 +269,13 @@ class Markdown extends PureComponent {
     }
   }
 
+  shouldFocusMarkdown(event, container, menuRef) {
+    if (container?.contains(event.target)) return true;
+    if (menuRef?.contains(event.target)) return true;
+
+    return false;
+  }
+
   renderEditMode() {
     return (
       <MarkdownEditor
@@ -343,6 +351,7 @@ class Markdown extends PureComponent {
         {({ dragSourceRef }) => (
           <WithPopoverMenu
             onChangeFocus={this.handleChangeFocus}
+            shouldFocus={this.shouldFocusMarkdown}
             menuItems={[
               <MarkdownModeDropdown
                 id={`${component.id}-mode`}

superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.test.jsx

@@ -17,7 +17,13 @@
  * under the License.
  */
 import { Provider } from 'react-redux';
-import { act, render, screen, fireEvent } from 'spec/helpers/testing-library';
+import {
+  act,
+  render,
+  screen,
+  fireEvent,
+  userEvent,
+} from 'spec/helpers/testing-library';
 import { mockStore } from 'spec/fixtures/mockStore';
 import { dashboardLayout as mockLayout } from 'spec/fixtures/mockDashboardLayout';
 import MarkdownConnected from './Markdown';
@@ -44,6 +50,7 @@ describe('Markdown', () => {
   };
 
   beforeAll(() => {
+    const originalError = console.error;
     jest.spyOn(console, 'error').mockImplementation(msg => {
       if (
         typeof msg === 'string' &&
@@ -52,7 +59,7 @@ describe('Markdown', () => {
         !msg.includes('Warning: React does not recognize') &&
         !msg.includes("Warning: Can't perform a React state update")
       ) {
-        console.error(msg);
+        originalError.call(console, msg);
       }
     });
   });
@@ -334,4 +341,75 @@ describe('Markdown', () => {
     // Check that width is no longer 248px
     expect(updatedParent).not.toHaveStyle('width: 248px');
   });
+
+  test('shouldFocusMarkdown returns true when clicking inside markdown container', async () => {
+    await setup({ editMode: true });
+
+    const markdownContainer = screen.getByTestId(
+      'dashboard-component-chart-holder',
+    );
+
+    userEvent.click(markdownContainer);
+
+    expect(await screen.findByRole('textbox')).toBeInTheDocument();
+  });
+
+  test('shouldFocusMarkdown returns false when clicking outside markdown container', async () => {
+    await setup({ editMode: true });
+
+    const markdownContainer = screen.getByTestId(
+      'dashboard-component-chart-holder',
+    );
+
+    userEvent.click(markdownContainer);
+
+    expect(await screen.findByRole('textbox')).toBeInTheDocument();
+
+    userEvent.click(document.body);
+    await new Promise(resolve => setTimeout(resolve, 50));
+
+    expect(screen.queryByRole('textbox')).not.toBeInTheDocument();
+  });
+
+  test('shouldFocusMarkdown keeps focus when clicking on menu items', async () => {
+    await setup({ editMode: true });
+
+    const markdownContainer = screen.getByTestId(
+      'dashboard-component-chart-holder',
+    );
+
+    userEvent.click(markdownContainer);
+
+    expect(await screen.findByRole('textbox')).toBeInTheDocument();
+
+    const editButton = screen.getByText('Edit');
+
+    userEvent.click(editButton);
+    await new Promise(resolve => setTimeout(resolve, 50));
+
+    expect(screen.queryByRole('textbox')).toBeInTheDocument();
+  });
+
+  test('should exit edit mode when clicking outside in same row', async () => {
+    await setup({ editMode: true });
+
+    const markdownContainer = screen.getByTestId(
+      'dashboard-component-chart-holder',
+    );
+
+    userEvent.click(markdownContainer);
+
+    expect(await screen.findByRole('textbox')).toBeInTheDocument();
+
+    const outsideElement = document.createElement('div');
+    outsideElement.className = 'grid-row';
+    document.body.appendChild(outsideElement);
+
+    userEvent.click(outsideElement);
+    await new Promise(resolve => setTimeout(resolve, 50));
+
+    expect(screen.queryByRole('textbox')).not.toBeInTheDocument();
+
+    document.body.removeChild(outsideElement);
+  });
 });

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.jsx

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { fireEvent, render } from 'spec/helpers/testing-library';
+import { render, userEvent } from 'spec/helpers/testing-library';
 
 import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';
 
@@ -44,40 +44,102 @@ test('should render the passed children', () => {
   expect(container.querySelector('#child')).toBeInTheDocument();
 });
 
-test('should focus on click in editMode', () => {
+test('should focus on click in editMode', async () => {
   const { container } = setup({ editMode: true });
-  fireEvent.click(container.querySelector('.with-popover-menu'));
+  await userEvent.click(container.querySelector('.with-popover-menu'));
   expect(
     container.querySelector('.with-popover-menu--focused'),
   ).toBeInTheDocument();
 });
 
-test('should render menuItems when focused', () => {
+test('should render menuItems when focused', async () => {
   const { container } = setup({ editMode: true });
   expect(container.querySelector('#menu1')).not.toBeInTheDocument();
   expect(container.querySelector('#menu2')).not.toBeInTheDocument();
 
-  fireEvent.click(container.querySelector('.with-popover-menu'));
+  await userEvent.click(container.querySelector('.with-popover-menu'));
   expect(container.querySelector('#menu1')).toBeInTheDocument();
   expect(container.querySelector('#menu2')).toBeInTheDocument();
 });
 
-test('should not focus when disableClick=true', () => {
+test('should not focus when disableClick=true', async () => {
   const { container } = setup({ disableClick: true, editMode: true });
 
-  fireEvent.click(container.querySelector('.with-popover-menu'));
+  await userEvent.click(container.querySelector('.with-popover-menu'));
   expect(
     container.querySelector('.with-popover-menu--focused'),
   ).not.toBeInTheDocument();
 });
 
-test('should use the passed shouldFocus func to determine if it should focus', () => {
+test('should use the passed shouldFocus func to determine if it should focus', async () => {
   const { container } = setup({ editMode: true, shouldFocus: () => false });
   expect(
     container.querySelector('.with-popover-menu--focused'),
   ).not.toBeInTheDocument();
-  fireEvent.click(container.querySelector('.with-popover-menu'));
+  await userEvent.click(container.querySelector('.with-popover-menu'));
   expect(
     container.querySelector('.with-popover-menu--focused'),
   ).not.toBeInTheDocument();
 });
+
+test('should allow event propagation to enable multiple components to work independently', async () => {
+  const onChangeFocus = jest.fn();
+
+  const { container } = setup({
+    editMode: true,
+    disableClick: false,
+    shouldFocus: () => true,
+    onChangeFocus,
+  });
+
+  const menuComponent = container.querySelector('.with-popover-menu');
+  await userEvent.click(menuComponent);
+
+  expect(onChangeFocus).toHaveBeenCalledWith(true);
+});
+
+test('should unfocus when another component is clicked', async () => {
+  const onChangeFocusA = jest.fn();
+  const onChangeFocusB = jest.fn();
+
+  const componentA = render(
+    <WithPopoverMenu
+      {...props}
+      editMode
+      shouldFocus={(event, container) => container?.contains(event.target)}
+      onChangeFocus={onChangeFocusA}
+    >
+      <div id="child-a" />
+    </WithPopoverMenu>,
+  );
+
+  const componentB = render(
+    <WithPopoverMenu
+      {...props}
+      editMode
+      shouldFocus={(event, container) => container?.contains(event.target)}
+      onChangeFocus={onChangeFocusB}
+    >
+      <div id="child-b" />
+    </WithPopoverMenu>,
+  );
+
+  const menuA = componentA.container.querySelector('.with-popover-menu');
+  const menuB = componentB.container.querySelector('.with-popover-menu');
+
+  await userEvent.click(menuA);
+  expect(onChangeFocusA).toHaveBeenCalledWith(true);
+  expect(
+    componentA.container.querySelector('.with-popover-menu--focused'),
+  ).toBeInTheDocument();
+
+  await userEvent.click(menuB);
+  expect(onChangeFocusB).toHaveBeenCalledWith(true);
+  expect(onChangeFocusA).toHaveBeenCalledWith(false);
+  expect(
+    componentA.container.querySelector('.with-popover-menu--focused'),
+  ).not.toBeInTheDocument();
+  expect(
+    componentB.container.querySelector('.with-popover-menu--focused'),
+  ).toBeInTheDocument();
+});

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx

@@ -33,7 +33,11 @@ interface WithPopoverMenuProps {
   // Event argument is left as "any" because of the clash. In defaultProps it seems
   // like it should be React.FocusEvent<>, however from handleClick() we can also
   // derive that type is EventListenerOrEventListenerObject.
-  shouldFocus: (event: any, container: ShouldFocusContainer) => Boolean;
+  shouldFocus: (
+    event: any,
+    container: ShouldFocusContainer,
+    menuRef: HTMLDivElement | null,
+  ) => Boolean;
   editMode: Boolean;
   style: CSSProperties;
 }
@@ -105,16 +109,21 @@ export default class WithPopoverMenu extends PureComponent<
 > {
   container: ShouldFocusContainer;
 
+  menuRef: HTMLDivElement | null;
+
   static defaultProps = {
     children: null,
     disableClick: false,
     onChangeFocus: null,
     menuItems: [],
     isFocused: false,
-    shouldFocus: (event: any, container: ShouldFocusContainer) => {
+    shouldFocus: (
+      event: any,
+      container: ShouldFocusContainer,
+      menuRef: HTMLDivElement | null,
+    ) => {
       if (container?.contains(event.target)) return true;
-      if (event.target.id === 'menu-item') return true;
-      if (event.target.parentNode?.id === 'menu-item') return true;
+      if (menuRef?.contains(event.target)) return true;
       return false;
     },
     style: null,
@@ -125,7 +134,9 @@ export default class WithPopoverMenu extends PureComponent<
     this.state = {
       isFocused: props.isFocused!,
     };
+    this.menuRef = null;
     this.setRef = this.setRef.bind(this);
+    this.setMenuRef = this.setMenuRef.bind(this);
     this.handleClick = this.handleClick.bind(this);
   }
 
@@ -150,37 +161,49 @@ export default class WithPopoverMenu extends PureComponent<
     this.container = ref;
   }
 
+  setMenuRef(ref: HTMLDivElement | null) {
+    this.menuRef = ref;
+  }
+
+  shouldHandleFocusChange(shouldFocus: Boolean): boolean {
+    const { disableClick } = this.props;
+    const { isFocused } = this.state;
+
+    return (
+      (!disableClick && shouldFocus && !isFocused) ||
+      (!shouldFocus && isFocused)
+    );
+  }
+
   handleClick(event: any) {
     if (!this.props.editMode) {
       return;
     }
 
-    event.stopPropagation();
-
     const {
       onChangeFocus,
       shouldFocus: shouldFocusFunc,
       disableClick,
     } = this.props;
 
-    const shouldFocus = shouldFocusFunc(event, this.container);
+    const shouldFocus = shouldFocusFunc(event, this.container, this.menuRef);
+
+    if (shouldFocus === this.state.isFocused) return;
 
     if (!disableClick && shouldFocus && !this.state.isFocused) {
-      // if not focused, set focus and add a window event listener to capture outside clicks
-      // this enables us to not set a click listener for ever item on a dashboard
       document.addEventListener('click', this.handleClick);
       document.addEventListener('drag', this.handleClick);
+
       this.setState(() => ({ isFocused: true }));
-      if (onChangeFocus) {
-        onChangeFocus(true);
-      }
+
+      if (onChangeFocus) onChangeFocus(true);
     } else if (!shouldFocus && this.state.isFocused) {
       document.removeEventListener('click', this.handleClick);
       document.removeEventListener('drag', this.handleClick);
+
       this.setState(() => ({ isFocused: false }));
-      if (onChangeFocus) {
-        onChangeFocus(false);
-      }
+
+      if (onChangeFocus) onChangeFocus(false);
     }
   }
 
@@ -201,7 +224,7 @@ export default class WithPopoverMenu extends PureComponent<
       >
         {children}
         {editMode && isFocused && (menuItems?.length ?? 0) > 0 && (
-          <PopoverMenuStyles>
+          <PopoverMenuStyles ref={this.setMenuRef}>
             {menuItems.map((node: ReactNode, i: Number) => (
               <div className="menu-item" key={`menu-item-${i}`}>
                 {node}