From f966b6f883e574194815e46dc21defe15ab7289d Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Thu, 19 Apr 2018 10:50:47 +1000 Subject: [PATCH] Maintain focus when moving between lists (#449) --- .size-snapshot.json | 22 +- README.md | 15 +- src/view/data-attributes.js | 5 + src/view/drag-handle/drag-handle-types.js | 4 +- src/view/drag-handle/drag-handle.jsx | 105 ++++- .../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 + src/view/drag-handle/util/focus-retainer.js | 88 +++++ .../drag-handle/util/get-drag-handle-ref.js | 22 ++ src/view/draggable/draggable.jsx | 28 +- src/view/droppable/droppable.jsx | 9 + src/view/style-marshal/get-styles.js | 9 +- src/view/style-marshal/selectors.js | 0 src/view/style-marshal/style-marshal.js | 3 +- 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 | 11 - stories/src/primatives/quote-list.jsx | 10 +- stories/src/reorder.js | 4 - ...simple-vertical-list-with-keyboard.spec.js | 8 +- .../{ => drag-handle}/drag-handle.spec.js | 49 ++- .../view/drag-handle/focus-management.spec.js | 365 ++++++++++++++++++ test/unit/view/style-marshal.spec.js | 3 +- test/unit/view/unconnected-draggable.spec.js | 75 ++-- test/unit/view/unconnected-droppable.spec.js | 7 +- .../src/components/examples/board/board.jsx | 8 - .../src/components/examples/board/column.jsx | 2 - .../multiple-horizontal/quote-app.jsx | 4 +- .../examples/multiple-vertical/quote-app.jsx | 13 +- .../examples/primatives/author-item.jsx | 10 - .../examples/primatives/author-list.jsx | 2 - .../examples/primatives/quote-item.jsx | 11 - .../examples/primatives/quote-list.jsx | 10 +- website/src/components/examples/reorder.js | 4 - 40 files changed, 718 insertions(+), 244 deletions(-) create mode 100644 src/view/data-attributes.js create mode 100644 src/view/drag-handle/util/focus-retainer.js create mode 100644 src/view/drag-handle/util/get-drag-handle-ref.js create mode 100644 src/view/style-marshal/selectors.js rename test/unit/view/{ => drag-handle}/drag-handle.spec.js (98%) create mode 100644 test/unit/view/drag-handle/focus-management.spec.js 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/README.md b/README.md index 274a73ad2e..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 @@ -692,15 +690,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. @@ -1027,6 +1016,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 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 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` 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/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..0b47f29022 100644 --- a/src/view/drag-handle/drag-handle.jsx +++ b/src/view/drag-handle/drag-handle.jsx @@ -2,6 +2,9 @@ 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 { Props, DragHandleProps, @@ -16,6 +19,7 @@ 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 +39,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 +52,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 +80,53 @@ 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; + } + + // 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) { + 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 + + // 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(); if (!isCapturing) { @@ -122,6 +164,45 @@ 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 = (() => { + if (!this.props.isEnabled) { + return false; + } + + // 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 +258,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/drag-handle/util/focus-retainer.js b/src/view/drag-handle/util/focus-retainer.js new file mode 100644 index 0000000000..b80c81c29f --- /dev/null +++ b/src/view/drag-handle/util/focus-retainer.js @@ -0,0 +1,88 @@ +// @flow +import getDragHandleRef from './get-drag-handle-ref'; +import type { DraggableId } from '../../../types'; + +type FocusRetainer = {| + retain: (draggableId: DraggableId) => void, + tryRestoreFocus: (draggableId: DraggableId, draggableRef: HTMLElement) => void, +|} + +// our shared state +let retainingFocusFor: ?DraggableId = null; + +// If we focus on +const clearRetentionOnFocusChange = (() => { + let isBound: boolean = false; + + const bind = () => { + if (isBound) { + return; + } + + 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 }); + }; + + 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 = () => { + // unbinding self after single use + unbind(); + retainingFocusFor = null; + }; + + const result = () => bind(); + result.cancel = () => unbind(); + + return result; +})(); + +const retain = (id: DraggableId) => { + retainingFocusFor = id; + clearRetentionOnFocusChange(); +}; + +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 + + retainingFocusFor = null; + // no need to clear it - we are already clearing it + clearRetentionOnFocusChange.cancel(); + + 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 = { + retain, + tryRestoreFocus, +}; + +export default retainer; diff --git a/src/view/drag-handle/util/get-drag-handle-ref.js b/src/view/drag-handle/util/get-drag-handle-ref.js new file mode 100644 index 0000000000..fc2070cb61 --- /dev/null +++ b/src/view/drag-handle/util/get-drag-handle-ref.js @@ -0,0 +1,22 @@ +// @flow +import { dragHandle } from '../../data-attributes'; + +const selector: string = `[${dragHandle}]`; + +// 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); + + return el || null; +}; + +export default getDragHandleRef; diff --git a/src/view/draggable/draggable.jsx b/src/view/draggable/draggable.jsx index b97990bda0..659efa5c7f 100644 --- a/src/view/draggable/draggable.jsx +++ b/src/view/draggable/draggable.jsx @@ -44,7 +44,6 @@ export default class Draggable extends Component { /* eslint-disable react/sort-comp */ callbacks: DragHandleCallbacks styleContext: string - isFocused: boolean = false ref: ?HTMLElement = null // Need to declare contextTypes without flow @@ -58,8 +57,6 @@ export default class Draggable extends Component { super(props, context); const callbacks: DragHandleCallbacks = { - onFocus: this.onFocus, - onBlur: this.onBlur, onLift: this.onLift, onMove: this.onMove, onDrop: this.onDrop, @@ -75,6 +72,15 @@ 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 + `); + } + } + componentWillUnmount() { // releasing reference to ref for cleanup this.ref = null; @@ -117,14 +123,6 @@ export default class Draggable extends Component { lift(draggableId, initial, getViewport(), autoScrollMode); } - onFocus = () => { - this.isFocused = true; - } - - onBlur = () => { - this.isFocused = false; - } - onMove = (client: Position) => { this.throwIfCannotDrag(); @@ -188,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.isFocused) { - this.ref.focus(); - } }) getDraggableRef = (): ?HTMLElement => this.ref; @@ -379,6 +370,7 @@ export default class Draggable 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 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/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 1efdce758d..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` @@ -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/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/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/drag-handle.spec.js b/test/unit/view/drag-handle/drag-handle.spec.js similarity index 98% rename from test/unit/view/drag-handle.spec.js rename to test/unit/view/drag-handle/drag-handle.spec.js index 97817afff8..78851bd2ad 100644 --- a/test/unit/view/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,23 +16,22 @@ 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; const getStubCallbacks = (): Callbacks => ({ - onFocus: jest.fn(), - onBlur: jest.fn(), onLift: jest.fn(), onMove: jest.fn(), onMoveForward: jest.fn(), @@ -167,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, @@ -182,6 +184,7 @@ const getNestedWrapper = (parentCallbacks: Callbacks, childCallbacks: Callbacks) callbacks={parentCallbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => parentRef} canDragInteractiveElements={false} @@ -193,6 +196,7 @@ const getNestedWrapper = (parentCallbacks: Callbacks, childCallbacks: Callbacks) callbacks={childCallbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => childRef} canDragInteractiveElements={false} @@ -216,6 +220,7 @@ const getWrapper = (callbacks: Callbacks): ReactWrapper => callbacks={callbacks} direction="vertical" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -266,6 +271,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -291,6 +297,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -324,6 +331,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -1497,6 +1505,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction="vertical" getDraggableRef={() => singleRef} @@ -1560,6 +1569,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -1663,6 +1673,7 @@ describe('drag handle', () => { callbacks={customCallbacks} direction="horizontal" isDragging={false} + isDropAnimating={false} isEnabled getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -2682,6 +2693,7 @@ describe('drag handle', () => { callbacks={callbacks} isEnabled={false} isDragging={false} + isDropAnimating={false} direction={null} getDraggableRef={() => singleRef} canDragInteractiveElements={false} @@ -3065,6 +3077,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3100,6 +3113,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3138,6 +3152,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3181,6 +3196,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3227,6 +3243,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3269,6 +3286,7 @@ describe('drag handle', () => { draggableId={draggableId} callbacks={customCallbacks} isDragging={false} + isDropAnimating={false} isEnabled direction={null} getDraggableRef={() => singleRef} @@ -3323,6 +3341,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..7b51b24ba8 --- /dev/null +++ b/test/unit/view/drag-handle/focus-management.spec.js @@ -0,0 +1,365 @@ +// @flow +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 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(), +}); + +const options = { + context: { + [styleContextKey]: 'hello', + [canLiftContextKey]: () => true, + }, + childContextTypes: { + [styleContextKey]: PropTypes.string.isRequired, + [canLiftContextKey]: PropTypes.func.isRequired, + }, +}; + +const body: ?HTMLElement = document.body; + +if (!body) { + throw new Error('document.body not found'); +} + +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) => ( + + )} + + ); + } + } + + it('should retain focus if draggable ref is changing and had focus', () => { + const wrapper = mount(, options); + + const original: HTMLElement = wrapper.getDOMNode(); + expect(original).not.toBe(document.activeElement); + + // giving it focus + original.focus(); + wrapper.simulate('focus'); + expect(original).toBe(document.activeElement); + + // forcing change of parent ref by moving into a portal + wrapper.setProps({ + usePortal: true, + }); + + 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('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 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/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 147761cf47..a4fab593d7 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() { @@ -49,7 +50,7 @@ class Item extends Component<{ provided: Provided }> { return (
provided.innerRef(ref)} + ref={provided.innerRef} {...provided.draggableProps} {...provided.dragHandleProps} > @@ -104,18 +105,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 = { @@ -255,7 +255,13 @@ const getStubber = stub => const snapshot: StateSnapshot = this.props.snapshot; stub({ provided, snapshot }); return ( -
+
+ Drag me! +
); } }; @@ -266,6 +272,32 @@ 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'); +}; +class WithNestedHandle extends Component<{ provided: Provided }> { + render() { + const provided: Provided = this.props.provided; + return ( +
+
+ Cannot drag by me +
+
+ Can drag by me +
+
+ ); + } +} + describe('Draggable - unconnected', () => { beforeAll(() => { requestAnimationFrame.reset(); @@ -303,7 +335,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', () => { @@ -341,25 +373,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({ @@ -1367,7 +1380,10 @@ describe('Draggable - unconnected', () => { }); }); - 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'); @@ -1456,6 +1472,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', () => { 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 +
); } }; 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, }; };