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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
now focusing on drag handle rather than draggable
alexreardon committed Apr 18, 2018
commit 63f0c41ab8f3f331449a15431d8546e87bba8d38
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1018,6 +1018,10 @@ It is a contract of this library that it owns the positioning logic of the dragg

To get around this you can use [`React.Portal`](https://reactjs.org/docs/portals.html). We do not enable this functionality by default as it has performance problems. We have a [using a portal guide](/guides/using-a-portal.md) explaining the performance problem in more detail and how you can set up your own `React.Portal` if you want to.

##### Focus retention on movement

When moving a `Draggable` from one list to another the default browser behaviour is for the *drag handle* element to loose focus. This is not good when dragging with a keyboard as the user is then unable to continue to interact with the element. To improve this user experience we give a *drag handle* focus as it mounts if it had browser focus when it unmounted and nothing else has obtained browser focus.

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

15 changes: 8 additions & 7 deletions src/view/draggable/draggable.jsx
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ import type {
import DraggableDimensionPublisher from '../draggable-dimension-publisher/';
import Moveable from '../moveable/';
import DragHandle from '../drag-handle';
import focusOnDragHandle from './focus-on-drag-handle';
import getViewport from '../window/get-viewport';
// eslint-disable-next-line no-duplicate-imports
import type {
@@ -53,7 +54,7 @@ export default class Draggable extends Component<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
@@ -98,8 +99,8 @@ export default class Draggable extends Component<Props> {
return;
}

// This draggable was previously focused - give it focus!
this.ref.focus();
// This drag handle was previously focused - give it focus!
focusOnDragHandle(this.ref);
}

componentWillUnmount() {
@@ -145,13 +146,13 @@ export default class Draggable extends Component<Props> {
}

onFocus = () => {
this.isFocused = true;
this.isDragHandleFocused = true;
// Record that this was the last focused draggable
lastFocused = this.props.draggableId;
}

onBlur = () => {
this.isFocused = false;
this.isDragHandleFocused = false;
// On blur we can clear our last focused
lastFocused = null;
}
@@ -223,8 +224,8 @@ export default class Draggable extends Component<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);
}
})

30 changes: 30 additions & 0 deletions src/view/draggable/focus-on-drag-handle.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// @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
const el: ?HTMLElement = draggableRef.querySelector(selector);

if (!el) {
return null;
}

return el;
};

const focusOnDragHandle = (draggableRef: HTMLElement) => {
const dragHandleRef: ?HTMLElement = getDragHandleRef(draggableRef);
if (!dragHandleRef) {
console.error('Draggable cannot focus on the drag handle as it cannot be found');
return;
}
dragHandleRef.focus();
};

export default focusOnDragHandle;
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,
@@ -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

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
@@ -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;
8 changes: 5 additions & 3 deletions test/browser/simple-vertical-list-with-keyboard.spec.js
Original file line number Diff line number Diff line change
@@ -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;
3 changes: 2 additions & 1 deletion test/unit/view/style-marshal.spec.js
Original file line number Diff line number Diff line change
@@ -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);
3 changes: 2 additions & 1 deletion test/unit/view/unconnected-draggable.spec.js
Original file line number Diff line number Diff line change
@@ -41,6 +41,7 @@ import { combine, withStore, withDroppableId, withStyleContext, withDimensionMar
import { dispatchWindowMouseEvent, mouseEvent } from '../../utils/user-input-util';
import getViewport from '../../../src/view/window/get-viewport';
import { setViewport, resetViewport } from '../../utils/viewport';
import * as attributes from '../../../src/view/data-attributes';

class Item extends Component<{ provided: Provided }> {
render() {
@@ -310,7 +311,7 @@ describe('Draggable - unconnected', () => {
});
const provided: Provided = getLastCall(myMock)[0].provided;

expect(provided.draggableProps['data-react-beautiful-dnd-draggable']).toEqual(styleMarshal.styleContext);
expect(provided.draggableProps[attributes.draggable]).toEqual(styleMarshal.styleContext);
});

describe('drag handle', () => {