Skip to content

Commit

Permalink
Link popover: prevent mouse event propagation (#11753)
Browse files Browse the repository at this point in the history
* Prevent mouse event propagation in popover

This could propagate through to parent handlers (such as mousedown for selection) that removes the popover, causing it to disappear.

* Add e2e test to test IsolatedEventContainer

Triggers problem in #11186 and ensures link popover remains

* Update doc manifest with IsolatedEventContainer
  • Loading branch information
johngodley authored Nov 15, 2018
1 parent bcf55ff commit 2e6f051
Show file tree
Hide file tree
Showing 8 changed files with 141 additions and 14 deletions.
6 changes: 6 additions & 0 deletions docs/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,12 @@
"markdown_source": "https://raw.githubusercontent.com/WordPress/gutenberg/master/packages/components/src/icon/README.md",
"parent": "components"
},
{
"title": "IsolatedEventContainer",
"slug": "isolated-event-container",
"markdown_source": "https://raw.githubusercontent.com/WordPress/gutenberg/master/packages/components/src/isolated-event-container/README.md",
"parent": "components"
},
{
"title": "KeyboardShortcuts",
"slug": "keyboard-shortcuts",
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export { default as Toolbar } from './toolbar';
export { default as ToolbarButton } from './toolbar-button';
export { default as Tooltip } from './tooltip';
export { default as TreeSelect } from './tree-select';
export { default as IsolatedEventContainer } from './isolated-event-container';
export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill';

// Higher-Order Components
Expand Down
34 changes: 34 additions & 0 deletions packages/components/src/isolated-event-container/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Isolated Event Container

This is a container that prevents certain events from propagating outside of the container. This is used to wrap
UI elements such as modals and popovers where the propagated event can cause problems. The event continues to work
inside the component.

For example, a `mousedown` event in a modal container can propagate to the surrounding DOM, causing UI outside of the
modal to be interacted with.

The current isolated events are:
- mousedown - This prevents UI interaction with other `mousedown` event handlers, such as selection

## Usage

Creates a custom component that won't propagate `mousedown` events outside of the component.

```jsx
import { IsolatedEventContainer } from '@wordpress/components';

const MyModal = () => {
return (
<IsolatedEventContainer
className="component-some_component"
onClick={ clickHandler }
>
<p>This is an isolated component</p>
</IsolatedEventContainer>
);
};
```

## Props

All props are passed as-is to the `<IsolatedEventContainer />`
34 changes: 34 additions & 0 deletions packages/components/src/isolated-event-container/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* External dependencies
*/
import { Component } from '@wordpress/element';

class IsolatedEventContainer extends Component {
constructor( props ) {
super( props );

this.stopEventPropagationOutsideContainer = this.stopEventPropagationOutsideContainer.bind( this );
}

stopEventPropagationOutsideContainer( event ) {
event.stopPropagation();
}

render() {
const { children, ... props } = this.props;

// Disable reason: this stops certain events from propagating outside of the component.
// - onMouseDown is disabled as this can cause interactions with other DOM elements
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
{ ... props }
onMouseDown={ this.stopEventPropagationOutsideContainer }
>
{ children }
</div>
);
}
}

export default IsolatedEventContainer;
26 changes: 26 additions & 0 deletions packages/components/src/isolated-event-container/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';

/**
* Internal dependencies
*/
import IsolatedEventContainer from '../';

describe( 'IsolatedEventContainer', () => {
it( 'should pass props to container', () => {
const isolated = shallow( <IsolatedEventContainer className="test" onClick="click" /> );

expect( isolated.hasClass( 'test' ) ).toBe( true );
expect( isolated.prop( 'onClick' ) ).toBe( 'click' );
} );

it( 'should stop mousedown event propagation', () => {
const isolated = shallow( <IsolatedEventContainer /> );
const event = { stopPropagation: jest.fn() };

isolated.simulate( 'mousedown', event );
expect( event.stopPropagation ).toHaveBeenCalled();
} );
} );
15 changes: 3 additions & 12 deletions packages/components/src/modal/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { withInstanceId } from '@wordpress/compose';
import ModalFrame from './frame';
import ModalHeader from './header';
import * as ariaHelper from './aria-helper';
import IsolatedEventContainer from '../isolated-event-container';

