diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx index a546244d1aa..9f11710f7e0 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx +++ b/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 ( ( { }; 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); + }); }); diff --git a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.jsx b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.jsx index 0718e6f4740..407907e7100 100644 --- a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.test.jsx +++ b/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( + container?.contains(event.target)} + onChangeFocus={onChangeFocusA} + > +
+ , + ); + + const componentB = render( + container?.contains(event.target)} + onChangeFocus={onChangeFocusB} + > +
+ , + ); + + 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(); +}); diff --git a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx b/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx index 2d7005b526d..98392676550 100644 --- a/superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx +++ b/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 && ( - + {menuItems.map((node: ReactNode, i: Number) => (
{node}