thisbeyond / solid-dnd

A lightweight, performant, extensible drag and drop toolkit for Solid JS.
https://solid-dnd.com
MIT License
516 stars 34 forks source link

Keyboard interactions #76

Open royeden opened 1 year ago

royeden commented 1 year ago

Hi! Thanks for the cool library :smile:

I was setting it up for a personal project and I noticed there weren't any keyboard interactions available, so I tried my best to set them up:

Changes:

// Before
const resolvedTransform = transform(); // Signal gets transform value
if (!transformsAreEqual(resolvedTransform, noopTransform())) { // It's compared
  const style = transformStyle(transform());  // Signal gets transform value again (this is unnecessary, as it should be the same transform)
  element.style.setProperty("transform", style.transform); // Set transform value
}

// After
const resolvedTransform = transform(); // Signal gets transform value and it's reused everywhere else
if (!transformsAreEqual(resolvedTransform, noopTransform())) {
  const style = transformStyle(resolvedTransform);
  element.style.setProperty("transform", style.transform as string);
}

I also accidentally ran into #28 when applying transitions to the elements and found a possible hack-ish solution around it for some cases (didn't get it to work with overlays, sadly, because they unmount before they can execute their transitions). Let me know if you would like to chat about it in another PR.

Caveats:

royeden commented 1 year ago

Also leaving these questions / observations that I had on the code and couldn't quite understand while developing this feature: *I don't know where they should be addressed (is it right to do it here, should it be on an issue?)

For example, the hero that appears on the site with the dnd component is using

const onDragEnd = ({ draggable, droppable }) => {
    if (droppable) {
      const child = droppable.node.children[0];
      droppable.node.insertBefore(draggable.node, child);
      setInDropZone(true);
    } else if (draggable.node.parentElement !== draggablesContainer) {
      draggablesContainer.append(draggable.node);
      setInDropZone(false);
    }
  };

This is the interaction that sets the element inside of the dropzone. The issue with this approach is that before doing that it removes the transform on the draggable, so the element loses context of the previous action. This is another case where #28 comes into the conversation: losing context of the drag action state before computing the result of it can lead to some weird interactions. This is what I'm referring to as the hack-ish solution to transitions: I found the way around the order of the drop and the reset of the transforms and used the new order to recalculate the layout before resetting the transform on the draggable.

martinpengellyphillips commented 1 year ago

Thanks for this - I'll take a look at it soon.

martinpengellyphillips commented 1 year ago

I'll pick this up next now #80 is done. But in the meantime, I realised I hadn't answered your questions so here you go:

Why does a droppable support transforms? Is there anything that's setting them? How would they work, would they offset the container? Is this intended for nested support?

An example would be sortables. The droppables are transformed as part of the illusion of moving items. This needs to be distinct from draggable as the 'space' droppable for the dragged item needs to be positioned correctly as it moves.

Why does the library reset transforms before firing the events that affect the placement of the elements on the DOM? How much control should the library have of the final placements of the elements?

At the point onDragEnd etc fire, the drag is considered finished (the mouse has been released for example). I tried many iterations of deferring this to make things like animating to final position easier, but they ended up introducing more complexity and edge cases throughout the library. So I'll take a different approach now for #28

As an aside, the 'hero' example is outdated and a poor one as it manipulates the dom directly. I updated all the other examples to use Solid's reactive system more, but must have missed that one. It should be more like:


export const DragAndDropExample = () => {
  const [where, setWhere] = createSignal("outside");

  const onDragEnd: DragEventHandler = ({ droppable }) => {
    if (droppable) {
      setWhere("inside");
    } else {
      setWhere("outside");
    }
  };

  return (
    <DragDropProvider onDragEnd={onDragEnd}>
      <DragDropSensors />
      <div class="min-h-15">
        <Show when={where() === "outside"}>
          <Draggable />
        </Show>
      </div>
      <Droppable>
        <Show when={where() === "inside"}>
          <Draggable />
        </Show>
      </Droppable>
    </DragDropProvider>
  );
};
royeden commented 1 year ago

Thank you so much for the answers, they are absolutely clear and make a ton of sense!

I see that after the merge there's conflicts to solve, so I'll be doing that when I'm available either this weekend or this following week.

I've also checked a bit how dnd-kit does their aria attributes support and they use an announcement component that wraps their dnd container with a div that acts as an aria live region, where they display information on the current action inside of a node. It got me thinking on how to handle the same approach on this library, but I believe that for that to happen there's a need of the context of the conclusion of the action, therefore that would have to exist in userland, perhaps it can be achieved as another example on the site, so I'll check that in the future. If it can be generalized, it could be added to the package or as a separate dependency (although I believe accessibility is important and this is one of the few solutions for DND on solidjs, so I would go for adding the few extra bytes on the main package if it means that can be added). In the meantime, I believe that we can simply add aria attributes to the dragged node indicating where it's positioned relative to it's original position and indicating if it's positioned on top of a droppable during sensor actions, perhaps that can be addressed as another issue and resolved in another PR.

There should also be a PR if this is merged to change the dragabbles from divs to buttons as they are focusable elements that will allow for keyboard interactions 😁

royeden commented 1 year ago

Fixed the conflicts!

paxee commented 1 year ago

Hi guys, I tested this and discovered that after moving a draggable with the keyboard other keyboard interactions and things broke, for example input fields losing focus automatically. Apparently adding this check fixes the issue:

const clearSelection = () => {
    // window.getSelection()?.removeAllRanges();
    if (isActiveSensor()) {
      window.getSelection()?.removeAllRanges();
    }
  };