From 61778c2f6808b585dd4fe22eafa3d1b5d4943e97 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 09:30:28 +1000 Subject: [PATCH 01/17] initial implementation --- src/view/drag-handle/drag-handle-types.js | 4 +- src/view/draggable/draggable.jsx | 31 +++++ stories/src/primatives/quote-item.jsx | 10 -- test/unit/view/unconnected-draggable.spec.js | 119 ++++++++++++++++++- 4 files changed, 147 insertions(+), 17 deletions(-) diff --git a/src/view/drag-handle/drag-handle-types.js b/src/view/drag-handle/drag-handle-types.js index a577a2db8d..1b372424df 100644 --- a/src/view/drag-handle/drag-handle-types.js +++ b/src/view/drag-handle/drag-handle-types.js @@ -25,8 +25,8 @@ export type DragHandleProps = {| // If a consumer is using a portal then the item will loose focus // when moving to the portal. This breaks keyboard dragging. // To get around this we manually apply focus if needed when mounting - onFocus: () => void, - onBlur: () => void, + onFocus: (event: FocusEvent) => void, + onBlur: (event: FocusEvent) => void, // Used to initiate dragging onMouseDown: (event: MouseEvent) => void, diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index b97990bda0..057de6c342 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -6,6 +6,7 @@ import memoizeOne from 'memoize-one'; import invariant from 'tiny-invariant'; import type { Position, + DraggableId, DraggableDimension, InitialDragPositions, DroppableId, @@ -40,6 +41,14 @@ export const zIndexOptions: ZIndexOptions = { dropAnimating: 4500, }; +// When moving an item from one list to another +// the component is: +// - unmounted from the old list +// - remounted in the new list +// We help consumers by preserving focus when moving a +// draggable from one list to another +let lastFocused: ?DraggableId; + export default class Draggable extends Component { /* eslint-disable react/sort-comp */ callbacks: DragHandleCallbacks @@ -75,6 +84,24 @@ export default class Draggable extends Component { this.styleContext = context[styleContextKey]; } + componentDidMount() { + if (!this.ref) { + console.error(` + Draggable has not been provided with a ref. + Please use the DraggableProvided > innerRef function + `); + return; + } + + // This draggable was not previously focused + if (this.props.draggableId !== lastFocused) { + return; + } + + // This draggable was previously focused - give it focus! + this.ref.focus(); + } + componentWillUnmount() { // releasing reference to ref for cleanup this.ref = null; @@ -119,10 +146,14 @@ export default class Draggable extends Component { onFocus = () => { this.isFocused = true; + // Record that this was the last focused draggable + lastFocused = this.props.draggableId; } onBlur = () => { this.isFocused = false; + // On blur we can clear our last focused + lastFocused = null; } onMove = (client: Position) => { diff --git a/stories/src/primatives/quote-item.jsx b/stories/src/primatives/quote-item.jsx index 1efdce758d..ed323cc4c4 100644 --- a/stories/src/primatives/quote-item.jsx +++ b/stories/src/primatives/quote-item.jsx @@ -99,16 +99,6 @@ flex-grow: 1; // things we should be doing in the selector as we do not know if consumers // will be using PureComponent export default class QuoteItem extends React.PureComponent { - componentDidMount() { - if (!this.props.autoFocus) { - return; - } - - // eslint-disable-next-line react/no-find-dom-node - const node: HTMLElement = (ReactDOM.findDOMNode(this) : any); - node.focus(); - } - render() { const { quote, isDragging, provided } = this.props; diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 147761cf47..23a97c6d48 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -104,18 +104,17 @@ const getDispatchPropsStub = (): DispatchProps => ({ dropAnimationFinished: jest.fn(), }); -// $ExpectError - not setting children function const defaultOwnProps: OwnProps = { draggableId, + index: 0, isDragDisabled: false, - type, + disableInteractiveElementBlocking: false, + children: () => null, }; -// $ExpectError - not setting children function const disabledOwnProps: OwnProps = { - draggableId, + ...defaultOwnProps, isDragDisabled: true, - type, }; const defaultMapProps: MapProps = { @@ -266,6 +265,14 @@ const customViewport: Viewport = { subject: getArea({ top: 200, left: 100, right: 300, bottom: 300 }), }; +const looseFocus = (wrapper: ReactWrapper) => { + const el: HTMLElement = wrapper.getDOMNode(); + // raw event + el.blur(); + // let the wrapper know about it + wrapper.simulate('blur'); +} + describe('Draggable - unconnected', () => { beforeAll(() => { requestAnimationFrame.reset(); @@ -1367,6 +1374,105 @@ describe('Draggable - unconnected', () => { }); }); + describe('Cross list movement focus retention', () => { + it('should maintain focus when mounted into a different list', () => { + const first = mountDraggable(); + const original: HTMLElement = first.getDOMNode(); + + // Originally does not have focus + expect(original).not.toBe(document.activeElement); + + // Giving focus to draggable + original.focus(); + // Ensuring that the focus event handler is called + first.simulate('focus'); + // Asserting that it is now the focused element + expect(original).toBe(document.activeElement); + + // unmounting original + first.unmount(); + + const second = mountDraggable(); + const last: HTMLElement = second.getDOMNode(); + + expect(last).toBe(document.activeElement); + + // cleanup + looseFocus(second); + }); + + it('should not maintain focus if it did not have it when moving into a new list', () => { + const wrapper = mountDraggable(); + const node: HTMLElement = wrapper.getDOMNode(); + + // Originally does not have focus + expect(node).not.toBe(document.activeElement); + + const second = mountDraggable(); + const fresh: HTMLElement = second.getDOMNode(); + + // Still does not have focus + expect(fresh).not.toBe(document.activeElement); + }); + + it('should not obtain focus if focus is lost before remount', () => { + const firstWrapper = mountDraggable(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); + + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); + + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + firstWrapper.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); + + // Now loosing focus + firstNode.blur(); + firstWrapper.simulate('blur'); + expect(firstNode).not.toBe(document.activeElement); + + // unmounting original + firstWrapper.unmount(); + + const secondWrapper = mountDraggable(); + const secondNode: HTMLElement = secondWrapper.getDOMNode(); + + expect(secondNode).not.toBe(document.activeElement); + }); + + it('should not give focus to a mounting draggable that did not have the last focused id', () => { + const firstWrapper = mountDraggable(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); + + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); + + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + firstWrapper.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); + + // Mounting new draggable with different id - it should not get focus + const secondProps: OwnProps = { + ...defaultOwnProps, + draggableId: 'my cool new id', + }; + const secondWrapper = mountDraggable({ + ownProps: secondProps, + }); + const secondNode: HTMLElement = secondWrapper.getDOMNode(); + expect(secondNode).not.toBe(document.activeElement); + + // cleanup + looseFocus(firstWrapper); + }); + }); + describe('Portal usage', () => { const body: ?HTMLElement = document.body; if (!body) { @@ -1456,6 +1562,9 @@ describe('Draggable - unconnected', () => { expect(latest).not.toBe(original); // no longer in a portal expect(latest).not.toBe(wrapper.find(WithPortal).instance().portal); + + // cleanup + looseFocus(wrapper); }); it('should not take focus if moving to a portal and did not previously have focus', () => { From 7254b05270b6e77583cb55b7fe179ab016385de4 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 09:33:08 +1000 Subject: [PATCH 02/17] adding warning for droppable --- src/view/droppable/droppable.jsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/view/droppable/droppable.jsx b/src/view/droppable/droppable.jsx index bbe566608e..4b84cf0fad 100644 --- a/src/view/droppable/droppable.jsx +++ b/src/view/droppable/droppable.jsx @@ -43,6 +43,15 @@ export default class Droppable extends Component { return value; } + componentDidMount() { + if (!this.ref) { + console.error(` + Droppable has not been provided with a ref. + Please use the DroppableProvided > innerRef function + `); + } + } + /* eslint-enable */ // React calls ref callback twice for every render From b702f48c00a8d34ea8f346ea6cbbd01186660d36 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 10:02:55 +1000 Subject: [PATCH 03/17] removing custom auto focus code --- README.md | 9 --------- stories/src/board/board.jsx | 8 -------- stories/src/board/column.jsx | 2 -- stories/src/multiple-horizontal/quote-app.jsx | 4 +--- stories/src/multiple-vertical/quote-app.jsx | 13 +------------ stories/src/primatives/author-item.jsx | 14 -------------- stories/src/primatives/author-list.jsx | 2 -- stories/src/primatives/quote-item.jsx | 1 - stories/src/primatives/quote-list.jsx | 10 +--------- stories/src/reorder.js | 4 ---- website/src/components/examples/board/board.jsx | 8 -------- website/src/components/examples/board/column.jsx | 2 -- .../examples/multiple-horizontal/quote-app.jsx | 4 +--- .../examples/multiple-vertical/quote-app.jsx | 13 +------------ .../components/examples/primatives/author-item.jsx | 10 ---------- .../components/examples/primatives/author-list.jsx | 2 -- .../components/examples/primatives/quote-item.jsx | 11 ----------- .../components/examples/primatives/quote-list.jsx | 10 +--------- website/src/components/examples/reorder.js | 4 ---- 19 files changed, 6 insertions(+), 125 deletions(-) diff --git a/README.md b/README.md index 274a73ad2e..44529eed9e 100644 --- a/README.md +++ b/README.md @@ -692,15 +692,6 @@ Here are a few poor user experiences that can occur if you change things *during - If you remove the node that the user is dragging, then the drag will instantly end - If you change the dimension of the dragging node, then other things will not move out of the way at the correct time. -#### Force focus after a transition between lists - -When an item is moved from one list to a different list, it loses browser focus if it had it. This is because `React` creates a new node in this situation. It will not lose focus if transitioned within the same list. The dragging item will always have had browser focus if it is dragging with a keyboard. It is highly recommended that you give the item (which is now in a different list) focus again. You can see an example of how to do this in our stories. Here is an example of how you could do it: - -- `onDragEnd`: move the item into the new list and record the id of the item that has moved -- When rendering the reordered list, pass down a prop which will tell the newly moved item to obtain focus -- In the `componentDidMount` lifecycle call back check if the item needs to gain focus based on its props (such as an `autoFocus` prop) -- If focus is required - call `.focus` on the node. You can obtain the node by using `ReactDOM.findDOMNode` or monkey patching the `provided.innerRef` callback. - ### `onDragStart` and `onDragEnd` pairing We try very hard to ensure that each `onDragStart` event is paired with a single `onDragEnd` event. However, there maybe a rogue situation where this is not the case. If that occurs - it is a bug. Currently there is no mechanism to tell the library to cancel a current drag externally. diff --git a/stories/src/board/board.jsx b/stories/src/board/board.jsx index 5e6d3e99eb..4304b43913 100644 --- a/stories/src/board/board.jsx +++ b/stories/src/board/board.jsx @@ -39,7 +39,6 @@ type Props = {| type State = {| columns: QuoteMap, ordered: string[], - autoFocusQuoteId: ?string, |} export default class Board extends Component { @@ -48,7 +47,6 @@ export default class Board extends Component { state: State = { columns: this.props.initial, ordered: Object.keys(this.props.initial), - autoFocusQuoteId: null, } boardRef: ?HTMLElement @@ -64,10 +62,6 @@ export default class Board extends Component { onDragStart = (initial: DragStart) => { publishOnDragStart(initial); - - this.setState({ - autoFocusQuoteId: null, - }); } onDragEnd = (result: DropResult) => { @@ -110,7 +104,6 @@ export default class Board extends Component { this.setState({ columns: data.quoteMap, - autoFocusQuoteId: data.autoFocusQuoteId, }); } @@ -134,7 +127,6 @@ export default class Board extends Component { index={index} title={key} quotes={columns[key]} - autoFocusQuoteId={this.state.autoFocusQuoteId} /> ))} diff --git a/stories/src/board/column.jsx b/stories/src/board/column.jsx index cd73f7bc8a..8c9f206855 100644 --- a/stories/src/board/column.jsx +++ b/stories/src/board/column.jsx @@ -32,7 +32,6 @@ type Props = {| title: string, quotes: Quote[], index: number, - autoFocusQuoteId: ?string, |} export default class Column extends Component { @@ -59,7 +58,6 @@ export default class Column extends Component { listId={title} listType="QUOTE" quotes={quotes} - autoFocusQuoteId={this.props.autoFocusQuoteId} /> )} diff --git a/stories/src/multiple-horizontal/quote-app.jsx b/stories/src/multiple-horizontal/quote-app.jsx index 55a90b004d..8d4daf6d76 100644 --- a/stories/src/multiple-horizontal/quote-app.jsx +++ b/stories/src/multiple-horizontal/quote-app.jsx @@ -35,7 +35,6 @@ export default class QuoteApp extends Component { state: State = { quoteMap: this.props.initial, - autoFocusQuoteId: null, }; onDragStart = (initial: DragStart) => { @@ -58,7 +57,7 @@ export default class QuoteApp extends Component { } render() { - const { quoteMap, autoFocusQuoteId } = this.state; + const { quoteMap } = this.state; return ( { listId={key} listType="CARD" quotes={quoteMap[key]} - autoFocusQuoteId={autoFocusQuoteId} /> ))} diff --git a/stories/src/multiple-vertical/quote-app.jsx b/stories/src/multiple-vertical/quote-app.jsx index c1e8bd6cc8..7fb64dd886 100644 --- a/stories/src/multiple-vertical/quote-app.jsx +++ b/stories/src/multiple-vertical/quote-app.jsx @@ -65,7 +65,6 @@ export default class QuoteApp extends Component { state: State = { quoteMap: this.props.initial, - autoFocusQuoteId: null, }; onDragStart = (initial: DragStart) => { @@ -107,7 +106,7 @@ export default class QuoteApp extends Component { } render() { - const { quoteMap, autoFocusQuoteId } = this.state; + const { quoteMap } = this.state; const disabledDroppable = 'TODO'; return ( @@ -124,7 +123,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'alpha'} quotes={quoteMap.alpha} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -134,7 +132,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'beta'} quotes={quoteMap.beta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -144,7 +141,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'gamma'} quotes={quoteMap.gamma} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -156,7 +152,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'delta'} quotes={quoteMap.delta} - autoFocusQuoteId={autoFocusQuoteId} /> { internalScroll isDropDisabled={disabledDroppable === 'epsilon'} quotes={quoteMap.epsilon} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -176,7 +170,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'zeta'} quotes={quoteMap.zeta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -186,7 +179,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'eta'} quotes={quoteMap.eta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -196,7 +188,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'theta'} quotes={quoteMap.theta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -207,7 +198,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'iota'} quotes={quoteMap.iota} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -218,7 +208,6 @@ export default class QuoteApp extends Component { internalScroll isDropDisabled={disabledDroppable === 'kappa'} quotes={quoteMap.kappa} - autoFocusQuoteId={autoFocusQuoteId} /> diff --git a/stories/src/primatives/author-item.jsx b/stories/src/primatives/author-item.jsx index cfba27c41b..a59fbf9d0b 100644 --- a/stories/src/primatives/author-item.jsx +++ b/stories/src/primatives/author-item.jsx @@ -1,13 +1,10 @@ // @flow import React, { Component } from 'react'; -import ReactDOM from 'react-dom'; import styled from 'styled-components'; import { colors, grid } from '../constants'; import type { DraggableProvided, DraggableStateSnapshot } from '../../../src/'; import type { Author } from '../types'; -type HTMLElement = any; - const Avatar = styled.img` width: 60px; height: 60px; @@ -31,20 +28,9 @@ type Props = {| author: Author, provided: DraggableProvided, snapshot: DraggableStateSnapshot, - autoFocus?: boolean, |} export default class AuthorItem extends Component { - componentDidMount() { - if (!this.props.autoFocus) { - return; - } - - // eslint-disable-next-line react/no-find-dom-node - const node: HTMLElement = ReactDOM.findDOMNode(this); - node.focus(); - } - render() { const author: Author = this.props.author; const provided: DraggableProvided = this.props.provided; diff --git a/stories/src/primatives/author-list.jsx b/stories/src/primatives/author-list.jsx index 7e95c0723f..899285081f 100644 --- a/stories/src/primatives/author-list.jsx +++ b/stories/src/primatives/author-list.jsx @@ -53,7 +53,6 @@ type Props = {| listId: string, listType?: string, internalScroll?: boolean, - autoFocusQuoteId?: ?string, |} export default class AuthorList extends Component { @@ -75,7 +74,6 @@ export default class AuthorList extends Component { author={quote.author} provided={dragProvided} snapshot={dragSnapshot} - autoFocus={this.props.autoFocusQuoteId === quote.id} /> )} diff --git a/stories/src/primatives/quote-item.jsx b/stories/src/primatives/quote-item.jsx index ed323cc4c4..181d83b63a 100644 --- a/stories/src/primatives/quote-item.jsx +++ b/stories/src/primatives/quote-item.jsx @@ -10,7 +10,6 @@ type Props = { quote: Quote, isDragging: boolean, provided: DraggableProvided, - autoFocus?: boolean, } const Container = styled.a` diff --git a/stories/src/primatives/quote-list.jsx b/stories/src/primatives/quote-list.jsx index 7080aa1519..d19504f116 100644 --- a/stories/src/primatives/quote-list.jsx +++ b/stories/src/primatives/quote-list.jsx @@ -50,13 +50,11 @@ type Props = {| isDropDisabled ?: boolean, style?: Object, // may not be provided - and might be null - autoFocusQuoteId?: ?string, ignoreContainerClipping?: boolean, |} type QuoteListProps = {| quotes: Quote[], - autoFocusQuoteId: ?string, |} class InnerQuoteList extends React.Component { @@ -77,7 +75,6 @@ class InnerQuoteList extends React.Component { quote={quote} isDragging={dragSnapshot.isDragging} provided={dragProvided} - autoFocus={this.props.autoFocusQuoteId === quote.id} /> )} @@ -89,12 +86,11 @@ type InnerListProps = {| dropProvided: DroppableProvided, quotes: Quote[], title: ?string, - autoFocusQuoteId: ?string, |} class InnerList extends React.Component { render() { - const { quotes, dropProvided, autoFocusQuoteId } = this.props; + const { quotes, dropProvided } = this.props; const title = this.props.title ? ( {this.props.title} ) : null; @@ -105,7 +101,6 @@ class InnerList extends React.Component { {dropProvided.placeholder} @@ -124,7 +119,6 @@ export default class QuoteList extends React.Component { listType, style, quotes, - autoFocusQuoteId, title, } = this.props; @@ -148,7 +142,6 @@ export default class QuoteList extends React.Component { quotes={quotes} title={title} dropProvided={dropProvided} - autoFocusQuoteId={autoFocusQuoteId} /> ) : ( @@ -156,7 +149,6 @@ export default class QuoteList extends React.Component { quotes={quotes} title={title} dropProvided={dropProvided} - autoFocusQuoteId={autoFocusQuoteId} /> )} diff --git a/stories/src/reorder.js b/stories/src/reorder.js index 54ab4fd640..ad8cd57c9a 100644 --- a/stories/src/reorder.js +++ b/stories/src/reorder.js @@ -24,7 +24,6 @@ type ReorderQuoteMapArgs = {| export type ReorderQuoteMapResult = {| quoteMap: QuoteMap, - autoFocusQuoteId: ?string, |} export const reorderQuoteMap = ({ @@ -49,8 +48,6 @@ export const reorderQuoteMap = ({ }; return { quoteMap: result, - // not auto focusing in own list - autoFocusQuoteId: null, }; } @@ -69,7 +66,6 @@ export const reorderQuoteMap = ({ return { quoteMap: result, - autoFocusQuoteId: target.id, }; }; diff --git a/website/src/components/examples/board/board.jsx b/website/src/components/examples/board/board.jsx index 132a15cf26..61547f33d9 100644 --- a/website/src/components/examples/board/board.jsx +++ b/website/src/components/examples/board/board.jsx @@ -40,7 +40,6 @@ type Props = {| type State = {| columns: QuoteMap, ordered: string[], - autoFocusQuoteId: ?string, |} export default class Board extends Component { @@ -49,7 +48,6 @@ export default class Board extends Component { state: State = { columns: this.props.initial, ordered: Object.keys(this.props.initial), - autoFocusQuoteId: null, } boardRef: ?HTMLElement @@ -65,10 +63,6 @@ export default class Board extends Component { onDragStart = (initial: DragStart) => { publishOnDragStart(initial); - - this.setState({ - autoFocusQuoteId: null, - }); } onDragEnd = (result: DropResult) => { @@ -113,7 +107,6 @@ export default class Board extends Component { this.setState({ columns: data.quoteMap, - autoFocusQuoteId: data.autoFocusQuoteId, }); } @@ -137,7 +130,6 @@ export default class Board extends Component { index={index} title={key} quotes={columns[key]} - autoFocusQuoteId={this.state.autoFocusQuoteId} /> ))} diff --git a/website/src/components/examples/board/column.jsx b/website/src/components/examples/board/column.jsx index 62eb31ea6e..7c8bec5ac3 100644 --- a/website/src/components/examples/board/column.jsx +++ b/website/src/components/examples/board/column.jsx @@ -32,7 +32,6 @@ type Props = {| title: string, quotes: Quote[], index: number, - autoFocusQuoteId: ?string, |} export default class Column extends Component { @@ -59,7 +58,6 @@ export default class Column extends Component { listId={title} listType="QUOTE" quotes={quotes} - autoFocusQuoteId={this.props.autoFocusQuoteId} /> )} diff --git a/website/src/components/examples/multiple-horizontal/quote-app.jsx b/website/src/components/examples/multiple-horizontal/quote-app.jsx index 76fa42856d..af8e35563b 100644 --- a/website/src/components/examples/multiple-horizontal/quote-app.jsx +++ b/website/src/components/examples/multiple-horizontal/quote-app.jsx @@ -36,7 +36,6 @@ export default class QuoteApp extends Component { state: State = { quoteMap: this.props.initial, - autoFocusQuoteId: null, } onDragStart = (initial: DragStart) => { @@ -61,7 +60,7 @@ export default class QuoteApp extends Component { } render() { - const { quoteMap, autoFocusQuoteId } = this.state; + const { quoteMap } = this.state; return ( { listId={key} listType="CARD" quotes={quoteMap[key]} - autoFocusQuoteId={autoFocusQuoteId} /> ))} diff --git a/website/src/components/examples/multiple-vertical/quote-app.jsx b/website/src/components/examples/multiple-vertical/quote-app.jsx index 28bc931b66..0b84087162 100644 --- a/website/src/components/examples/multiple-vertical/quote-app.jsx +++ b/website/src/components/examples/multiple-vertical/quote-app.jsx @@ -70,7 +70,6 @@ export default class QuoteApp extends Component { state: State = { quoteMap: this.props.initial, - autoFocusQuoteId: null, } onDragStart = (initial: DragStart) => { @@ -114,7 +113,7 @@ export default class QuoteApp extends Component { } render() { - const { quoteMap, autoFocusQuoteId } = this.state; + const { quoteMap } = this.state; const disabledDroppable = 'TODO'; return ( @@ -131,7 +130,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'alpha'} quotes={quoteMap.alpha} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -141,7 +139,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'beta'} quotes={quoteMap.beta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -151,7 +148,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'gamma'} quotes={quoteMap.gamma} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -163,7 +159,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'delta'} quotes={quoteMap.delta} - autoFocusQuoteId={autoFocusQuoteId} /> { internalScroll isDropDisabled={disabledDroppable === 'epsilon'} quotes={quoteMap.epsilon} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -183,7 +177,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'zeta'} quotes={quoteMap.zeta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -193,7 +186,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'eta'} quotes={quoteMap.eta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -203,7 +195,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'theta'} quotes={quoteMap.theta} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -214,7 +205,6 @@ export default class QuoteApp extends Component { listType="card" isDropDisabled={disabledDroppable === 'iota'} quotes={quoteMap.iota} - autoFocusQuoteId={autoFocusQuoteId} /> @@ -225,7 +215,6 @@ export default class QuoteApp extends Component { internalScroll isDropDisabled={disabledDroppable === 'kappa'} quotes={quoteMap.kappa} - autoFocusQuoteId={autoFocusQuoteId} /> diff --git a/website/src/components/examples/primatives/author-item.jsx b/website/src/components/examples/primatives/author-item.jsx index ad5e123baf..a02739260a 100644 --- a/website/src/components/examples/primatives/author-item.jsx +++ b/website/src/components/examples/primatives/author-item.jsx @@ -31,19 +31,9 @@ type Props = {| author: Author, provided: DraggableProvided, snapshot: DraggableStateSnapshot, - autoFocus?: boolean, |} export default class AuthorItem extends Component { - componentDidMount() { - if (!this.props.autoFocus) { - return; - } - - // eslint-disable-next-line react/no-find-dom-node - const node: HTMLElement = ReactDOM.findDOMNode(this); - node.focus(); - } render() { const author: Author = this.props.author; diff --git a/website/src/components/examples/primatives/author-list.jsx b/website/src/components/examples/primatives/author-list.jsx index 282ece6efa..31b09c00c3 100644 --- a/website/src/components/examples/primatives/author-list.jsx +++ b/website/src/components/examples/primatives/author-list.jsx @@ -53,7 +53,6 @@ type Props = {| listId: string, listType?: string, internalScroll?: boolean, - autoFocusQuoteId?: ?string, |} export default class AuthorList extends Component { @@ -75,7 +74,6 @@ export default class AuthorList extends Component { author={quote.author} provided={dragProvided} snapshot={dragSnapshot} - autoFocus={this.props.autoFocusQuoteId === quote.id} /> )} diff --git a/website/src/components/examples/primatives/quote-item.jsx b/website/src/components/examples/primatives/quote-item.jsx index 141628c64b..393c31044c 100644 --- a/website/src/components/examples/primatives/quote-item.jsx +++ b/website/src/components/examples/primatives/quote-item.jsx @@ -10,7 +10,6 @@ type Props = { quote: Quote, isDragging: boolean, provided: DraggableProvided, - autoFocus?: boolean, } const Container = styled.a` @@ -99,16 +98,6 @@ flex-grow: 1; // things we should be doing in the selector as we do not know if consumers // will be using PureComponent export default class QuoteItem extends React.PureComponent { - componentDidMount() { - if (!this.props.autoFocus) { - return; - } - - // eslint-disable-next-line react/no-find-dom-node - const node: HTMLElement = (ReactDOM.findDOMNode(this) : any); - node.focus(); - } - render() { const { quote, isDragging, provided } = this.props; diff --git a/website/src/components/examples/primatives/quote-list.jsx b/website/src/components/examples/primatives/quote-list.jsx index d0475e43e0..cf28d6be5f 100644 --- a/website/src/components/examples/primatives/quote-list.jsx +++ b/website/src/components/examples/primatives/quote-list.jsx @@ -50,13 +50,11 @@ type Props = {| isDropDisabled ?: boolean, style?: Object, // may not be provided - and might be null - autoFocusQuoteId?: ?string, ignoreContainerClipping?: boolean, |} type QuoteListProps = {| quotes: Quote[], - autoFocusQuoteId: ?string, |} class InnerQuoteList extends Component { @@ -79,7 +77,6 @@ class InnerQuoteList extends Component { quote={quote} isDragging={dragSnapshot.isDragging} provided={dragProvided} - autoFocus={this.props.autoFocusQuoteId === quote.id} /> )} @@ -93,12 +90,11 @@ type InnerListProps = {| dropProvided: DroppableProvided, quotes: Quote[], title: ?string, - autoFocusQuoteId: ?string, |} class InnerList extends Component { render() { - const { quotes, dropProvided, autoFocusQuoteId } = this.props; + const { quotes, dropProvided } = this.props; const title = this.props.title ? ( {this.props.title} ) : null; @@ -109,7 +105,6 @@ class InnerList extends Component { {dropProvided.placeholder} @@ -128,7 +123,6 @@ export default class QuoteList extends Component { listType, style, quotes, - autoFocusQuoteId, title, } = this.props; @@ -152,7 +146,6 @@ export default class QuoteList extends Component { quotes={quotes} title={title} dropProvided={dropProvided} - autoFocusQuoteId={autoFocusQuoteId} /> ) : ( @@ -160,7 +153,6 @@ export default class QuoteList extends Component { quotes={quotes} title={title} dropProvided={dropProvided} - autoFocusQuoteId={autoFocusQuoteId} /> )} diff --git a/website/src/components/examples/reorder.js b/website/src/components/examples/reorder.js index 2525127c81..87309cd76f 100644 --- a/website/src/components/examples/reorder.js +++ b/website/src/components/examples/reorder.js @@ -24,7 +24,6 @@ type ReorderQuoteMapArgs = {| export type ReorderQuoteMapResult = {| quoteMap: QuoteMap, - autoFocusQuoteId: ?string, |} export const reorderQuoteMap = ({ @@ -49,8 +48,6 @@ export const reorderQuoteMap = ({ }; return { quoteMap: result, - // not auto focusing in own list - autoFocusQuoteId: null, }; } @@ -69,6 +66,5 @@ export const reorderQuoteMap = ({ return { quoteMap: result, - autoFocusQuoteId: target.id, }; }; From 63f0c41ab8f3f331449a15431d8546e87bba8d38 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 11:04:34 +1000 Subject: [PATCH 04/17] now focusing on drag handle rather than draggable --- README.md | 4 +++ src/view/data-attributes.js | 5 ++++ src/view/draggable/draggable.jsx | 15 +++++----- src/view/draggable/focus-on-drag-handle.js | 30 +++++++++++++++++++ src/view/style-marshal/get-styles.js | 9 +++--- src/view/style-marshal/selectors.js | 0 src/view/style-marshal/style-marshal.js | 3 +- ...simple-vertical-list-with-keyboard.spec.js | 8 +++-- test/unit/view/style-marshal.spec.js | 3 +- test/unit/view/unconnected-draggable.spec.js | 3 +- 10 files changed, 61 insertions(+), 19 deletions(-) create mode 100644 src/view/data-attributes.js create mode 100644 src/view/draggable/focus-on-drag-handle.js create mode 100644 src/view/style-marshal/selectors.js diff --git a/README.md b/README.md index 44529eed9e..f7c3464751 100644 --- a/README.md +++ b/README.md @@ -1018,6 +1018,10 @@ It is a contract of this library that it owns the positioning logic of the dragg To get around this you can use [`React.Portal`](https://reactjs.org/docs/portals.html). We do not enable this functionality by default as it has performance problems. We have a [using a portal guide](/guides/using-a-portal.md) explaining the performance problem in more detail and how you can set up your own `React.Portal` if you want to. +##### Focus retention on movement + +When moving a `Draggable` from one list to another the default browser behaviour is for the *drag handle* element to loose focus. This is not good when dragging with a keyboard as the user is then unable to continue to interact with the element. To improve this user experience we give a *drag handle* focus as it mounts if it had browser focus when it unmounted and nothing else has obtained browser focus. + ##### Extending `DraggableProps.style` If you are using inline styles you are welcome to extend the `DraggableProps.style` object. You are also welcome to apply the `DraggableProps.style` object using inline styles and use your own styling solution for the component itself - such as [styled-components](https://github.com/styled-components/styled-components). diff --git a/src/view/data-attributes.js b/src/view/data-attributes.js new file mode 100644 index 0000000000..c41d51f57a --- /dev/null +++ b/src/view/data-attributes.js @@ -0,0 +1,5 @@ +// @flow +export const prefix: string = 'data-react-beautiful-dnd'; +export const dragHandle: string = `${prefix}-drag-handle`; +export const draggable: string = `${prefix}-draggable`; +export const droppable: string = `${prefix}-droppable`; diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index 057de6c342..90738bbbe9 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -15,6 +15,7 @@ import type { import DraggableDimensionPublisher from '../draggable-dimension-publisher/'; import Moveable from '../moveable/'; import DragHandle from '../drag-handle'; +import focusOnDragHandle from './focus-on-drag-handle'; import getViewport from '../window/get-viewport'; // eslint-disable-next-line no-duplicate-imports import type { @@ -53,7 +54,7 @@ export default class Draggable extends Component { /* eslint-disable react/sort-comp */ callbacks: DragHandleCallbacks styleContext: string - isFocused: boolean = false + isDragHandleFocused: boolean = false ref: ?HTMLElement = null // Need to declare contextTypes without flow @@ -98,8 +99,8 @@ export default class Draggable extends Component { return; } - // This draggable was previously focused - give it focus! - this.ref.focus(); + // This drag handle was previously focused - give it focus! + focusOnDragHandle(this.ref); } componentWillUnmount() { @@ -145,13 +146,13 @@ export default class Draggable extends Component { } onFocus = () => { - this.isFocused = true; + this.isDragHandleFocused = true; // Record that this was the last focused draggable lastFocused = this.props.draggableId; } onBlur = () => { - this.isFocused = false; + this.isDragHandleFocused = false; // On blur we can clear our last focused lastFocused = null; } @@ -223,8 +224,8 @@ export default class Draggable extends Component { // After a ref change we might need to manually force focus onto the ref. // When moving something into or out of a portal the element looses focus // https://github.com/facebook/react/issues/12454 - if (this.ref && this.isFocused) { - this.ref.focus(); + if (this.ref && this.isDragHandleFocused) { + focusOnDragHandle(this.ref); } }) diff --git a/src/view/draggable/focus-on-drag-handle.js b/src/view/draggable/focus-on-drag-handle.js new file mode 100644 index 0000000000..9d5394df51 --- /dev/null +++ b/src/view/draggable/focus-on-drag-handle.js @@ -0,0 +1,30 @@ +// @flow +import { dragHandle } from '../data-attributes'; + +const selector: string = `[${dragHandle}]`; + +const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => { + if (draggableRef.hasAttribute(dragHandle)) { + return draggableRef; + } + + // find the first nested drag handle + const el: ?HTMLElement = draggableRef.querySelector(selector); + + if (!el) { + return null; + } + + return el; +}; + +const focusOnDragHandle = (draggableRef: HTMLElement) => { + const dragHandleRef: ?HTMLElement = getDragHandleRef(draggableRef); + if (!dragHandleRef) { + console.error('Draggable cannot focus on the drag handle as it cannot be found'); + return; + } + dragHandleRef.focus(); +}; + +export default focusOnDragHandle; diff --git a/src/view/style-marshal/get-styles.js b/src/view/style-marshal/get-styles.js index 48895ac94d..082370ad7f 100644 --- a/src/view/style-marshal/get-styles.js +++ b/src/view/style-marshal/get-styles.js @@ -1,5 +1,6 @@ // @flow import { css } from '../animation'; +import * as attributes from '../data-attributes'; export type Styles = {| dragging: string, @@ -8,12 +9,10 @@ export type Styles = {| userCancel: string, |} -const prefix: string = 'data-react-beautiful-dnd'; - export default (styleContext: string): Styles => { - const dragHandleSelector: string = `[${prefix}-drag-handle="${styleContext}"]`; - const draggableSelector: string = `[${prefix}-draggable="${styleContext}"]`; - const droppableSelector: string = `[${prefix}-droppable="${styleContext}"]`; + const dragHandleSelector: string = `[${attributes.dragHandle}="${styleContext}"]`; + const draggableSelector: string = `[${attributes.draggable}="${styleContext}"]`; + const droppableSelector: string = `[${attributes.droppable}="${styleContext}"]`; // ## Drag handle styles diff --git a/src/view/style-marshal/selectors.js b/src/view/style-marshal/selectors.js new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/view/style-marshal/style-marshal.js b/src/view/style-marshal/style-marshal.js index 4c1868dbca..9cf5741769 100644 --- a/src/view/style-marshal/style-marshal.js +++ b/src/view/style-marshal/style-marshal.js @@ -2,6 +2,7 @@ import memoizeOne from 'memoize-one'; import invariant from 'tiny-invariant'; import getStyles, { type Styles } from './get-styles'; +import { prefix } from '../data-attributes'; import type { StyleMarshal } from './style-marshal-types'; import type { State as AppState, @@ -15,8 +16,6 @@ type State = {| el: ?HTMLStyleElement, |} -const prefix: string = 'data-react-beautiful-dnd'; - // Required for server side rendering as count is persisted across requests export const resetStyleContext = () => { count = 0; diff --git a/test/browser/simple-vertical-list-with-keyboard.spec.js b/test/browser/simple-vertical-list-with-keyboard.spec.js index deeca6e438..d368cc3a98 100644 --- a/test/browser/simple-vertical-list-with-keyboard.spec.js +++ b/test/browser/simple-vertical-list-with-keyboard.spec.js @@ -2,6 +2,8 @@ * @jest-environment node */ // @flow +import * as attributes from '../../src/view/data-attributes'; + const puppeteer = require('puppeteer'); const urlSingleList: string = 'http://localhost:9002/iframe.html?selectedKind=single%20vertical%20list&selectedStory=basic'; @@ -29,9 +31,9 @@ const returnPositionAndText = async (page: Object, selector: Selector) => }); /* Css selectors used */ -const singleListContainer: string = '[data-react-beautiful-dnd-droppable]'; -const firstCard: string = '[data-react-beautiful-dnd-drag-handle]:nth-child(1)'; -const secondCard: string = '[data-react-beautiful-dnd-drag-handle]:nth-child(2)'; +const singleListContainer: string = `[${attributes.droppable}]`; +const firstCard: string = `[${attributes.dragHandle}]:nth-child(1)`; +const secondCard: string = `[${attributes.dragHandle}]:nth-child(2)`; describe('Browser test: single vertical list with keyboard', () => { let browser; diff --git a/test/unit/view/style-marshal.spec.js b/test/unit/view/style-marshal.spec.js index a20829b66c..678c4568b2 100644 --- a/test/unit/view/style-marshal.spec.js +++ b/test/unit/view/style-marshal.spec.js @@ -2,13 +2,14 @@ import createStyleMarshal, { resetStyleContext } from '../../../src/view/style-marshal/style-marshal'; import getStyles, { type Styles } from '../../../src/view/style-marshal/get-styles'; import getStatePreset from '../../utils/get-simple-state-preset'; +import { prefix } from '../../../src/view/data-attributes'; import type { StyleMarshal } from '../../../src/view/style-marshal/style-marshal-types'; import type { State } from '../../../src/types'; const state = getStatePreset(); const getStyleTagSelector = (context: string) => - `style[data-react-beautiful-dnd="${context}"]`; + `style[${prefix}="${context}"]`; const getStyleFromTag = (context: string): string => { const selector: string = getStyleTagSelector(context); diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 23a97c6d48..7d9321c917 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -41,6 +41,7 @@ import { combine, withStore, withDroppableId, withStyleContext, withDimensionMar import { dispatchWindowMouseEvent, mouseEvent } from '../../utils/user-input-util'; import getViewport from '../../../src/view/window/get-viewport'; import { setViewport, resetViewport } from '../../utils/viewport'; +import * as attributes from '../../../src/view/data-attributes'; class Item extends Component<{ provided: Provided }> { render() { @@ -310,7 +311,7 @@ describe('Draggable - unconnected', () => { }); const provided: Provided = getLastCall(myMock)[0].provided; - expect(provided.draggableProps['data-react-beautiful-dnd-draggable']).toEqual(styleMarshal.styleContext); + expect(provided.draggableProps[attributes.draggable]).toEqual(styleMarshal.styleContext); }); describe('drag handle', () => { From e02fc79212f0f2116fb08ede061a6033af59be6b Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 11:35:26 +1000 Subject: [PATCH 05/17] fixing linting and flow --- src/view/drag-handle/drag-handle-types.js | 4 +- src/view/draggable/draggable.jsx | 8 +- src/view/draggable/focus-on-drag-handle.js | 8 +- test/unit/view/unconnected-draggable.spec.js | 237 ++++++++++++------- test/unit/view/unconnected-droppable.spec.js | 7 +- 5 files changed, 161 insertions(+), 103 deletions(-) diff --git a/src/view/drag-handle/drag-handle-types.js b/src/view/drag-handle/drag-handle-types.js index 1b372424df..a577a2db8d 100644 --- a/src/view/drag-handle/drag-handle-types.js +++ b/src/view/drag-handle/drag-handle-types.js @@ -25,8 +25,8 @@ export type DragHandleProps = {| // If a consumer is using a portal then the item will loose focus // when moving to the portal. This breaks keyboard dragging. // To get around this we manually apply focus if needed when mounting - onFocus: (event: FocusEvent) => void, - onBlur: (event: FocusEvent) => void, + onFocus: () => void, + onBlur: () => void, // Used to initiate dragging onMouseDown: (event: MouseEvent) => void, diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index 90738bbbe9..a971444b95 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -68,8 +68,8 @@ export default class Draggable extends Component { super(props, context); const callbacks: DragHandleCallbacks = { - onFocus: this.onFocus, - onBlur: this.onBlur, + onFocus: this.onDragHandleFocus, + onBlur: this.onDragHandleBlur, onLift: this.onLift, onMove: this.onMove, onDrop: this.onDrop, @@ -145,13 +145,13 @@ export default class Draggable extends Component { lift(draggableId, initial, getViewport(), autoScrollMode); } - onFocus = () => { + onDragHandleFocus = () => { this.isDragHandleFocused = true; // Record that this was the last focused draggable lastFocused = this.props.draggableId; } - onBlur = () => { + onDragHandleBlur = () => { this.isDragHandleFocused = false; // On blur we can clear our last focused lastFocused = null; diff --git a/src/view/draggable/focus-on-drag-handle.js b/src/view/draggable/focus-on-drag-handle.js index 9d5394df51..21a626a365 100644 --- a/src/view/draggable/focus-on-drag-handle.js +++ b/src/view/draggable/focus-on-drag-handle.js @@ -9,13 +9,11 @@ const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => { } // find the first nested drag handle + // querySelector will return the first match on a breadth first search which is what we want + // https://codepen.io/alexreardon/pen/erOqyZ const el: ?HTMLElement = draggableRef.querySelector(selector); - if (!el) { - return null; - } - - return el; + return el || null; }; const focusOnDragHandle = (draggableRef: HTMLElement) => { diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 7d9321c917..bb2a0dd7ff 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -272,6 +272,24 @@ const looseFocus = (wrapper: ReactWrapper) => { el.blur(); // let the wrapper know about it wrapper.simulate('blur'); +}; +class WithNestedHandle extends Component<{ provided: Provided }> { + render() { + const provided: Provided = this.props.provided; + return ( +
provided.innerRef(ref)} + {...provided.draggableProps} + > +
+ Cannot drag by me +
+
+ Can drag by me +
+
+ ); + } } describe('Draggable - unconnected', () => { @@ -349,25 +367,6 @@ describe('Draggable - unconnected', () => { }); describe('non standard drag handle', () => { - class WithNestedHandle extends Component<{ provided: Provided }> { - render() { - const provided: Provided = this.props.provided; - return ( -
provided.innerRef(ref)} - {...provided.draggableProps} - > -
- Cannot drag by me -
-
- Can drag by me -
-
- ); - } - } - it('should allow the ability to have the drag handle to be a child of the draggable', () => { const dispatchProps: DispatchProps = getDispatchPropsStub(); managedWrapper = mountDraggable({ @@ -1376,101 +1375,157 @@ describe('Draggable - unconnected', () => { }); describe('Cross list movement focus retention', () => { - it('should maintain focus when mounted into a different list', () => { - const first = mountDraggable(); - const original: HTMLElement = first.getDOMNode(); + describe('draggable is the drag handle', () => { + it('should maintain focus when mounted into a different list', () => { + const first = mountDraggable(); + const original: HTMLElement = first.getDOMNode(); - // Originally does not have focus - expect(original).not.toBe(document.activeElement); + // Originally does not have focus + expect(original).not.toBe(document.activeElement); - // Giving focus to draggable - original.focus(); - // Ensuring that the focus event handler is called - first.simulate('focus'); - // Asserting that it is now the focused element - expect(original).toBe(document.activeElement); + // Giving focus to draggable + original.focus(); + // Ensuring that the focus event handler is called + first.simulate('focus'); + // Asserting that it is now the focused element + expect(original).toBe(document.activeElement); - // unmounting original - first.unmount(); + // unmounting original + first.unmount(); - const second = mountDraggable(); - const last: HTMLElement = second.getDOMNode(); + const second = mountDraggable(); + const last: HTMLElement = second.getDOMNode(); - expect(last).toBe(document.activeElement); + expect(last).toBe(document.activeElement); - // cleanup - looseFocus(second); - }); + // cleanup + looseFocus(second); + first.unmount(); + second.unmount(); + }); - it('should not maintain focus if it did not have it when moving into a new list', () => { - const wrapper = mountDraggable(); - const node: HTMLElement = wrapper.getDOMNode(); + it('should not maintain focus if it did not have it when moving into a new list', () => { + const wrapper = mountDraggable(); + const node: HTMLElement = wrapper.getDOMNode(); - // Originally does not have focus - expect(node).not.toBe(document.activeElement); + // Originally does not have focus + expect(node).not.toBe(document.activeElement); - const second = mountDraggable(); - const fresh: HTMLElement = second.getDOMNode(); + const second = mountDraggable(); + const fresh: HTMLElement = second.getDOMNode(); - // Still does not have focus - expect(fresh).not.toBe(document.activeElement); - }); + // Still does not have focus + expect(fresh).not.toBe(document.activeElement); - it('should not obtain focus if focus is lost before remount', () => { - const firstWrapper = mountDraggable(); - const firstNode: HTMLElement = firstWrapper.getDOMNode(); + wrapper.unmount(); + }); - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); + it('should not obtain focus if focus is lost before remount', () => { + const firstWrapper = mountDraggable(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - firstWrapper.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); - // Now loosing focus - firstNode.blur(); - firstWrapper.simulate('blur'); - expect(firstNode).not.toBe(document.activeElement); + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + firstWrapper.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); - // unmounting original - firstWrapper.unmount(); + // Now loosing focus + firstNode.blur(); + firstWrapper.simulate('blur'); + expect(firstNode).not.toBe(document.activeElement); - const secondWrapper = mountDraggable(); - const secondNode: HTMLElement = secondWrapper.getDOMNode(); + // unmounting original + firstWrapper.unmount(); - expect(secondNode).not.toBe(document.activeElement); - }); + const secondWrapper = mountDraggable(); + const secondNode: HTMLElement = secondWrapper.getDOMNode(); - it('should not give focus to a mounting draggable that did not have the last focused id', () => { - const firstWrapper = mountDraggable(); - const firstNode: HTMLElement = firstWrapper.getDOMNode(); + expect(secondNode).not.toBe(document.activeElement); - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); + // cleanup + firstWrapper.unmount(); + secondWrapper.unmount(); + }); - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - firstWrapper.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); + it('should not give focus to a mounting draggable that did not have the last focused id', () => { + const firstWrapper = mountDraggable(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); - // Mounting new draggable with different id - it should not get focus - const secondProps: OwnProps = { - ...defaultOwnProps, - draggableId: 'my cool new id', - }; - const secondWrapper = mountDraggable({ - ownProps: secondProps, + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); + + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + firstWrapper.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); + + // Mounting new draggable with different id - it should not get focus + const secondProps: OwnProps = { + ...defaultOwnProps, + draggableId: 'my cool new id', + }; + const secondWrapper = mountDraggable({ + ownProps: secondProps, + }); + const secondNode: HTMLElement = secondWrapper.getDOMNode(); + expect(secondNode).not.toBe(document.activeElement); + + // cleanup + looseFocus(firstWrapper); + firstWrapper.unmount(); + secondWrapper.unmount(); }); - const secondNode: HTMLElement = secondWrapper.getDOMNode(); - expect(secondNode).not.toBe(document.activeElement); + }); - // cleanup - looseFocus(firstWrapper); + describe('draggable is not the drag handle', () => { + it('should restore focus after an unmount', () => { + const original = mountDraggable({ + WrappedComponent: WithNestedHandle, + }); + const handle: ?HTMLElement = original.getDOMNode().querySelector('.can-drag'); + + if (!handle) { + throw new Error('Could not find drag handle'); + } + + // Should not have focus by default + expect(handle).not.toBe(document.activeElement); + // Lets give it focus + handle.focus(); + original.find('.can-drag').simulate('focus'); + // Okay, it now has focus + expect(handle).toBe(document.activeElement); + + // Goodbye old friend + original.unmount(); + + const next = mountDraggable({ + WrappedComponent: WithNestedHandle, + }); + + const newHandle: ?HTMLElement = next.getDOMNode().querySelector('.can-drag'); + + if (!newHandle) { + throw new Error('Could not find drag handle'); + } + + expect(newHandle).toBe(document.activeElement); + + // cleanup + // remove focus + handle.blur(); + next.find('.can-drag').simulate('blur'); + // unmount all the things + next.unmount(); + original.unmount(); + }); }); }); diff --git a/test/unit/view/unconnected-droppable.spec.js b/test/unit/view/unconnected-droppable.spec.js index 0005a6001d..6b5883cfca 100644 --- a/test/unit/view/unconnected-droppable.spec.js +++ b/test/unit/view/unconnected-droppable.spec.js @@ -27,7 +27,12 @@ const getStubber = (mock: Function) => snapshot: this.props.snapshot, }); return ( -
Hey there
+
+ Hey there +
); } }; From da31bf6917cf4a5af3ca1a16e89ec3583d1ced7b Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 11:41:00 +1000 Subject: [PATCH 06/17] improving readme --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f7c3464751..1465114b08 100644 --- a/README.md +++ b/README.md @@ -1018,9 +1018,9 @@ It is a contract of this library that it owns the positioning logic of the dragg To get around this you can use [`React.Portal`](https://reactjs.org/docs/portals.html). We do not enable this functionality by default as it has performance problems. We have a [using a portal guide](/guides/using-a-portal.md) explaining the performance problem in more detail and how you can set up your own `React.Portal` if you want to. -##### Focus retention on movement +##### Focus retention when moving between lists -When moving a `Draggable` from one list to another the default browser behaviour is for the *drag handle* element to loose focus. This is not good when dragging with a keyboard as the user is then unable to continue to interact with the element. To improve this user experience we give a *drag handle* focus as it mounts if it had browser focus when it unmounted and nothing else has obtained browser focus. +When moving a `Draggable` from one list to another the default browser behaviour is for the *drag handle* element to loose focus. This is because the old element is being destroyed and a new one is being created. The loss of focus is not good when dragging with a keyboard as the user is then unable to continue to interact with the element. To improve this user experience we give a *drag handle* focus as it mounts if it had browser focus when it unmounted and nothing else has obtained browser focus. ##### Extending `DraggableProps.style` From 8166325f8e66efd288d4be5629e090de22382856 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 15:35:03 +1000 Subject: [PATCH 07/17] improved focus retention --- src/view/draggable/draggable.jsx | 23 +---- src/view/draggable/focus-retainer.js | 99 ++++++++++++++++++++ test/unit/view/unconnected-draggable.spec.js | 44 ++++++++- 3 files changed, 146 insertions(+), 20 deletions(-) create mode 100644 src/view/draggable/focus-retainer.js diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index a971444b95..8a69ded267 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -15,6 +15,7 @@ import type { import DraggableDimensionPublisher from '../draggable-dimension-publisher/'; import Moveable from '../moveable/'; import DragHandle from '../drag-handle'; +import focusRetainer from './focus-retainer'; import focusOnDragHandle from './focus-on-drag-handle'; import getViewport from '../window/get-viewport'; // eslint-disable-next-line no-duplicate-imports @@ -42,14 +43,6 @@ export const zIndexOptions: ZIndexOptions = { dropAnimating: 4500, }; -// When moving an item from one list to another -// the component is: -// - unmounted from the old list -// - remounted in the new list -// We help consumers by preserving focus when moving a -// draggable from one list to another -let lastFocused: ?DraggableId; - export default class Draggable extends Component { /* eslint-disable react/sort-comp */ callbacks: DragHandleCallbacks @@ -94,13 +87,7 @@ export default class Draggable extends Component { return; } - // This draggable was not previously focused - if (this.props.draggableId !== lastFocused) { - return; - } - - // This drag handle was previously focused - give it focus! - focusOnDragHandle(this.ref); + focusRetainer.tryRestoreFocus(this.props.draggableId, this.ref); } componentWillUnmount() { @@ -147,14 +134,12 @@ export default class Draggable extends Component { onDragHandleFocus = () => { this.isDragHandleFocused = true; - // Record that this was the last focused draggable - lastFocused = this.props.draggableId; + focusRetainer.onDragHandleFocus(this.props.draggableId); } onDragHandleBlur = () => { this.isDragHandleFocused = false; - // On blur we can clear our last focused - lastFocused = null; + focusRetainer.onDragHandleBlur(); } onMove = (client: Position) => { diff --git a/src/view/draggable/focus-retainer.js b/src/view/draggable/focus-retainer.js new file mode 100644 index 0000000000..e7f6cbf8ae --- /dev/null +++ b/src/view/draggable/focus-retainer.js @@ -0,0 +1,99 @@ +// @flow +import focusOnDragHandle from './focus-on-drag-handle'; +import * as attributes from '../data-attributes'; +import type { DraggableId } from '../../types'; + +type FocusRetainer = {| + onDragHandleFocus: (draggableId: DraggableId) => void, + onDragHandleBlur: () => void, + tryRestoreFocus: (draggableId: DraggableId, draggableRef: HTMLElement) => void, +|} + +// our shared state +let retainingFocusFor: ?DraggableId = null; + +// If we focus on +const clearRetentionOnFocusShift = (() => { + let isBound: boolean = false; + + const bind = () => { + if (isBound) { + return; + } + + isBound = true; + // eslint-disable-next-line no-use-before-define + window.addEventListener('focus', onWindowFocusChange, { capture: true }); + }; + + const unbind = () => { + if (!isBound) { + return; + } + + isBound = false; + // eslint-disable-next-line no-use-before-define + window.removeEventListener('focus', onWindowFocusChange, { capture: true }); + }; + + // focusin will fire after the focus event fires on the element + const onWindowFocusChange = (event: FocusEvent) => { + // unbinding self after single use + unbind(); + + if (!(event.target instanceof HTMLElement)) { + // we are not focusing on a drag handle + retainingFocusFor = null; + return; + } + + const isADragHandle: boolean = Boolean(event.target.getAttribute(attributes.dragHandle)); + + // The focus has shifted away from a drag handle - we can clear our retention + if (!isADragHandle) { + retainingFocusFor = null; + } + }; + + const result = () => bind(); + result.cancel = () => unbind(); + + return result; +})(); + +const clearRetention = () => { + retainingFocusFor = null; + // no need to clear it - we are already clearing it + clearRetentionOnFocusShift.cancel(); +}; + +const onDragHandleFocus = (id: DraggableId) => { + retainingFocusFor = id; + clearRetentionOnFocusShift(); +}; + +const onDragHandleBlur = () => clearRetention(); + +const tryRestoreFocus = (id: DraggableId, draggableRef: HTMLElement) => { + // Not needing to retain focus + if (!retainingFocusFor) { + return; + } + // Not needing to retain focus for this draggable + if (id !== retainingFocusFor) { + return; + } + + // We are about to force force onto a drag handle + + clearRetention(); + focusOnDragHandle(draggableRef); +}; + +const retainer: FocusRetainer = { + onDragHandleFocus, + onDragHandleBlur, + tryRestoreFocus, +}; + +export default retainer; diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index bb2a0dd7ff..509cdec2a4 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -1454,8 +1454,8 @@ describe('Draggable - unconnected', () => { it('should not give focus to a mounting draggable that did not have the last focused id', () => { const firstWrapper = mountDraggable(); - const firstNode: HTMLElement = firstWrapper.getDOMNode(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); // Originally does not have focus expect(firstNode).not.toBe(document.activeElement); @@ -1482,6 +1482,48 @@ describe('Draggable - unconnected', () => { firstWrapper.unmount(); secondWrapper.unmount(); }); + + it('should not give focus draggable if something else on the page has been focused on', () => { + const body: ?HTMLElement = document.body; + if (!body) { + throw new Error('document.body required for test'); + } + const button: HTMLButtonElement = document.createElement('button'); + body.appendChild(button); + + const firstWrapper = mountDraggable(); + const firstNode: HTMLElement = firstWrapper.getDOMNode(); + + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); + + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + firstWrapper.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); + + // unmounting original + firstWrapper.unmount(); + + // now focusing on something else + button.focus(); + expect(button).toBe(document.activeElement); + + const secondWrapper = mountDraggable(); + const secondNode: HTMLElement = secondWrapper.getDOMNode(); + + // does not get focus as button has it + expect(secondNode).not.toBe(document.activeElement); + expect(button).toBe(document.activeElement); + + // cleanup + firstWrapper.unmount(); + secondWrapper.unmount(); + button.blur(); + body.removeChild(button); + }); }); describe('draggable is not the drag handle', () => { From 842dfa94e0bdb1e8d9f668305073a0eaf0c0e427 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 16:28:33 +1000 Subject: [PATCH 08/17] improving comment --- src/view/draggable/focus-retainer.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/view/draggable/focus-retainer.js b/src/view/draggable/focus-retainer.js index e7f6cbf8ae..7335a75013 100644 --- a/src/view/draggable/focus-retainer.js +++ b/src/view/draggable/focus-retainer.js @@ -22,6 +22,9 @@ const clearRetentionOnFocusShift = (() => { } isBound = true; + // Using capture: true as focus events do not bubble + // Additionally doing this prevents us from intercepting the initial + // focus event as it does not bubble up to this listener // eslint-disable-next-line no-use-before-define window.addEventListener('focus', onWindowFocusChange, { capture: true }); }; From d81b7aa03f217c43d3686bd768ec0e8661719da7 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 16:58:28 +1000 Subject: [PATCH 09/17] more tests --- test/unit/view/unconnected-draggable.spec.js | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 509cdec2a4..285a7b70a5 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -1404,6 +1404,47 @@ describe('Draggable - unconnected', () => { second.unmount(); }); + it('should maintain focus if another component is mounted before the focused component', () => { + const first = mountDraggable(); + const firstNode: HTMLElement = first.getDOMNode(); + + // Originally does not have focus + expect(firstNode).not.toBe(document.activeElement); + + // Giving focus to draggable + firstNode.focus(); + // Ensuring that the focus event handler is called + first.simulate('focus'); + // Asserting that it is now the focused element + expect(firstNode).toBe(document.activeElement); + + // unmounting original + first.unmount(); + + // mounting another draggable that should not take focus + const otherProps: OwnProps = { + ...defaultOwnProps, + draggableId: 'my cool new id', + }; + const other = mountDraggable({ + ownProps: otherProps, + }); + const otherNode: HTMLElement = other.getDOMNode(); + expect(otherNode).not.toBe(document.activeElement); + + // Now when we mount the component that should get focus + + const second = mountDraggable(); + const secondNode: HTMLElement = second.getDOMNode(); + + expect(secondNode).toBe(document.activeElement); + + // cleanup + looseFocus(second); + second.unmount(); + other.unmount(); + }); + it('should not maintain focus if it did not have it when moving into a new list', () => { const wrapper = mountDraggable(); const node: HTMLElement = wrapper.getDOMNode(); From 011bf84eb2d133ac45fd93d30b500f6926202abd Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 17:02:43 +1000 Subject: [PATCH 10/17] adding comment --- test/unit/view/unconnected-draggable.spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 285a7b70a5..b3ce700d1b 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -1567,6 +1567,7 @@ describe('Draggable - unconnected', () => { }); }); + // A lighter set of tests describe('draggable is not the drag handle', () => { it('should restore focus after an unmount', () => { const original = mountDraggable({ From b72ef064c2a2ef3d67c31fd319a793e06906bdbe Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 17:18:15 +1000 Subject: [PATCH 11/17] simplication --- src/view/draggable/draggable.jsx | 16 +++++++- src/view/draggable/focus-retainer.js | 41 ++++++-------------- test/unit/view/unconnected-draggable.spec.js | 2 +- 3 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index 8a69ded267..a4253b4236 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -93,6 +93,20 @@ export default class Draggable extends Component { componentWillUnmount() { // releasing reference to ref for cleanup this.ref = null; + + // Was not focused + if (!this.isDragHandleFocused) { + return; + } + + const wasDragging: boolean = this.props.isDragging || this.props.isDropAnimating; + + if (!wasDragging) { + return; + } + + // Attempting to retain focus when moving between lists + focusRetainer.retain(this.props.draggableId); } // This should already be handled gracefully in DragHandle. @@ -134,12 +148,10 @@ export default class Draggable extends Component { onDragHandleFocus = () => { this.isDragHandleFocused = true; - focusRetainer.onDragHandleFocus(this.props.draggableId); } onDragHandleBlur = () => { this.isDragHandleFocused = false; - focusRetainer.onDragHandleBlur(); } onMove = (client: Position) => { diff --git a/src/view/draggable/focus-retainer.js b/src/view/draggable/focus-retainer.js index 7335a75013..c30a4faa9e 100644 --- a/src/view/draggable/focus-retainer.js +++ b/src/view/draggable/focus-retainer.js @@ -4,8 +4,7 @@ import * as attributes from '../data-attributes'; import type { DraggableId } from '../../types'; type FocusRetainer = {| - onDragHandleFocus: (draggableId: DraggableId) => void, - onDragHandleBlur: () => void, + retain: (draggableId: DraggableId) => void, tryRestoreFocus: (draggableId: DraggableId, draggableRef: HTMLElement) => void, |} @@ -13,7 +12,7 @@ type FocusRetainer = {| let retainingFocusFor: ?DraggableId = null; // If we focus on -const clearRetentionOnFocusShift = (() => { +const clearRetentionOnFocusChange = (() => { let isBound: boolean = false; const bind = () => { @@ -40,22 +39,11 @@ const clearRetentionOnFocusShift = (() => { }; // focusin will fire after the focus event fires on the element - const onWindowFocusChange = (event: FocusEvent) => { + const onWindowFocusChange = () => { + console.log('ON WINDOW FOCUS CHANGE'); // unbinding self after single use unbind(); - - if (!(event.target instanceof HTMLElement)) { - // we are not focusing on a drag handle - retainingFocusFor = null; - return; - } - - const isADragHandle: boolean = Boolean(event.target.getAttribute(attributes.dragHandle)); - - // The focus has shifted away from a drag handle - we can clear our retention - if (!isADragHandle) { - retainingFocusFor = null; - } + retainingFocusFor = null; }; const result = () => bind(); @@ -64,19 +52,11 @@ const clearRetentionOnFocusShift = (() => { return result; })(); -const clearRetention = () => { - retainingFocusFor = null; - // no need to clear it - we are already clearing it - clearRetentionOnFocusShift.cancel(); -}; - -const onDragHandleFocus = (id: DraggableId) => { +const retain = (id: DraggableId) => { retainingFocusFor = id; - clearRetentionOnFocusShift(); + clearRetentionOnFocusChange(); }; -const onDragHandleBlur = () => clearRetention(); - const tryRestoreFocus = (id: DraggableId, draggableRef: HTMLElement) => { // Not needing to retain focus if (!retainingFocusFor) { @@ -89,13 +69,14 @@ const tryRestoreFocus = (id: DraggableId, draggableRef: HTMLElement) => { // We are about to force force onto a drag handle - clearRetention(); + retainingFocusFor = null; + // no need to clear it - we are already clearing it + clearRetentionOnFocusChange.cancel(); focusOnDragHandle(draggableRef); }; const retainer: FocusRetainer = { - onDragHandleFocus, - onDragHandleBlur, + retain, tryRestoreFocus, }; diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index b3ce700d1b..1dfe04f3ce 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -1376,7 +1376,7 @@ describe('Draggable - unconnected', () => { describe('Cross list movement focus retention', () => { describe('draggable is the drag handle', () => { - it('should maintain focus when mounted into a different list', () => { + it('should maintain focus when moving to a different list', () => { const first = mountDraggable(); const original: HTMLElement = first.getDOMNode(); From aec9d2f558969f340dd7ec2d46100283a021b25e Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 18 Apr 2018 21:53:24 +1000 Subject: [PATCH 12/17] moving focus management to drag handle --- src/view/drag-handle/drag-handle-types.js | 4 +- src/view/drag-handle/drag-handle.jsx | 81 ++++++++++++++++--- .../sensor/create-keyboard-sensor.js | 4 +- .../drag-handle/sensor/create-mouse-sensor.js | 4 +- .../drag-handle/sensor/create-touch-sensor.js | 4 +- src/view/drag-handle/sensor/sensor-types.js | 1 + .../util}/focus-on-drag-handle.js | 2 +- .../util}/focus-retainer.js | 4 +- src/view/draggable/draggable.jsx | 39 +-------- test/unit/view/unconnected-draggable.spec.js | 3 + 10 files changed, 81 insertions(+), 65 deletions(-) rename src/view/{draggable => drag-handle/util}/focus-on-drag-handle.js (93%) rename src/view/{draggable => drag-handle/util}/focus-retainer.js (93%) diff --git a/src/view/drag-handle/drag-handle-types.js b/src/view/drag-handle/drag-handle-types.js index a577a2db8d..2019d26670 100644 --- a/src/view/drag-handle/drag-handle-types.js +++ b/src/view/drag-handle/drag-handle-types.js @@ -8,8 +8,6 @@ import type { } from '../../types'; export type Callbacks = {| - onFocus: () => void, - onBlur: () => void, onLift: ({ client: Position, autoScrollMode: AutoScrollMode }) => void, onMove: (point: Position) => void, onWindowScroll: () => void, @@ -54,6 +52,8 @@ export type Props = {| isEnabled: boolean, // whether the application thinks a drag is occurring isDragging: boolean, + // whether the application thinks a drop is occurring + isDropAnimating: boolean, // the direction of the current droppable direction: ?Direction, // get the ref of the draggable diff --git a/src/view/drag-handle/drag-handle.jsx b/src/view/drag-handle/drag-handle.jsx index 654ef039c7..64397f09f4 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -2,6 +2,7 @@ import { Component } from 'react'; import PropTypes from 'prop-types'; import memoizeOne from 'memoize-one'; +import getWindowFromRef from '../get-window-from-ref'; import type { Props, DragHandleProps, @@ -12,10 +13,12 @@ import type { TouchSensor, CreateSensorArgs, } from './sensor/sensor-types'; +import focusOnDragHandle from './util/focus-on-drag-handle'; import type { DraggableId, } from '../../types'; import { styleContextKey, canLiftContextKey } from '../context-keys'; +import focusRetainer from './util/focus-retainer'; import shouldAllowDraggingFromTarget from './util/should-allow-dragging-from-target'; import createMouseSensor from './sensor/create-mouse-sensor'; import createKeyboardSensor from './sensor/create-keyboard-sensor'; @@ -35,6 +38,8 @@ export default class DragHandle extends Component { sensors: Sensor[]; styleContext: string; canLift: (id: DraggableId) => boolean; + isFocused: boolean = false; + lastDraggableRef: ?HTMLElement; // Need to declare contextTypes without flow // https://github.com/brigand/babel-plugin-flow-react-proptypes/issues/22 @@ -46,9 +51,12 @@ export default class DragHandle extends Component { constructor(props: Props, context: Object) { super(props, context); + const getWindow = (): HTMLElement => getWindowFromRef(this.props.getDraggableRef()); + const args: CreateSensorArgs = { callbacks: this.props.callbacks, getDraggableRef: this.props.getDraggableRef, + getWindow, canStartCapturing: this.canStartCapturing, }; @@ -71,20 +79,34 @@ export default class DragHandle extends Component { this.canLift = context[canLiftContextKey]; } - componentWillUnmount() { - this.sensors.forEach((sensor: Sensor) => { - // kill the current drag and fire a cancel event if - const wasDragging = sensor.isDragging(); + componentDidMount() { + const draggableRef: ?HTMLElement = this.props.getDraggableRef(); - sensor.unmount(); - // cancel if drag was occurring - if (wasDragging) { - this.props.callbacks.onCancel(); - } - }); + // storing a reference for later + this.lastDraggableRef = draggableRef; + + if (!draggableRef) { + console.error('Cannot get draggable ref from drag handle'); + return; + } + + focusRetainer.tryRestoreFocus(this.props.draggableId, draggableRef); } componentDidUpdate(prevProps: Props) { + const ref: ?HTMLElement = this.props.getDraggableRef(); + + if (ref !== this.lastDraggableRef) { + this.lastDraggableRef = ref; + + // After a ref change we might need to manually force focus onto the ref. + // When moving something into or out of a portal the element looses focus + // https://github.com/facebook/react/issues/12454 + if (ref && this.isFocused) { + focusOnDragHandle(ref); + } + } + const isCapturing: boolean = this.isAnySensorCapturing(); if (!isCapturing) { @@ -122,6 +144,41 @@ export default class DragHandle extends Component { } } + componentWillUnmount() { + this.sensors.forEach((sensor: Sensor) => { + // kill the current drag and fire a cancel event if + const wasDragging = sensor.isDragging(); + + sensor.unmount(); + // cancel if drag was occurring + if (wasDragging) { + this.props.callbacks.onCancel(); + } + }); + + const shouldRetainFocus: boolean = (() => { + // not already focused + if (!this.isFocused) { + return false; + } + + // a drag is finishing + return (this.props.isDragging || this.props.isDropAnimating); + })(); + + if (shouldRetainFocus) { + focusRetainer.retain(this.props.draggableId); + } + } + + onFocus = () => { + this.isFocused = true; + } + + onBlur = () => { + this.isFocused = false; + } + onKeyDown = (event: KeyboardEvent) => { // let the mouse sensor deal with it if (this.mouseSensor.isCapturing()) { @@ -177,8 +234,8 @@ export default class DragHandle extends Component { onMouseDown: this.onMouseDown, onKeyDown: this.onKeyDown, onTouchStart: this.onTouchStart, - onFocus: this.props.callbacks.onFocus, - onBlur: this.props.callbacks.onBlur, + onFocus: this.onFocus, + onBlur: this.onBlur, tabIndex: 0, 'data-react-beautiful-dnd-drag-handle': this.styleContext, // English default. Consumers are welcome to add their own start instruction diff --git a/src/view/drag-handle/sensor/create-keyboard-sensor.js b/src/view/drag-handle/sensor/create-keyboard-sensor.js index b88d12618f..f6da110a26 100644 --- a/src/view/drag-handle/sensor/create-keyboard-sensor.js +++ b/src/view/drag-handle/sensor/create-keyboard-sensor.js @@ -3,7 +3,6 @@ import createScheduler from '../util/create-scheduler'; import preventStandardKeyEvents from '../util/prevent-standard-key-events'; import * as keyCodes from '../../key-codes'; -import getWindowFromRef from '../../get-window-from-ref'; import getCenterPosition from '../../get-center-position'; import { bindEvents, unbindEvents } from '../util/bind-events'; import supportedPageVisibilityEventName from '../util/supported-page-visibility-event-name'; @@ -38,6 +37,7 @@ const noop = () => { }; export default ({ callbacks, + getWindow, getDraggableRef, canStartCapturing, }: CreateSensorArgs): KeyboardSensor => { @@ -47,8 +47,6 @@ export default ({ const setState = (newState: State): void => { state = newState; }; - const getWindow = (): HTMLElement => getWindowFromRef(getDraggableRef()); - const startDragging = (fn?: Function = noop) => { setState({ isDragging: true, diff --git a/src/view/drag-handle/sensor/create-mouse-sensor.js b/src/view/drag-handle/sensor/create-mouse-sensor.js index 3b02945476..609df48532 100644 --- a/src/view/drag-handle/sensor/create-mouse-sensor.js +++ b/src/view/drag-handle/sensor/create-mouse-sensor.js @@ -2,7 +2,6 @@ /* eslint-disable no-use-before-define */ import createScheduler from '../util/create-scheduler'; import isSloppyClickThresholdExceeded from '../util/is-sloppy-click-threshold-exceeded'; -import getWindowFromRef from '../../get-window-from-ref'; import * as keyCodes from '../../key-codes'; import preventStandardKeyEvents from '../util/prevent-standard-key-events'; import createPostDragEventPreventer, { type EventPreventer } from '../util/create-post-drag-event-preventer'; @@ -34,7 +33,7 @@ const mouseDownMarshal: EventMarshal = createEventMarshal(); export default ({ callbacks, - getDraggableRef, + getWindow, canStartCapturing, }: CreateSensorArgs): MouseSensor => { let state: State = { @@ -47,7 +46,6 @@ export default ({ const isDragging = (): boolean => state.isDragging; const isCapturing = (): boolean => Boolean(state.pending || state.isDragging); const schedule = createScheduler(callbacks); - const getWindow = (): HTMLElement => getWindowFromRef(getDraggableRef()); const postDragEventPreventer: EventPreventer = createPostDragEventPreventer(getWindow); const startDragging = (fn?: Function = noop) => { diff --git a/src/view/drag-handle/sensor/create-touch-sensor.js b/src/view/drag-handle/sensor/create-touch-sensor.js index 25caedf98f..133788e753 100644 --- a/src/view/drag-handle/sensor/create-touch-sensor.js +++ b/src/view/drag-handle/sensor/create-touch-sensor.js @@ -1,7 +1,6 @@ // @flow /* eslint-disable no-use-before-define */ import createScheduler from '../util/create-scheduler'; -import getWindowFromRef from '../../get-window-from-ref'; import createPostDragEventPreventer, { type EventPreventer } from '../util/create-post-drag-event-preventer'; import createEventMarshal, { type EventMarshal } from '../util/create-event-marshal'; import { bindEvents, unbindEvents } from '../util/bind-events'; @@ -98,12 +97,11 @@ const initial: State = { export default ({ callbacks, - getDraggableRef, + getWindow, canStartCapturing, }: CreateSensorArgs): TouchSensor => { let state: State = initial; - const getWindow = (): HTMLElement => getWindowFromRef(getDraggableRef()); const setState = (partial: Object): void => { state = { ...state, diff --git a/src/view/drag-handle/sensor/sensor-types.js b/src/view/drag-handle/sensor/sensor-types.js index 94c372c161..953d048319 100644 --- a/src/view/drag-handle/sensor/sensor-types.js +++ b/src/view/drag-handle/sensor/sensor-types.js @@ -16,6 +16,7 @@ type SensorBase = {| export type CreateSensorArgs = {| callbacks: Callbacks, getDraggableRef: () => ?HTMLElement, + getWindow: () => HTMLElement, canStartCapturing: (event: Event) => boolean, |} diff --git a/src/view/draggable/focus-on-drag-handle.js b/src/view/drag-handle/util/focus-on-drag-handle.js similarity index 93% rename from src/view/draggable/focus-on-drag-handle.js rename to src/view/drag-handle/util/focus-on-drag-handle.js index 21a626a365..6c581d0b92 100644 --- a/src/view/draggable/focus-on-drag-handle.js +++ b/src/view/drag-handle/util/focus-on-drag-handle.js @@ -1,5 +1,5 @@ // @flow -import { dragHandle } from '../data-attributes'; +import { dragHandle } from '../../data-attributes'; const selector: string = `[${dragHandle}]`; diff --git a/src/view/draggable/focus-retainer.js b/src/view/drag-handle/util/focus-retainer.js similarity index 93% rename from src/view/draggable/focus-retainer.js rename to src/view/drag-handle/util/focus-retainer.js index c30a4faa9e..c816cd7943 100644 --- a/src/view/draggable/focus-retainer.js +++ b/src/view/drag-handle/util/focus-retainer.js @@ -1,7 +1,6 @@ // @flow import focusOnDragHandle from './focus-on-drag-handle'; -import * as attributes from '../data-attributes'; -import type { DraggableId } from '../../types'; +import type { DraggableId } from '../../../types'; type FocusRetainer = {| retain: (draggableId: DraggableId) => void, @@ -40,7 +39,6 @@ const clearRetentionOnFocusChange = (() => { // focusin will fire after the focus event fires on the element const onWindowFocusChange = () => { - console.log('ON WINDOW FOCUS CHANGE'); // unbinding self after single use unbind(); retainingFocusFor = null; diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index a4253b4236..659efa5c7f 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -6,7 +6,6 @@ import memoizeOne from 'memoize-one'; import invariant from 'tiny-invariant'; import type { Position, - DraggableId, DraggableDimension, InitialDragPositions, DroppableId, @@ -15,8 +14,6 @@ import type { import DraggableDimensionPublisher from '../draggable-dimension-publisher/'; import Moveable from '../moveable/'; import DragHandle from '../drag-handle'; -import focusRetainer from './focus-retainer'; -import focusOnDragHandle from './focus-on-drag-handle'; import getViewport from '../window/get-viewport'; // eslint-disable-next-line no-duplicate-imports import type { @@ -47,7 +44,6 @@ export default class Draggable extends Component { /* eslint-disable react/sort-comp */ callbacks: DragHandleCallbacks styleContext: string - isDragHandleFocused: boolean = false ref: ?HTMLElement = null // Need to declare contextTypes without flow @@ -61,8 +57,6 @@ export default class Draggable extends Component { super(props, context); const callbacks: DragHandleCallbacks = { - onFocus: this.onDragHandleFocus, - onBlur: this.onDragHandleBlur, onLift: this.onLift, onMove: this.onMove, onDrop: this.onDrop, @@ -84,29 +78,12 @@ export default class Draggable extends Component { Draggable has not been provided with a ref. Please use the DraggableProvided > innerRef function `); - return; } - - focusRetainer.tryRestoreFocus(this.props.draggableId, this.ref); } componentWillUnmount() { // releasing reference to ref for cleanup this.ref = null; - - // Was not focused - if (!this.isDragHandleFocused) { - return; - } - - const wasDragging: boolean = this.props.isDragging || this.props.isDropAnimating; - - if (!wasDragging) { - return; - } - - // Attempting to retain focus when moving between lists - focusRetainer.retain(this.props.draggableId); } // This should already be handled gracefully in DragHandle. @@ -146,14 +123,6 @@ export default class Draggable extends Component { lift(draggableId, initial, getViewport(), autoScrollMode); } - onDragHandleFocus = () => { - this.isDragHandleFocused = true; - } - - onDragHandleBlur = () => { - this.isDragHandleFocused = false; - } - onMove = (client: Position) => { this.throwIfCannotDrag(); @@ -217,13 +186,6 @@ export default class Draggable extends Component { // At this point the ref has been changed or initially populated this.ref = ref; - - // After a ref change we might need to manually force focus onto the ref. - // When moving something into or out of a portal the element looses focus - // https://github.com/facebook/react/issues/12454 - if (this.ref && this.isDragHandleFocused) { - focusOnDragHandle(this.ref); - } }) getDraggableRef = (): ?HTMLElement => this.ref; @@ -408,6 +370,7 @@ export default class Draggable extends Component { { // Asserting that it is now the focused element expect(original).toBe(document.activeElement); + // now dragging + executeOnLift(first); + // unmounting original first.unmount(); From c8e21ef607810b2b6996bbcf9b99abc051084a21 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 07:43:26 +1000 Subject: [PATCH 13/17] adding new tests --- test/unit/view/drag-handle.spec.js | 19 ++- .../view/drag-handle/focus-management.spec.js | 125 ++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 test/unit/view/drag-handle/focus-management.spec.js diff --git a/test/unit/view/drag-handle.spec.js b/test/unit/view/drag-handle.spec.js index 97817afff8..bae8b44d49 100644 --- a/test/unit/view/drag-handle.spec.js +++ b/test/unit/view/drag-handle.spec.js @@ -31,8 +31,6 @@ const primaryButton: number = 0; const auxiliaryButton: number = 1; const getStubCallbacks = (): Callbacks => ({ - onFocus: jest.fn(), - onBlur: jest.fn(), onLift: jest.fn(), onMove: jest.fn(), onMoveForward: jest.fn(), @@ -182,6 +180,7 @@ const getNestedWrapper = (parentCallbacks: Callbacks, childCallbacks: Callbacks) callbacks={parentCallbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => parentRef} canDragInteractiveElements={false} @@ -193,6 +192,7 @@ const getNestedWrapper = (parentCallbacks: Callbacks, childCallbacks: Callbacks) callbacks={childCallbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => childRef} canDragInteractiveElements={false} @@ -216,6 +216,7 @@ const getWrapper = (callbacks: Callbacks): ReactWrapper => callbacks={callbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -266,6 +267,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -291,6 +293,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -324,6 +327,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -1497,6 +1501,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction="vertical" getDraggableRef={() => singleRef} @@ -1560,6 +1565,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -1663,6 +1669,7 @@ describe('drag handle', () => { callbacks={customCallbacks} direction="horizontal" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -2682,6 +2689,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled={false} isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -3065,6 +3073,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3100,6 +3109,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3138,6 +3148,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3181,6 +3192,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3227,6 +3239,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3269,6 +3282,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3323,6 +3337,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} diff --git a/test/unit/view/drag-handle/focus-management.spec.js b/test/unit/view/drag-handle/focus-management.spec.js new file mode 100644 index 0000000000..dc9baf3e56 --- /dev/null +++ b/test/unit/view/drag-handle/focus-management.spec.js @@ -0,0 +1,125 @@ +// @flow +import React from 'react'; +import { mount } from 'enzyme'; +import type { ReactWrapper } from 'enzyme'; +import DragHandle from '../../../../src/view/drag-handle'; +import { styleContextKey, canLiftContextKey } from '../../../../src/view/context-keys'; +import forceUpdate from '../../../utils/force-update'; +import type { Callbacks, DragHandleProps } from '../../../../src/view/drag-handle/drag-handle-types'; + +const getStubCallbacks = (): Callbacks => ({ + onLift: jest.fn(), + onMove: jest.fn(), + onMoveForward: jest.fn(), + onMoveBackward: jest.fn(), + onCrossAxisMoveForward: jest.fn(), + onCrossAxisMoveBackward: jest.fn(), + onDrop: jest.fn(), + onCancel: jest.fn(), + onWindowScroll: jest.fn(), +}); + +type ChildProps = {| + dragHandleProps: ?DragHandleProps +|} + +class Child extends React.Component { + render() { + return ( +
+ Drag me! +
+ ); + } +} + +const basicContext = { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => true, +}; + +const body: ?HTMLElement = document.body; + +if (!body) { + throw new Error('document.body not found'); +} + +const first: HTMLElement = document.createElement('div'); +first.setAttribute('id', 'first'); +body.appendChild(first); +const second: HTMLElement = document.createElement('div'); +second.setAttribute('id', 'second'); +body.appendChild(second); + +describe('ref changes (dragging in a portal)', () => { + it('should retain focus if draggable ref is changing and had focus', () => { + // setting our active element to be the first + let dragHandleRef: HTMLElement = first; + + const wrapper: ReactWrapper = mount( + dragHandleRef} + canDragInteractiveElements={false} + > + {(props: ?DragHandleProps) => } + , + { context: basicContext } + ); + + // does not start with focus + expect(first).not.toBe(document.activeElement); + + // we give the first element focus + first.focus(); + wrapper.simulate('focus'); + // expect(first).toBe(document.activeElement); + // expect(second).not.toBe(document.activeElement); + + // changing our drag handle ref + dragHandleRef = second; + + // forcing a re-render + forceUpdate(wrapper); + + // first element has lost focus + expect(first).not.toBe(document.activeElement); + // second element now has focus + expect(second).toBe(document.activeElement); + }); + + it('should not retain focus if draggable ref is changing and did not have focus', () => { + + }); +}); + +describe('between mounts (allowing maintaining focus across list transitions)', () => { + it('should maintain focus if unmounting while dragging', () => { + + }); + + it('should maintain focus if unmounting while drop animating', () => { + + }); + + it('should not give focus to something that was not previously focused', () => { + + }); + + it('should maintain focus if another component is mounted before the focused component', () => { + + }); + + it('should not obtain focus if focus is lost before unmount and remount', () => { + + }); + + it('should not give focus if something else on the page has been focused on after unmount', () => { + + }); +}); From f44925fc2f1d211f0da947a5f5715730f705d592 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 09:49:32 +1000 Subject: [PATCH 14/17] finishing tests --- src/view/drag-handle/drag-handle.jsx | 7 +- src/view/drag-handle/util/focus-retainer.js | 4 +- ...-drag-handle.js => get-drag-handle-ref.js} | 16 +- .../view/drag-handle/focus-management.spec.js | 362 +++++++++++++++--- test/unit/view/unconnected-draggable.spec.js | 247 +----------- 5 files changed, 315 insertions(+), 321 deletions(-) rename src/view/drag-handle/util/{focus-on-drag-handle.js => get-drag-handle-ref.js} (51%) diff --git a/src/view/drag-handle/drag-handle.jsx b/src/view/drag-handle/drag-handle.jsx index 64397f09f4..5e3ba6ee97 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -3,6 +3,7 @@ import { Component } from 'react'; import PropTypes from 'prop-types'; import memoizeOne from 'memoize-one'; import getWindowFromRef from '../get-window-from-ref'; +import getDragHandleRef from './util/get-drag-handle-ref'; import type { Props, DragHandleProps, @@ -13,7 +14,6 @@ import type { TouchSensor, CreateSensorArgs, } from './sensor/sensor-types'; -import focusOnDragHandle from './util/focus-on-drag-handle'; import type { DraggableId, } from '../../types'; @@ -90,12 +90,11 @@ export default class DragHandle extends Component { return; } - focusRetainer.tryRestoreFocus(this.props.draggableId, draggableRef); + focusRetainer.tryRestoreFocus(this.props.draggableId, getDragHandleRef(draggableRef)); } componentDidUpdate(prevProps: Props) { const ref: ?HTMLElement = this.props.getDraggableRef(); - if (ref !== this.lastDraggableRef) { this.lastDraggableRef = ref; @@ -103,7 +102,7 @@ export default class DragHandle extends Component { // When moving something into or out of a portal the element looses focus // https://github.com/facebook/react/issues/12454 if (ref && this.isFocused) { - focusOnDragHandle(ref); + getDragHandleRef(ref).focus(); } } diff --git a/src/view/drag-handle/util/focus-retainer.js b/src/view/drag-handle/util/focus-retainer.js index c816cd7943..90ef2b69ff 100644 --- a/src/view/drag-handle/util/focus-retainer.js +++ b/src/view/drag-handle/util/focus-retainer.js @@ -1,5 +1,5 @@ // @flow -import focusOnDragHandle from './focus-on-drag-handle'; +import getDragHandleRef from './get-drag-handle-ref'; import type { DraggableId } from '../../../types'; type FocusRetainer = {| @@ -70,7 +70,7 @@ const tryRestoreFocus = (id: DraggableId, draggableRef: HTMLElement) => { retainingFocusFor = null; // no need to clear it - we are already clearing it clearRetentionOnFocusChange.cancel(); - focusOnDragHandle(draggableRef); + getDragHandleRef(draggableRef).focus(); }; const retainer: FocusRetainer = { diff --git a/src/view/drag-handle/util/focus-on-drag-handle.js b/src/view/drag-handle/util/get-drag-handle-ref.js similarity index 51% rename from src/view/drag-handle/util/focus-on-drag-handle.js rename to src/view/drag-handle/util/get-drag-handle-ref.js index 6c581d0b92..b9bced3fdd 100644 --- a/src/view/drag-handle/util/focus-on-drag-handle.js +++ b/src/view/drag-handle/util/get-drag-handle-ref.js @@ -1,9 +1,10 @@ // @flow +import invariant from 'tiny-invariant'; import { dragHandle } from '../../data-attributes'; const selector: string = `[${dragHandle}]`; -const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => { +const getDragHandleRef = (draggableRef: HTMLElement): HTMLElement => { if (draggableRef.hasAttribute(dragHandle)) { return draggableRef; } @@ -13,16 +14,9 @@ const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => { // https://codepen.io/alexreardon/pen/erOqyZ const el: ?HTMLElement = draggableRef.querySelector(selector); - return el || null; -}; + invariant(el, 'Could not find draggable ref'); -const focusOnDragHandle = (draggableRef: HTMLElement) => { - const dragHandleRef: ?HTMLElement = getDragHandleRef(draggableRef); - if (!dragHandleRef) { - console.error('Draggable cannot focus on the drag handle as it cannot be found'); - return; - } - dragHandleRef.focus(); + return el; }; -export default focusOnDragHandle; +export default getDragHandleRef; diff --git a/test/unit/view/drag-handle/focus-management.spec.js b/test/unit/view/drag-handle/focus-management.spec.js index dc9baf3e56..7b51b24ba8 100644 --- a/test/unit/view/drag-handle/focus-management.spec.js +++ b/test/unit/view/drag-handle/focus-management.spec.js @@ -1,10 +1,11 @@ // @flow -import React from 'react'; +import React, { type Node } from 'react'; +import ReactDOM from 'react-dom'; +import PropTypes from 'prop-types'; import { mount } from 'enzyme'; import type { ReactWrapper } from 'enzyme'; import DragHandle from '../../../../src/view/drag-handle'; import { styleContextKey, canLiftContextKey } from '../../../../src/view/context-keys'; -import forceUpdate from '../../../utils/force-update'; import type { Callbacks, DragHandleProps } from '../../../../src/view/drag-handle/drag-handle-types'; const getStubCallbacks = (): Callbacks => ({ @@ -19,23 +20,15 @@ const getStubCallbacks = (): Callbacks => ({ onWindowScroll: jest.fn(), }); -type ChildProps = {| - dragHandleProps: ?DragHandleProps -|} - -class Child extends React.Component { - render() { - return ( -
- Drag me! -
- ); - } -} - -const basicContext = { - [styleContextKey]: 'hello', - [canLiftContextKey]: () => true, +const options = { + context: { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => true, + }, + childContextTypes: { + [styleContextKey]: PropTypes.string.isRequired, + [canLiftContextKey]: PropTypes.func.isRequired, + }, }; const body: ?HTMLElement = document.body; @@ -44,82 +37,329 @@ if (!body) { throw new Error('document.body not found'); } -const first: HTMLElement = document.createElement('div'); -first.setAttribute('id', 'first'); -body.appendChild(first); -const second: HTMLElement = document.createElement('div'); -second.setAttribute('id', 'second'); -body.appendChild(second); +describe('Portal usage (ref changing while mounted)', () => { + type ChildProps = {| + dragHandleProps: ?DragHandleProps, + usePortal: boolean, + setRef: (ref: ?HTMLElement) => void, + |} + class Child extends React.Component { + // eslint-disable-next-line react/sort-comp + portal: ?HTMLElement; + + componentDidMount() { + this.portal = document.createElement('div'); + body.appendChild(this.portal); + } + + componentWillUnmount() { + if (!this.portal) { + return; + } + body.removeChild(this.portal); + this.portal = null; + } + + render() { + const child: Node = ( +
+ Drag me! +
+ ); + + if (this.portal && this.props.usePortal) { + return ReactDOM.createPortal(child, this.portal); + } + + return child; + } + } + class WithParentRefAndPortalChild extends React.Component<{ usePortal: boolean }> { + // eslint-disable-next-line react/sort-comp + ref: ?HTMLElement; + + setRef = (ref: ?HTMLElement) => { + this.ref = ref; + } + + render() { + return ( + this.ref} + canDragInteractiveElements={false} + > + {(dragHandleProps: ?DragHandleProps) => ( + + )} + + ); + } + } -describe('ref changes (dragging in a portal)', () => { it('should retain focus if draggable ref is changing and had focus', () => { - // setting our active element to be the first - let dragHandleRef: HTMLElement = first; - - const wrapper: ReactWrapper = mount( - dragHandleRef} - canDragInteractiveElements={false} - > - {(props: ?DragHandleProps) => } - , - { context: basicContext } - ); + const wrapper = mount(, options); - // does not start with focus - expect(first).not.toBe(document.activeElement); + const original: HTMLElement = wrapper.getDOMNode(); + expect(original).not.toBe(document.activeElement); - // we give the first element focus - first.focus(); + // giving it focus + original.focus(); wrapper.simulate('focus'); - // expect(first).toBe(document.activeElement); - // expect(second).not.toBe(document.activeElement); + expect(original).toBe(document.activeElement); - // changing our drag handle ref - dragHandleRef = second; + // forcing change of parent ref by moving into a portal + wrapper.setProps({ + usePortal: true, + }); - // forcing a re-render - forceUpdate(wrapper); - - // first element has lost focus - expect(first).not.toBe(document.activeElement); - // second element now has focus - expect(second).toBe(document.activeElement); + const inPortal: HTMLElement = wrapper.getDOMNode(); + expect(inPortal).toBe(document.activeElement); + expect(inPortal).not.toBe(original); + expect(original).not.toBe(document.activeElement); }); it('should not retain focus if draggable ref is changing and did not have focus', () => { + const wrapper = mount(, options); + + const original: HTMLElement = wrapper.getDOMNode(); + expect(original).not.toBe(document.activeElement); + // forcing change of parent ref by moving into a portal + wrapper.setProps({ + usePortal: true, + }); + + const inPortal: HTMLElement = wrapper.getDOMNode(); + expect(inPortal).not.toBe(document.activeElement); + expect(inPortal).not.toBe(original); + expect(original).not.toBe(document.activeElement); }); }); -describe('between mounts (allowing maintaining focus across list transitions)', () => { +describe('Focus retention moving between lists (focus retention between mounts)', () => { + type WithParentRefProps = {| + draggableId: string, + isDragging: boolean, + isDropAnimating: boolean + |} + class WithParentRef extends React.Component { + // eslint-disable-next-line react/sort-comp + ref: ?HTMLElement; + + static defaultProps = { + draggableId: 'draggable', + isDragging: false, + isDropAnimating: false, + } + + setRef = (ref: ?HTMLElement) => { + this.ref = ref; + } + + render() { + return ( + this.ref} + canDragInteractiveElements={false} + > + {(dragHandleProps: ?DragHandleProps) => ( +
Drag me!
+ )} + + ); + } + } + it('should maintain focus if unmounting while dragging', () => { + const first: ReactWrapper = mount(, options); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + + first.unmount(); + + const second: ReactWrapper = mount(, options); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).toBe(document.activeElement); + // validation + expect(latest).not.toBe(original); + expect(original).not.toBe(document.activeElement); + // cleanup + second.unmount(); }); it('should maintain focus if unmounting while drop animating', () => { + const first: ReactWrapper = mount(, options); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + + first.unmount(); + + const second: ReactWrapper = mount(, options); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).toBe(document.activeElement); + // validation + expect(latest).not.toBe(original); + expect(original).not.toBe(document.activeElement); + + // cleanup + second.unmount(); + }); + + // This interaction has nothing to do with us! + it('should not maintain focus if the item was not dragging or drop animating', () => { + const first: ReactWrapper = mount( + , options + ); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + + first.unmount(); + + // will not get focus as it was not previously dragging or drop animating + const second: ReactWrapper = mount(, options); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).not.toBe(document.activeElement); + // validation + expect(latest).not.toBe(original); + expect(original).not.toBe(document.activeElement); }); it('should not give focus to something that was not previously focused', () => { + const first: ReactWrapper = mount(, options); + const original: HTMLElement = first.getDOMNode(); + + expect(original).not.toBe(document.activeElement); + first.unmount(); + const second: ReactWrapper = mount(, options); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).not.toBe(document.activeElement); + // validation + expect(latest).not.toBe(original); + expect(original).not.toBe(document.activeElement); + + // cleanup + second.unmount(); }); it('should maintain focus if another component is mounted before the focused component', () => { + const first: ReactWrapper = mount( + , options + ); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + // unmounting the first + first.unmount(); + + // mounting something with a different id + const other: ReactWrapper = mount( + , options + ); + expect(other.getDOMNode()).not.toBe(document.activeElement); + + // mounting something with the same id as the first + const second: ReactWrapper = mount( + , options + ); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).toBe(document.activeElement); + + // cleanup + other.unmount(); + second.unmount(); }); - it('should not obtain focus if focus is lost before unmount and remount', () => { + it('should only maintain focus once', () => { + const first: ReactWrapper = mount(, options); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + + first.unmount(); + // obtaining focus on first remount + const second: ReactWrapper = mount(, options); + const latest: HTMLElement = second.getDOMNode(); + expect(latest).toBe(document.activeElement); + // validation + expect(latest).not.toBe(original); + expect(original).not.toBe(document.activeElement); + + second.unmount(); + + // should not obtain focus on the second remount + const third: ReactWrapper = mount(, options); + expect(third.getDOMNode()).not.toBe(document.activeElement); + + // cleanup + second.unmount(); + third.unmount(); }); it('should not give focus if something else on the page has been focused on after unmount', () => { + const button: HTMLElement = document.createElement('button'); + body.appendChild(button); + const first: ReactWrapper = mount(, options); + const original: HTMLElement = first.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // get focus + original.focus(); + first.find(DragHandle).simulate('focus'); + expect(original).toBe(document.activeElement); + + first.unmount(); + + // a button is focused on + button.focus(); + expect(button).toBe(document.activeElement); + // remount should now not claim focus + const second: ReactWrapper = mount(, options); + expect(second.getDOMNode()).not.toBe(document.activeElement); + // focus maintained on button + expect(button).toBe(document.activeElement); }); }); diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index 39caf932e2..f37aaac40b 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -1374,249 +1374,10 @@ describe('Draggable - unconnected', () => { }); }); - describe('Cross list movement focus retention', () => { - describe('draggable is the drag handle', () => { - it('should maintain focus when moving to a different list', () => { - const first = mountDraggable(); - const original: HTMLElement = first.getDOMNode(); - - // Originally does not have focus - expect(original).not.toBe(document.activeElement); - - // Giving focus to draggable - original.focus(); - // Ensuring that the focus event handler is called - first.simulate('focus'); - // Asserting that it is now the focused element - expect(original).toBe(document.activeElement); - - // now dragging - executeOnLift(first); - - // unmounting original - first.unmount(); - - const second = mountDraggable(); - const last: HTMLElement = second.getDOMNode(); - - expect(last).toBe(document.activeElement); - - // cleanup - looseFocus(second); - first.unmount(); - second.unmount(); - }); - - it('should maintain focus if another component is mounted before the focused component', () => { - const first = mountDraggable(); - const firstNode: HTMLElement = first.getDOMNode(); - - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); - - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - first.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); - - // unmounting original - first.unmount(); - - // mounting another draggable that should not take focus - const otherProps: OwnProps = { - ...defaultOwnProps, - draggableId: 'my cool new id', - }; - const other = mountDraggable({ - ownProps: otherProps, - }); - const otherNode: HTMLElement = other.getDOMNode(); - expect(otherNode).not.toBe(document.activeElement); - - // Now when we mount the component that should get focus - - const second = mountDraggable(); - const secondNode: HTMLElement = second.getDOMNode(); - - expect(secondNode).toBe(document.activeElement); - - // cleanup - looseFocus(second); - second.unmount(); - other.unmount(); - }); - - it('should not maintain focus if it did not have it when moving into a new list', () => { - const wrapper = mountDraggable(); - const node: HTMLElement = wrapper.getDOMNode(); - - // Originally does not have focus - expect(node).not.toBe(document.activeElement); - - const second = mountDraggable(); - const fresh: HTMLElement = second.getDOMNode(); - - // Still does not have focus - expect(fresh).not.toBe(document.activeElement); - - wrapper.unmount(); - }); - - it('should not obtain focus if focus is lost before remount', () => { - const firstWrapper = mountDraggable(); - const firstNode: HTMLElement = firstWrapper.getDOMNode(); - - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); - - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - firstWrapper.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); - - // Now loosing focus - firstNode.blur(); - firstWrapper.simulate('blur'); - expect(firstNode).not.toBe(document.activeElement); - - // unmounting original - firstWrapper.unmount(); - - const secondWrapper = mountDraggable(); - const secondNode: HTMLElement = secondWrapper.getDOMNode(); - - expect(secondNode).not.toBe(document.activeElement); - - // cleanup - firstWrapper.unmount(); - secondWrapper.unmount(); - }); - - it('should not give focus to a mounting draggable that did not have the last focused id', () => { - const firstWrapper = mountDraggable(); - - const firstNode: HTMLElement = firstWrapper.getDOMNode(); - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); - - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - firstWrapper.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); - - // Mounting new draggable with different id - it should not get focus - const secondProps: OwnProps = { - ...defaultOwnProps, - draggableId: 'my cool new id', - }; - const secondWrapper = mountDraggable({ - ownProps: secondProps, - }); - const secondNode: HTMLElement = secondWrapper.getDOMNode(); - expect(secondNode).not.toBe(document.activeElement); - - // cleanup - looseFocus(firstWrapper); - firstWrapper.unmount(); - secondWrapper.unmount(); - }); - - it('should not give focus draggable if something else on the page has been focused on', () => { - const body: ?HTMLElement = document.body; - if (!body) { - throw new Error('document.body required for test'); - } - const button: HTMLButtonElement = document.createElement('button'); - body.appendChild(button); - - const firstWrapper = mountDraggable(); - const firstNode: HTMLElement = firstWrapper.getDOMNode(); - - // Originally does not have focus - expect(firstNode).not.toBe(document.activeElement); - - // Giving focus to draggable - firstNode.focus(); - // Ensuring that the focus event handler is called - firstWrapper.simulate('focus'); - // Asserting that it is now the focused element - expect(firstNode).toBe(document.activeElement); - - // unmounting original - firstWrapper.unmount(); - - // now focusing on something else - button.focus(); - expect(button).toBe(document.activeElement); - - const secondWrapper = mountDraggable(); - const secondNode: HTMLElement = secondWrapper.getDOMNode(); - - // does not get focus as button has it - expect(secondNode).not.toBe(document.activeElement); - expect(button).toBe(document.activeElement); - - // cleanup - firstWrapper.unmount(); - secondWrapper.unmount(); - button.blur(); - body.removeChild(button); - }); - }); - - // A lighter set of tests - describe('draggable is not the drag handle', () => { - it('should restore focus after an unmount', () => { - const original = mountDraggable({ - WrappedComponent: WithNestedHandle, - }); - const handle: ?HTMLElement = original.getDOMNode().querySelector('.can-drag'); - - if (!handle) { - throw new Error('Could not find drag handle'); - } - - // Should not have focus by default - expect(handle).not.toBe(document.activeElement); - // Lets give it focus - handle.focus(); - original.find('.can-drag').simulate('focus'); - // Okay, it now has focus - expect(handle).toBe(document.activeElement); - - // Goodbye old friend - original.unmount(); - - const next = mountDraggable({ - WrappedComponent: WithNestedHandle, - }); - - const newHandle: ?HTMLElement = next.getDOMNode().querySelector('.can-drag'); - - if (!newHandle) { - throw new Error('Could not find drag handle'); - } - - expect(newHandle).toBe(document.activeElement); - - // cleanup - // remove focus - handle.blur(); - next.find('.can-drag').simulate('blur'); - // unmount all the things - next.unmount(); - original.unmount(); - }); - }); - }); - - describe('Portal usage', () => { + // This is covered in focus-management.spec + // But I have included in here also to ensure that the entire + // consumer experience is tested (this is how a consumer would use it) + describe('Portal usage (Draggable consumer)', () => { const body: ?HTMLElement = document.body; if (!body) { throw new Error('Portal test requires document.body to be present'); From 8a0ebac50fc74bc0619c19fc431c6ce364c47cc8 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 09:55:49 +1000 Subject: [PATCH 15/17] moving test file. removing outdated warning from readme --- README.md | 2 -- test/unit/view/{ => drag-handle}/drag-handle.spec.js | 0 2 files changed, 2 deletions(-) rename test/unit/view/{ => drag-handle}/drag-handle.spec.js (100%) diff --git a/README.md b/README.md index 1465114b08..08ffebf5ac 100644 --- a/README.md +++ b/README.md @@ -76,8 +76,6 @@ You can check out all the features that will be landing soon [on our issue page] There are a lot of libraries out there that allow for drag and drop interactions within React. Most notable of these is the amazing [`react-dnd`](https://github.com/react-dnd/react-dnd). It does an incredible job at providing a great set of drag and drop primitives which work especially well with the [wildly inconsistent](https://www.quirksmode.org/blog/archives/2009/09/the_html5_drag.html) html5 drag and drop feature. **`react-beautiful-dnd` is a higher level abstraction specifically built for vertical and horizontal lists**. Within that subset of functionality `react-beautiful-dnd` offers a powerful, natural and beautiful drag and drop experience. However, it does not provide the breadth of functionality offered by react-dnd. So this library might not be for you depending on what your use case is. -`react-beautiful-dnd` [uses `position: fixed` to position the dragging element](#positioning-ownership). In some layouts, this might break how the element is rendered. One example is a ``-based layout which will lose column widths for dragged ``s. Follow [#103](https://github.com/atlassian/react-beautiful-dnd/issues/103) for updates on support for this use case. - ## Driving philosophy: physicality The core design idea of `react-beautiful-dnd` is physicality: we want users to feel like they are moving physical objects around diff --git a/test/unit/view/drag-handle.spec.js b/test/unit/view/drag-handle/drag-handle.spec.js similarity index 100% rename from test/unit/view/drag-handle.spec.js rename to test/unit/view/drag-handle/drag-handle.spec.js From 66580b3f5142e8154332b05c9403caf7647075b9 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 10:03:19 +1000 Subject: [PATCH 16/17] fixing drag handle tests --- .size-snapshot.json | 22 +++++++------- .../unit/view/drag-handle/drag-handle.spec.js | 30 +++++++++++-------- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/.size-snapshot.json b/.size-snapshot.json index 2567269021..20c8852094 100644 --- a/.size-snapshot.json +++ b/.size-snapshot.json @@ -1,21 +1,21 @@ { "dist/react-beautiful-dnd.js": { - "bundled": 399839, - "minified": 149239, - "gzipped": 42623 + "bundled": 402551, + "minified": 150440, + "gzipped": 42949 }, "dist/react-beautiful-dnd.min.js": { - "bundled": 358155, - "minified": 132319, - "gzipped": 37652 + "bundled": 360867, + "minified": 133520, + "gzipped": 37980 }, "dist/react-beautiful-dnd.esm.js": { - "bundled": 176656, - "minified": 88917, - "gzipped": 22577, + "bundled": 179282, + "minified": 90441, + "gzipped": 22922, "treeshaked": { - "rollup": 77471, - "webpack": 79091 + "rollup": 78671, + "webpack": 80293 } } } diff --git a/test/unit/view/drag-handle/drag-handle.spec.js b/test/unit/view/drag-handle/drag-handle.spec.js index bae8b44d49..78851bd2ad 100644 --- a/test/unit/view/drag-handle/drag-handle.spec.js +++ b/test/unit/view/drag-handle/drag-handle.spec.js @@ -4,10 +4,10 @@ import type { Node } from 'react'; import { mount } from 'enzyme'; // eslint-disable-next-line no-duplicate-imports import type { ReactWrapper } from 'enzyme'; -import DragHandle from '../../../src/view/drag-handle/drag-handle'; -import { sloppyClickThreshold } from '../../../src/view/drag-handle/util/is-sloppy-click-threshold-exceeded'; +import DragHandle from '../../../../src/view/drag-handle/drag-handle'; +import { sloppyClickThreshold } from '../../../../src/view/drag-handle/util/is-sloppy-click-threshold-exceeded'; // eslint-disable-next-line no-duplicate-imports -import type { Callbacks, DragHandleProps } from '../../../src/view/drag-handle/drag-handle-types'; +import type { Callbacks, DragHandleProps } from '../../../../src/view/drag-handle/drag-handle-types'; import { dispatchWindowMouseEvent, dispatchWindowKeyDownEvent, @@ -16,16 +16,17 @@ import { touchEvent, withKeyboard, dispatchWindowEvent, -} from '../../utils/user-input-util'; -import type { Position, DraggableId } from '../../../src/types'; -import * as keyCodes from '../../../src/view/key-codes'; -import getWindowScroll from '../../../src/view/window/get-window-scroll'; -import setWindowScroll from '../../utils/set-window-scroll'; -import getArea from '../../../src/state/get-area'; -import { timeForLongPress, forcePressThreshold } from '../../../src/view/drag-handle/sensor/create-touch-sensor'; -import { interactiveTagNames } from '../../../src/view/drag-handle/util/should-allow-dragging-from-target'; -import type { TagNameMap } from '../../../src/view/drag-handle/util/should-allow-dragging-from-target'; -import { styleContextKey, canLiftContextKey } from '../../../src/view/context-keys'; +} from '../../../utils/user-input-util'; +import type { Position, DraggableId } from '../../../../src/types'; +import * as keyCodes from '../../../../src/view/key-codes'; +import getWindowScroll from '../../../../src/view/window/get-window-scroll'; +import setWindowScroll from '../../../utils/set-window-scroll'; +import getArea from '../../../../src/state/get-area'; +import { timeForLongPress, forcePressThreshold } from '../../../../src/view/drag-handle/sensor/create-touch-sensor'; +import { interactiveTagNames } from '../../../../src/view/drag-handle/util/should-allow-dragging-from-target'; +import type { TagNameMap } from '../../../../src/view/drag-handle/util/should-allow-dragging-from-target'; +import { styleContextKey, canLiftContextKey } from '../../../../src/view/context-keys'; +import * as attributes from '../../../../src/view/data-attributes'; const primaryButton: number = 0; const auxiliaryButton: number = 1; @@ -165,6 +166,9 @@ const childRef: HTMLElement = document.createElement('div'); const singleRef: HTMLElement = document.createElement('div'); [parentRef, childRef, singleRef].forEach((ref: HTMLElement) => { + // faking that they are drag handles + ref.setAttribute(attributes.dragHandle, 'yolo'); + jest.spyOn(ref, 'getBoundingClientRect').mockImplementation(() => getArea({ left: 0, top: 0, From e5df5e66974d56c3c2e317194d69d413285582ff Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 10:42:38 +1000 Subject: [PATCH 17/17] fixing tests. correctly accounting for disabled state in focus managemente --- src/view/drag-handle/drag-handle.jsx | 31 +++++++++++++++++-- src/view/drag-handle/util/focus-retainer.js | 9 +++++- .../drag-handle/util/get-drag-handle-ref.js | 10 +++--- test/unit/view/unconnected-draggable.spec.js | 12 +++++-- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/view/drag-handle/drag-handle.jsx b/src/view/drag-handle/drag-handle.jsx index 5e3ba6ee97..0b47f29022 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -2,6 +2,7 @@ import { Component } from 'react'; import PropTypes from 'prop-types'; import memoizeOne from 'memoize-one'; +import invariant from 'tiny-invariant'; import getWindowFromRef from '../get-window-from-ref'; import getDragHandleRef from './util/get-drag-handle-ref'; import type { @@ -90,7 +91,15 @@ export default class DragHandle extends Component { return; } - focusRetainer.tryRestoreFocus(this.props.draggableId, getDragHandleRef(draggableRef)); + // drag handle ref will not be available when not enabled + if (!this.props.isEnabled) { + return; + } + + const dragHandleRef: ?HTMLElement = getDragHandleRef(draggableRef); + invariant(dragHandleRef, 'DragHandle could not find drag handle element'); + + focusRetainer.tryRestoreFocus(this.props.draggableId, dragHandleRef); } componentDidUpdate(prevProps: Props) { @@ -101,9 +110,21 @@ export default class DragHandle extends Component { // After a ref change we might need to manually force focus onto the ref. // When moving something into or out of a portal the element looses focus // https://github.com/facebook/react/issues/12454 - if (ref && this.isFocused) { - getDragHandleRef(ref).focus(); + + // No need to focus + if (!ref || !this.isFocused) { + return; } + + // No drag handle ref will be available to focus on + if (!this.props.isEnabled) { + return; + } + + const dragHandleRef: ?HTMLElement = getDragHandleRef(ref); + invariant(dragHandleRef, 'DragHandle could not find drag handle element'); + + dragHandleRef.focus(); } const isCapturing: boolean = this.isAnySensorCapturing(); @@ -156,6 +177,10 @@ export default class DragHandle extends Component { }); const shouldRetainFocus: boolean = (() => { + if (!this.props.isEnabled) { + return false; + } + // not already focused if (!this.isFocused) { return false; diff --git a/src/view/drag-handle/util/focus-retainer.js b/src/view/drag-handle/util/focus-retainer.js index 90ef2b69ff..b80c81c29f 100644 --- a/src/view/drag-handle/util/focus-retainer.js +++ b/src/view/drag-handle/util/focus-retainer.js @@ -70,7 +70,14 @@ const tryRestoreFocus = (id: DraggableId, draggableRef: HTMLElement) => { retainingFocusFor = null; // no need to clear it - we are already clearing it clearRetentionOnFocusChange.cancel(); - getDragHandleRef(draggableRef).focus(); + + const dragHandleRef: ?HTMLElement = getDragHandleRef(draggableRef); + + if (!dragHandleRef) { + console.warn('Could not find drag handle in the DOM to focus on it'); + return; + } + dragHandleRef.focus(); }; const retainer: FocusRetainer = { diff --git a/src/view/drag-handle/util/get-drag-handle-ref.js b/src/view/drag-handle/util/get-drag-handle-ref.js index b9bced3fdd..fc2070cb61 100644 --- a/src/view/drag-handle/util/get-drag-handle-ref.js +++ b/src/view/drag-handle/util/get-drag-handle-ref.js @@ -1,22 +1,22 @@ // @flow -import invariant from 'tiny-invariant'; import { dragHandle } from '../../data-attributes'; const selector: string = `[${dragHandle}]`; -const getDragHandleRef = (draggableRef: HTMLElement): HTMLElement => { +// If called when the component is disabled then the data +// attribute will not be present +const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => { if (draggableRef.hasAttribute(dragHandle)) { return draggableRef; } // find the first nested drag handle // querySelector will return the first match on a breadth first search which is what we want + // Search will fail when the drag handle is disabled // https://codepen.io/alexreardon/pen/erOqyZ const el: ?HTMLElement = draggableRef.querySelector(selector); - invariant(el, 'Could not find draggable ref'); - - return el; + return el || null; }; export default getDragHandleRef; diff --git a/test/unit/view/unconnected-draggable.spec.js b/test/unit/view/unconnected-draggable.spec.js index f37aaac40b..a4fab593d7 100644 --- a/test/unit/view/unconnected-draggable.spec.js +++ b/test/unit/view/unconnected-draggable.spec.js @@ -50,7 +50,7 @@ class Item extends Component<{ provided: Provided }> { return (
provided.innerRef(ref)} + ref={provided.innerRef} {...provided.draggableProps} {...provided.dragHandleProps} > @@ -255,7 +255,13 @@ const getStubber = stub => const snapshot: StateSnapshot = this.props.snapshot; stub({ provided, snapshot }); return ( -
+
+ Drag me! +
); } }; @@ -278,7 +284,7 @@ class WithNestedHandle extends Component<{ provided: Provided }> { const provided: Provided = this.props.provided; return (
provided.innerRef(ref)} + ref={provided.innerRef} {...provided.draggableProps} >