Maintain focus when moving between lists #449

alexreardon · 2018-04-18T01:36:53Z

Previously there was a lot of ad hoc code to maintain focus of a draggable when it moved from one list to another. It is so important for the user experience that I have baked it into the library

alexreardon · 2018-04-18T01:37:14Z

This will result in a minor version change

alexreardon · 2018-04-18T01:37:33Z

README.md

@@ -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


We now do this out of the box

alexreardon · 2018-04-18T01:38:33Z

src/view/draggable/focus-on-drag-handle.js

@@ -0,0 +1,28 @@
+// @flow


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

alexreardon · 2018-04-18T01:38:51Z

test/unit/view/unconnected-draggable.spec.js

@@ -1367,6 +1374,161 @@ describe('Draggable - unconnected', () => {
    });
  });

+  describe('Cross list movement focus retention', () => {


tests for everyone!

these exercise the feature fairly hard

alexreardon · 2018-04-18T01:39:05Z

website/src/components/examples/board/board.jsx

@@ -40,7 +40,6 @@ type Props = {|
 type State = {|
  columns: QuoteMap,
  ordered: string[],
-  autoFocusQuoteId: ?string,


I got rid of all the lame custom auto focus code

alexreardon · 2018-04-18T01:42:11Z

README.md

@@ -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


This explains the rationale for the change

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.

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

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

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.

alexreardon · 2018-04-18T06:38:39Z

src/view/data-attributes.js

@@ -0,0 +1,5 @@
+// @flow


rather than repeating these everywhere i pulled them out

alexreardon · 2018-04-18T06:38:50Z

src/view/draggable/draggable.jsx

@@ -75,6 +78,18 @@ export default class Draggable extends Component<Props> {
    this.styleContext = context[styleContextKey];
  }

+  componentDidMount() {
+    if (!this.ref) {
+      console.error(`


yummy new error message

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

it is a callback which they might not have called

alexreardon · 2018-04-18T06:39:53Z

src/view/draggable/focus-retainer.js

+// our shared state
+let retainingFocusFor: ?DraggableId = null;
+
+// If we focus on


TODO: add comment

alexreardon · 2018-04-18T06:40:41Z

test/unit/view/unconnected-draggable.spec.js

@@ -1367,6 +1374,161 @@ describe('Draggable - unconnected', () => {
    });
  });

+  describe('Cross list movement focus retention', () => {


these exercise the feature fairly hard

petegleeson

Let's move the information about retaining focus from the readme into a code comment.

petegleeson · 2018-04-18T07:07:11Z

src/view/draggable/draggable.jsx

@@ -75,6 +78,18 @@ export default class Draggable extends Component<Props> {
    this.styleContext = context[styleContextKey];
  }

+  componentDidMount() {
+    if (!this.ref) {
+      console.error(`


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

petegleeson · 2018-04-18T07:17:41Z

README.md

@@ -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


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.

alexreardon · 2018-04-18T07:21:52Z

src/view/draggable/draggable.jsx

  componentWillUnmount() {
    // releasing reference to ref for cleanup
    this.ref = null;
+


Simplification (thanks @petegleeson)

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

This also simplified the focus retainer

alexreardon · 2018-04-18T07:27:49Z

I have pushed up a simplification so that we only do the focus retention on a drag end which is way less work on the browser. I'll update the tests tomorrow

alexreardon · 2018-04-18T23:52:49Z

src/view/draggable/draggable.jsx

@@ -188,13 +186,6 @@ export default class Draggable extends Component<Props> {
    // 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.


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

alexreardon · 2018-04-18T23:54:23Z

src/view/draggable/draggable.jsx

@@ -44,7 +44,6 @@ export default class Draggable extends Component<Props> {
  /* eslint-disable react/sort-comp */
  callbacks: DragHandleCallbacks
  styleContext: string
-  isFocused: boolean = false


now handled by the drag handle

…mente

Blasz

LGTM, just found some typos.

Blasz · 2018-04-18T23:41:13Z

README.md

+##### 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.
+


s/loose/lose

Blasz · 2018-04-18T23:42:47Z

src/view/data-attributes.js

+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`;


Flow question, do these need to be typed as string or can the type be inferred from the assignment since they are constants?

i generally type every variable even if it can be inferred

Blasz · 2018-04-19T00:13:03Z

src/view/drag-handle/drag-handle-types.js

@@ -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,


Nit: Would it be better to name this isDropping to be consistent with the isDragging prop?

Perhaps! We currently use this naming in multiple places so i i'll leave it for now

Blasz · 2018-04-19T00:32:07Z

src/view/drag-handle/util/focus-retainer.js

+    return;
+  }
+
+  // We are about to force force onto a drag handle


s/force force/force focus/?

Blasz · 2018-04-19T00:36:10Z

src/view/drag-handle/drag-handle.jsx

+      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


s/looses/loses

Blasz · 2018-04-19T01:00:08Z

src/view/drag-handle/drag-handle.jsx

+    }
+
+    // drag handle ref will not be available when not enabled
+    if (!this.props.isEnabled) {


Unrelated to this PR but any reason why this prop isn't called isDisabled instead? It wasn't immediately obvious to me what this prop meant exactly. Changing this to isDisabled also aligns with the draggable's disabled prop.

I generally favour positive props for internal usage. As you mentioned, not part of this PR - but perhaps work thinking more about

Blasz · 2018-04-19T01:07:29Z

test/unit/view/unconnected-draggable.spec.js

@@ -266,6 +272,32 @@ const customViewport: Viewport = {
  subject: getArea({ top: 200, left: 100, right: 300, bottom: 300 }),
 };

+const looseFocus = (wrapper: ReactWrapper) => {


s/loose/lose

alexreardon added 5 commits April 18, 2018 09:30

initial implementation

61778c2

adding warning for droppable

7254b05

removing custom auto focus code

b702f48

now focusing on drag handle rather than draggable

63f0c41

fixing linting and flow

e02fc79

alexreardon requested a review from petegleeson April 18, 2018 01:36

alexreardon added the improvement ⭐️ label Apr 18, 2018