-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Maintain focus when moving between lists #449
Changes from 14 commits
61778c2
7254b05
b702f48
63f0c41
e02fc79
da31bf6
8166325
842dfa9
d81b7aa
011bf84
b72ef06
aec9d2f
c8e21ef
f44925f
8a0ebac
66580b3
e5df5e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -1027,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 when moving between lists | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This explains the rationale for the change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not convinced that this information is useful for the user. I would prefer to see this as a code comment in the context of the logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we are controlling the focus I would like to call it out. I have found that calling behaviour out leads to the least confusion (and issues raised). For example: how we use dom events Maintaining focus across list movements is a feature of the library that it is not native to react or the dom There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We call out other pieces of information like this through the docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me it seems like this change makes the focus behave more intuitively. What do you think about moving this section into the "how we use dom events" page? If you think this is important enough to have in the README I am happy to leave it as is. |
||
|
||
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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/loose/lose |
||
##### 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). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
// @flow | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than repeating these everywhere i pulled them out |
||
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`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flow question, do these need to be typed as string or can the type be inferred from the assignment since they are constants? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i generally type every variable even if it can be inferred |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Would it be better to name this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps! We currently use this naming in multiple places so i i'll leave it for now |
||
// the direction of the current droppable | ||
direction: ?Direction, | ||
// get the ref of the draggable | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
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, | ||
|
@@ -16,6 +18,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 +38,8 @@ export default class DragHandle extends Component<Props> { | |
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<Props> { | |
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,33 @@ export default class DragHandle extends Component<Props> { | |
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, getDragHandleRef(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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/looses/loses |
||
// https://github.com/facebook/react/issues/12454 | ||
if (ref && this.isFocused) { | ||
getDragHandleRef(ref).focus(); | ||
} | ||
} | ||
|
||
const isCapturing: boolean = this.isAnySensorCapturing(); | ||
|
||
if (!isCapturing) { | ||
|
@@ -122,6 +143,41 @@ export default class DragHandle extends Component<Props> { | |
} | ||
} | ||
|
||
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 +233,8 @@ export default class DragHandle extends Component<Props> { | |
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
// @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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/force force/force focus/? |
||
|
||
retainingFocusFor = null; | ||
// no need to clear it - we are already clearing it | ||
clearRetentionOnFocusChange.cancel(); | ||
getDragHandleRef(draggableRef).focus(); | ||
}; | ||
|
||
const retainer: FocusRetainer = { | ||
retain, | ||
tryRestoreFocus, | ||
}; | ||
|
||
export default retainer; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// @flow | ||
import invariant from 'tiny-invariant'; | ||
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 | ||
// 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); | ||
|
||
invariant(el, 'Could not find draggable ref'); | ||
|
||
return el; | ||
}; | ||
|
||
export default getDragHandleRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now do this out of the box