// Used to count the number of open modals.
let parentElement,
Expand All @@ -26,7 +27,6 @@ class Modal extends Component {
super( props );

this.prepareDOM();
this.stopEventPropagationOutsideModal = this.stopEventPropagationOutsideModal.bind( this );
}

/**
Expand Down Expand Up @@ -102,14 +102,6 @@ class Modal extends Component {
ariaHelper.showApp();
}

/**
* Stop all onMouseDown events propagating further - they should only go to the modal
* @param {string} event Event object
*/
stopEventPropagationOutsideModal( event ) {
event.stopPropagation();
}

/**
* Renders the modal.
*
Expand All @@ -136,9 +128,8 @@ class Modal extends Component {
// other elements underneath the modal overlay.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return createPortal(
<div
<IsolatedEventContainer
className={ classnames( 'components-modal__screen-overlay', overlayClassName ) }
onMouseDown={ this.stopEventPropagationOutsideModal }
>
<ModalFrame
className={ classnames(
Expand All @@ -164,7 +155,7 @@ class Modal extends Component {
{ children }
</div>
</ModalFrame>
</div>,
</IsolatedEventContainer>,
this.node
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import withConstrainedTabbing from '../higher-order/with-constrained-tabbing';
import PopoverDetectOutside from './detect-outside';
import IconButton from '../icon-button';
import ScrollLock from '../scroll-lock';
import IsolatedEventContainer from '../isolated-event-container';
import { Slot, Fill, Consumer } from '../slot-fill';

const FocusManaged = withConstrainedTabbing( withFocusReturn( ( { children } ) => children ) );
Expand Down Expand Up @@ -288,7 +289,7 @@ class Popover extends Component {
/* eslint-disable jsx-a11y/no-static-element-interactions */
let content = (
<PopoverDetectOutside onClickOutside={ onClickOutside }>
<div
<IsolatedEventContainer
className={ classes }
style={ {
top: ! isMobile && popoverTop ? popoverTop + 'px' : undefined,
Expand Down Expand Up @@ -317,7 +318,7 @@ class Popover extends Component {
>
{ children }
</div>
</div>
</IsolatedEventContainer>
</PopoverDetectOutside>
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
Expand Down
34 changes: 34 additions & 0 deletions test/e2e/specs/links.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
newPost,
pressWithModifier,
pressTimes,
insertBlock,
} from '../support/utils';

/**
Expand Down Expand Up @@ -432,4 +433,37 @@ describe( 'Links', () => {
const assertiveContent = await page.evaluate( () => document.querySelector( '#a11y-speak-assertive' ).textContent );
expect( assertiveContent.trim() ).toBe( 'Warning: the link has been inserted but may have errors. Please test it.' );
} );

it( 'link popover remains visible after a mouse drag event', async () => {
// Create some blocks so we components with event handlers on the page
for ( let loop = 0; loop < 5; loop++ ) {
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'This is Gutenberg' );
}

// Focus on first paragraph, so the link popover will appear over the subsequent ones
await page.mouse.click( 120, 280 );

// Select some text
await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' );

// Click on the Link button
await page.click( 'button[aria-label="Link"]' );

// Wait for the URL field to auto-focus
await waitForAutoFocus();

// Click on the Link Settings button
await page.click( 'button[aria-label="Link Settings"]' );

// Move mouse over the 'open in new tab' section, then click and drag
await page.mouse.move( 50, 330 );
await page.mouse.down();
await page.mouse.move( 100, 330, { steps: 10 } );
await page.mouse.up();

// The link popover should still be visible
const popover = await page.$$( '.editor-url-popover' );
expect( popover ).toHaveLength( 1 );
} );
} );

0 comments on commit 2e6f051

Please sign in to comment.