Skip to content
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

Merged
merged 17 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

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


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.
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explains the rationale for the change

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

@alexreardon alexreardon Apr 18, 2018

Choose a reason for hiding this comment

The 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We call out other pieces of information like this through the docs

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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).
Expand Down
5 changes: 5 additions & 0 deletions src/view/data-attributes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// @flow
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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`;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i generally type every variable even if it can be inferred

47 changes: 38 additions & 9 deletions src/view/draggable/draggable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import memoizeOne from 'memoize-one';
import invariant from 'tiny-invariant';
import type {
Position,
DraggableId,
DraggableDimension,
InitialDragPositions,
DroppableId,
Expand All @@ -14,6 +15,8 @@ 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 {
Expand Down Expand Up @@ -44,7 +47,7 @@ export default class Draggable extends Component<Props> {
/* eslint-disable react/sort-comp */
callbacks: DragHandleCallbacks
styleContext: string
isFocused: boolean = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now handled by the drag handle

isDragHandleFocused: boolean = false
ref: ?HTMLElement = null

// Need to declare contextTypes without flow
Expand All @@ -58,8 +61,8 @@ export default class Draggable extends Component<Props> {
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,
Expand All @@ -75,9 +78,35 @@ export default class Draggable extends Component<Props> {
this.styleContext = context[styleContextKey];
}

componentDidMount() {
if (!this.ref) {
console.error(`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yummy new error message

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is innerRef a required prop? Under what circumstances is ref undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a callback which they might not have called

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;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplification (thanks @petegleeson)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now only doing this focus management stuff if they where dragging or dropping when the unmount occurs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also simplified the focus retainer

// 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.
Expand Down Expand Up @@ -117,12 +146,12 @@ export default class Draggable extends Component<Props> {
lift(draggableId, initial, getViewport(), autoScrollMode);
}

onFocus = () => {
this.isFocused = true;
onDragHandleFocus = () => {
this.isDragHandleFocused = true;
}

onBlur = () => {
this.isFocused = false;
onDragHandleBlur = () => {
this.isDragHandleFocused = false;
}

onMove = (client: Position) => {
Expand Down Expand Up @@ -192,8 +221,8 @@ export default class Draggable extends Component<Props> {
// After a ref change we might need to manually force focus onto the ref.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

focus retention logic is now controlled by the component that has focus: the drag handle

// 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);
}
})

Expand Down
28 changes: 28 additions & 0 deletions src/view/draggable/focus-on-drag-handle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// @flow
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new file to help us to get the draggable ref which is currently not provided by consumers

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);

return el || null;
};

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;
83 changes: 83 additions & 0 deletions src/view/draggable/focus-retainer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// @flow
import focusOnDragHandle from './focus-on-drag-handle';
import * as attributes from '../data-attributes';
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: add comment

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 = () => {
console.log('ON WINDOW FOCUS CHANGE');
// 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();
focusOnDragHandle(draggableRef);
};

const retainer: FocusRetainer = {
retain,
tryRestoreFocus,
};

export default retainer;
9 changes: 9 additions & 0 deletions src/view/droppable/droppable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ export default class Droppable extends Component<Props> {
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
Expand Down
9 changes: 4 additions & 5 deletions src/view/style-marshal/get-styles.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow
import { css } from '../animation';
import * as attributes from '../data-attributes';

export type Styles = {|
dragging: string,
Expand All @@ -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

Expand Down
Empty file.
3 changes: 1 addition & 2 deletions src/view/style-marshal/style-marshal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand Down
8 changes: 0 additions & 8 deletions stories/src/board/board.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ type Props = {|
type State = {|
columns: QuoteMap,
ordered: string[],
autoFocusQuoteId: ?string,
|}

export default class Board extends Component<Props, State> {
Expand All @@ -48,7 +47,6 @@ export default class Board extends Component<Props, State> {
state: State = {
columns: this.props.initial,
ordered: Object.keys(this.props.initial),
autoFocusQuoteId: null,
}

boardRef: ?HTMLElement
Expand All @@ -64,10 +62,6 @@ export default class Board extends Component<Props, State> {

onDragStart = (initial: DragStart) => {
publishOnDragStart(initial);

this.setState({
autoFocusQuoteId: null,
});
}

onDragEnd = (result: DropResult) => {
Expand Down Expand Up @@ -110,7 +104,6 @@ export default class Board extends Component<Props, State> {

this.setState({
columns: data.quoteMap,
autoFocusQuoteId: data.autoFocusQuoteId,
});
}

Expand All @@ -134,7 +127,6 @@ export default class Board extends Component<Props, State> {
index={index}
title={key}
quotes={columns[key]}
autoFocusQuoteId={this.state.autoFocusQuoteId}
/>
))}
</Container>
Expand Down
2 changes: 0 additions & 2 deletions stories/src/board/column.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ type Props = {|
title: string,
quotes: Quote[],
index: number,
autoFocusQuoteId: ?string,
|}

export default class Column extends Component<Props> {
Expand All @@ -59,7 +58,6 @@ export default class Column extends Component<Props> {
listId={title}
listType="QUOTE"
quotes={quotes}
autoFocusQuoteId={this.props.autoFocusQuoteId}
/>
</Container>
)}
Expand Down
4 changes: 1 addition & 3 deletions stories/src/multiple-horizontal/quote-app.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export default class QuoteApp extends Component<Props, State> {

state: State = {
quoteMap: this.props.initial,
autoFocusQuoteId: null,
};

onDragStart = (initial: DragStart) => {
Expand All @@ -58,7 +57,7 @@ export default class QuoteApp extends Component<Props, State> {
}

render() {
const { quoteMap, autoFocusQuoteId } = this.state;
const { quoteMap } = this.state;

return (
<DragDropContext
Expand All @@ -73,7 +72,6 @@ export default class QuoteApp extends Component<Props, State> {
listId={key}
listType="CARD"
quotes={quoteMap[key]}
autoFocusQuoteId={autoFocusQuoteId}
/>
))}
</Root>
Expand Down
Loading