Add support for dragging SVGElement's

[mikesobol]

Bug or feature request?

Bug

Expected behavior

Ref check on a svg element does not fail.

Actual behavior

Ref check on a svg element fails.

Steps to reproduce

Add <svg ref={provided.innerRef}> as a child of <Draggable>. The throwIfRefIsInvalid check will fail because svg elements don't inherit from HTMLElement (they inherit from SVGElement).

What version of React are you using?

16.4.1

What version of react-beautiful-dnd are you running?

8.0.0

What browser are you using?

Chrome 66.0.3359.181 x64

Demo

https://codesandbox.io/s/31w7vj1vxp

It can be fixed by wrapping the svg in a div (or other element that inherits from HTMLElement).
Like this: https://codesandbox.io/s/2xzvrr2znr

[alexreardon]

Dragging an SVGElement was technically never officially supported:

type DraggableProvided = {|
+  innerRef: HTMLElement => void,
  draggableProps: DraggableProps,
  // will be null if the draggable is disabled
  dragHandleProps: ?DragHandleProps,
|};

The element provided needs to be a HTMLElement and not a Element or SVGElement

HTMLElement extends Element and SVGElement extends Element. But a SVGElement is not a HTMLElement. An Element cannot have a tabIndex and is therefore not accessible via a keyboard: https://developer.mozilla.org/en-US/docs/Web/API/Element. Additionally, an Element does not have any focus behaviour which we use and manipulate. We currently call .focus() on the element to try to preserve focus after a drag if it had it. This would throw an exception with a SVGElement (I guess it would never have had focus though).

Given our focus on accessibility, I think we need to keep the requirement of a Draggable being a HTMLElement so that a tabIndex can be correctly placed on the element for keyboard dragging.

Thoughts?

[alexreardon]

We did a bit of looking at this one is really confusing. We could not find much information about svg's and focus. It looks like if you add a tab index to an SVG it becomes tabbable. However, if you call .focus on an svg in IE11 it throws an exception. According to my understanding of the specs that is correct. The other browsers all it though...

Hmmm

[alexreardon]

So if you had a user using ie11 with keyboard dragging they would previously have gotten an exception.

I think the best option to keep the HTMLElement requirement and recommend wrapping SVG's in a HTMLElement for dragging.

Still thinking and exploring though!

[mikesobol]

The error message reads provided.innerRef has not been provided with valid DOM ref. It also links to a guide that mainly explains the difference between a ref on a DOM node and a React Component. IMO, both the error message and the guide should clarify, that the DOM element should be a descendant of HTMLElement, if there is no support for SVG's.

[alexreardon]

We can definitely add some clarity to both of those. We are still exploring for now

[alexreardon]

It turns out using .focus for SVG's is a complicated problem: https://allyjs.io/tutorials/focusing-in-svg.html

.focus() on an SVG is not supported in IE11. This means that if you use an SVG today as a drag handle you will get an exception when:

• moving an item into a portal - we can use .focus() to retain focus
• moving an item between lists - we can use .focus() to retain focus

We could look into borrowing something like this:

https://github.com/medialize/ally.js/blob/88e1af44df20df3209afe377aa4e9d21ef426ff2/src/element/focus.js.

In the short term we can add some more helpful messaging and docs while we continue to investigate this one