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
fixing linting and flow
alexreardon committed Apr 18, 2018
commit e02fc79212f0f2116fb08ede061a6033af59be6b
4 changes: 2 additions & 2 deletions src/view/drag-handle/drag-handle-types.js
Original file line number Diff line number Diff line change
@@ -25,8 +25,8 @@ export type DragHandleProps = {|
// If a consumer is using a portal then the item will loose focus
// when moving to the portal. This breaks keyboard dragging.
// To get around this we manually apply focus if needed when mounting
onFocus: (event: FocusEvent) => void,
onBlur: (event: FocusEvent) => void,
onFocus: () => void,
onBlur: () => void,

// Used to initiate dragging
onMouseDown: (event: MouseEvent) => void,
8 changes: 4 additions & 4 deletions src/view/draggable/draggable.jsx
Original file line number Diff line number Diff line change
@@ -68,8 +68,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,
@@ -145,13 +145,13 @@ export default class Draggable extends Component<Props> {
lift(draggableId, initial, getViewport(), autoScrollMode);
}

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

onBlur = () => {
onDragHandleBlur = () => {
this.isDragHandleFocused = false;
// On blur we can clear our last focused
lastFocused = null;
8 changes: 3 additions & 5 deletions src/view/draggable/focus-on-drag-handle.js
Original file line number Diff line number Diff line change
@@ -9,13 +9,11 @@ const getDragHandleRef = (draggableRef: HTMLElement): ?HTMLElement => {
}

// find the first nested drag handle
// querySelector will return the first match on a breadth first search which is what we want
// https://codepen.io/alexreardon/pen/erOqyZ
const el: ?HTMLElement = draggableRef.querySelector(selector);

if (!el) {
return null;
}

return el;
return el || null;
};

const focusOnDragHandle = (draggableRef: HTMLElement) => {
237 changes: 146 additions & 91 deletions test/unit/view/unconnected-draggable.spec.js
Original file line number Diff line number Diff line change
@@ -272,6 +272,24 @@ const looseFocus = (wrapper: ReactWrapper) => {
el.blur();
// let the wrapper know about it
wrapper.simulate('blur');
};
class WithNestedHandle extends Component<{ provided: Provided }> {
render() {
const provided: Provided = this.props.provided;
return (
<div
ref={ref => provided.innerRef(ref)}
{...provided.draggableProps}
>
<div className="cannot-drag">
Cannot drag by me
</div>
<div className="can-drag" {...provided.dragHandleProps}>
Can drag by me
</div>
</div>
);
}
}

describe('Draggable - unconnected', () => {
@@ -349,25 +367,6 @@ describe('Draggable - unconnected', () => {
});

describe('non standard drag handle', () => {
class WithNestedHandle extends Component<{ provided: Provided }> {
render() {
const provided: Provided = this.props.provided;
return (
<div
ref={ref => provided.innerRef(ref)}
{...provided.draggableProps}
>
<div className="cannot-drag">
Cannot drag by me
</div>
<div className="can-drag" {...provided.dragHandleProps}>
Can drag by me
</div>
</div>
);
}
}

it('should allow the ability to have the drag handle to be a child of the draggable', () => {
const dispatchProps: DispatchProps = getDispatchPropsStub();
managedWrapper = mountDraggable({
@@ -1376,101 +1375,157 @@ describe('Draggable - unconnected', () => {
});

describe('Cross list movement focus retention', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests for everyone!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these exercise the feature fairly hard

it('should maintain focus when mounted into a different list', () => {
const first = mountDraggable();
const original: HTMLElement = first.getDOMNode();
describe('draggable is the drag handle', () => {
it('should maintain focus when mounted into a different list', () => {
const first = mountDraggable();
const original: HTMLElement = first.getDOMNode();

// Originally does not have focus
expect(original).not.toBe(document.activeElement);
// Originally does not have focus
expect(original).not.toBe(document.activeElement);

// Giving focus to draggable
original.focus();
// Ensuring that the focus event handler is called
first.simulate('focus');
// Asserting that it is now the focused element
expect(original).toBe(document.activeElement);
// Giving focus to draggable
original.focus();
// Ensuring that the focus event handler is called
first.simulate('focus');
// Asserting that it is now the focused element
expect(original).toBe(document.activeElement);

// unmounting original
first.unmount();
// unmounting original
first.unmount();

const second = mountDraggable();
const last: HTMLElement = second.getDOMNode();
const second = mountDraggable();
const last: HTMLElement = second.getDOMNode();

expect(last).toBe(document.activeElement);
expect(last).toBe(document.activeElement);

// cleanup
looseFocus(second);
});
// cleanup
looseFocus(second);
first.unmount();
second.unmount();
});

it('should not maintain focus if it did not have it when moving into a new list', () => {
const wrapper = mountDraggable();
const node: HTMLElement = wrapper.getDOMNode();
it('should not maintain focus if it did not have it when moving into a new list', () => {
const wrapper = mountDraggable();
const node: HTMLElement = wrapper.getDOMNode();

// Originally does not have focus
expect(node).not.toBe(document.activeElement);
// Originally does not have focus
expect(node).not.toBe(document.activeElement);

const second = mountDraggable();
const fresh: HTMLElement = second.getDOMNode();
const second = mountDraggable();
const fresh: HTMLElement = second.getDOMNode();

// Still does not have focus
expect(fresh).not.toBe(document.activeElement);
});
// Still does not have focus
expect(fresh).not.toBe(document.activeElement);

it('should not obtain focus if focus is lost before remount', () => {
const firstWrapper = mountDraggable();
const firstNode: HTMLElement = firstWrapper.getDOMNode();
wrapper.unmount();
});

// Originally does not have focus
expect(firstNode).not.toBe(document.activeElement);
it('should not obtain focus if focus is lost before remount', () => {
const firstWrapper = mountDraggable();
const firstNode: HTMLElement = firstWrapper.getDOMNode();

// Giving focus to draggable
firstNode.focus();
// Ensuring that the focus event handler is called
firstWrapper.simulate('focus');
// Asserting that it is now the focused element
expect(firstNode).toBe(document.activeElement);
// Originally does not have focus
expect(firstNode).not.toBe(document.activeElement);

// Now loosing focus
firstNode.blur();
firstWrapper.simulate('blur');
expect(firstNode).not.toBe(document.activeElement);
// Giving focus to draggable
firstNode.focus();
// Ensuring that the focus event handler is called
firstWrapper.simulate('focus');
// Asserting that it is now the focused element
expect(firstNode).toBe(document.activeElement);

// unmounting original
firstWrapper.unmount();
// Now loosing focus
firstNode.blur();
firstWrapper.simulate('blur');
expect(firstNode).not.toBe(document.activeElement);

const secondWrapper = mountDraggable();
const secondNode: HTMLElement = secondWrapper.getDOMNode();
// unmounting original
firstWrapper.unmount();

expect(secondNode).not.toBe(document.activeElement);
});
const secondWrapper = mountDraggable();
const secondNode: HTMLElement = secondWrapper.getDOMNode();

it('should not give focus to a mounting draggable that did not have the last focused id', () => {
const firstWrapper = mountDraggable();
const firstNode: HTMLElement = firstWrapper.getDOMNode();
expect(secondNode).not.toBe(document.activeElement);

// Originally does not have focus
expect(firstNode).not.toBe(document.activeElement);
// cleanup
firstWrapper.unmount();
secondWrapper.unmount();
});

// Giving focus to draggable
firstNode.focus();
// Ensuring that the focus event handler is called
firstWrapper.simulate('focus');
// Asserting that it is now the focused element
expect(firstNode).toBe(document.activeElement);
it('should not give focus to a mounting draggable that did not have the last focused id', () => {
const firstWrapper = mountDraggable();
const firstNode: HTMLElement = firstWrapper.getDOMNode();

// Mounting new draggable with different id - it should not get focus
const secondProps: OwnProps = {
...defaultOwnProps,
draggableId: 'my cool new id',
};
const secondWrapper = mountDraggable({
ownProps: secondProps,
// Originally does not have focus
expect(firstNode).not.toBe(document.activeElement);

// Giving focus to draggable
firstNode.focus();
// Ensuring that the focus event handler is called
firstWrapper.simulate('focus');
// Asserting that it is now the focused element
expect(firstNode).toBe(document.activeElement);

// Mounting new draggable with different id - it should not get focus
const secondProps: OwnProps = {
...defaultOwnProps,
draggableId: 'my cool new id',
};
const secondWrapper = mountDraggable({
ownProps: secondProps,
});
const secondNode: HTMLElement = secondWrapper.getDOMNode();
expect(secondNode).not.toBe(document.activeElement);

// cleanup
looseFocus(firstWrapper);
firstWrapper.unmount();
secondWrapper.unmount();
});
const secondNode: HTMLElement = secondWrapper.getDOMNode();
expect(secondNode).not.toBe(document.activeElement);
});

// cleanup
looseFocus(firstWrapper);
describe('draggable is not the drag handle', () => {
it('should restore focus after an unmount', () => {
const original = mountDraggable({
WrappedComponent: WithNestedHandle,
});
const handle: ?HTMLElement = original.getDOMNode().querySelector('.can-drag');

if (!handle) {
throw new Error('Could not find drag handle');
}

// Should not have focus by default
expect(handle).not.toBe(document.activeElement);
// Lets give it focus
handle.focus();
original.find('.can-drag').simulate('focus');
// Okay, it now has focus
expect(handle).toBe(document.activeElement);

// Goodbye old friend
original.unmount();

const next = mountDraggable({
WrappedComponent: WithNestedHandle,
});

const newHandle: ?HTMLElement = next.getDOMNode().querySelector('.can-drag');

if (!newHandle) {
throw new Error('Could not find drag handle');
}

expect(newHandle).toBe(document.activeElement);

// cleanup
// remove focus
handle.blur();
next.find('.can-drag').simulate('blur');
// unmount all the things
next.unmount();
original.unmount();
});
});
});

7 changes: 6 additions & 1 deletion test/unit/view/unconnected-droppable.spec.js
Original file line number Diff line number Diff line change
@@ -27,7 +27,12 @@ const getStubber = (mock: Function) =>
snapshot: this.props.snapshot,
});
return (
<div>Hey there</div>
<div
ref={this.props.provided.innerRef}
{...this.props.provided.droppableProps}
>
Hey there
</div>
);
}
};