Sortable in modal - `keydown` event propagation issue

When using useSortable in a modal, there is an issue with keyboard event propagation. Currently, keyboard events are not prevented from propagation, causing unintended behavior. The modal catches keydown event (Escape) before dnd-kit listeners do, and closes instead of canceling the drag and stopping the propagation.

Expected behavior:

I'd expect that when keyboard navigation is activated within the sortable items, keyboard events should be stopped from further propagation. This would ensure that the modal (or other components) does not receive and act upon keyboard events when it shouldn't.

Codesandbox example

https://codesandbox.io/p/sandbox/priceless-currying-8ytlml?file=%2Fsrc%2FSortable.js%3A48%2C1

Steps to reproduce

Open modal
Press Tab to focus on one of the items
Press Space or Enter to activate keyboard listeners
Press Escape

Notes

I tried to prevent event propagation before invoking useSortable listeners, but this breaks the cancel functionality. I discovered that these events are attached to the document, causing them to be fired at the very end, which looks like the main issue. To fix this, I created MyKeyboardSensor and overrode attach() and detach() methods to set events directly to the target, and added event.stopPropagation(), this fixed my issue. I'm curious is there any particular reason why events are set to the document and not to the target itself?

class MyKeyboardSensor extends KeyboardSensor {
  private attach() {
    this.handleStart();

    this.windowListeners.add('resize', this.handleCancel);
    this.windowListeners.add('visibilitychange', this.handleCancel);

    this.props.event.target.addEventListener('keydown', this.handleKeyDown);
  }

  private detach() {
    this.props.event.target.removeEventListener('keydown', this.handleKeyDown);
    this.windowListeners.removeAll();
  }

  private handleCancel(event: Event) {
    const { onCancel } = this.props;

    event.stopPropagation();
    event.preventDefault();
    this.detach();
    onCancel();
  }
}

Also, I think listeners lack blur event handling, I'd expect that drag is canceled when this event is fired on the target element. Let me know if I should create a separate issue for this.

By the way, thanks for creating such an awesome library!

@wolkyura Thanks for getting me pointed in the right direction. Here is what I have come up with. It handles your methodology while adhering to the KeyboardSensor's methodology as well. It also handles blur cancellation effectively. Note that I have also included how to make this play niceley with typescript

@clauderic I have been messing with this for days and I think this combination is pretty solid and could be easily placed into core. I don't have time at this very second to make a PR but I would be happy to.

Custom keyboard sensor class

import { KeyboardSensor, KeyboardSensorProps } from "@dnd-kit/core";

/**
 * Creates a new keyboard sensor, extending the default `KeyboardSensor` class.
 */
export default class CustomKeyboardSensor extends KeyboardSensor {

    constructor(props: KeyboardSensorProps) {
        super(props);
        /**
         * Bind the new methods to the instance.
         */
        this.handleBlur = this.handleBlur.bind(this);
    }

    /**
     * Override the inherited `attach` method to add our new event handlers.
     */
    protected attach() {
        this.handleStart();

        this.windowListeners.add("resize", this.handleCancel);
        this.windowListeners.add("visibilitychange", this.handleCancel);

        setTimeout(() => {
            /**
             * Updates this event listener to fire in the capture phase.
             */
            this.listeners.add("keydown", this.handleKeyDown, { capture: true });
            /**
             * Add a blur event listener in the capture phase.
             */
            this.listeners.add("blur", this.handleBlur, { capture: true });
        });
    }

    /**
     * When the keyboard moves the active item around, it fires
     * blur events, however in those events the `relatedTarget`
     * is null. Check for the existence of this key to determine
     * if the event should fire a cancel or not.
     */
    protected handleBlur(event: FocusEvent) {
        if (event.relatedTarget instanceof HTMLElement) {
            this.handleCancel(event);
        }
    }

    /**
     * Override the inherited `handleEnd` method and stop propagation.
     * This is helpful because things like modals listen to escape keys,
     * and it is possible that someone marks the escape key as a trigger
     * for `end`.
     */
    protected handleEnd(event: Event) {
        const {onEnd} = this.props;

        event.preventDefault();
        event.stopPropagation();

        this.detach();
        onEnd();
    }

    /**
     * Override the inherited `handleCancel` method and stop propagation.
     * This is helpful because things like modals listen to escape keys,
     * and it is likely that people use the escape key to trigger a `cancel`,
     * is is done in the default implementation.
     */
    protected handleCancel(event: Event) {
        const {onCancel} = this.props;

        event.preventDefault();
        event.stopPropagation();

        this.detach();
        onCancel();
    }
}

In addition, I have declared the default KeyboardSensor class interface as an ambient declaration on the module. I mapped the methods a little better than the type that is being exported by the lib, marking the methods as protected instead of private so I could extend them. In general, the default sensors should probably use protected members and methods instead of private ones so users can override them without the headache if needed.

One final note on exports: The Listeners class and EventName enum are not exported from the lib at all, would be nice in this situation. I also noticed the defaultKeyboardCodes const is not exported either, I am not using it now, but was in a previous attempt. so exporting those could also be nice for users.

Typescript declaration file (`*.d.ts`)

import { Activators, KeyboardSensorOptions, KeyboardSensorProps, SensorInstance } from "@dnd-kit/core";

declare module "@dnd-kit/core" {

    declare class Listeners {
        constructor(target: EventTarget | null);
        public add<T extends Event>(eventName: string, handler: (event: T) => void, options?: AddEventListenerOptions | boolean);
        public removeAll();
    }

    declare class KeyboardSensor implements SensorInstance {
        protected props: KeyboardSensorProps;
        autoScrollEnabled: boolean;
        protected referenceCoordinates: Coordinates | undefined;
        protected listeners: Listeners;
        protected windowListeners: Listeners;
        constructor(props: KeyboardSensorProps);
        protected attach();
        protected handleStart();
        protected handleKeyDown(event: Event);
        protected handleMove(event: Event, coordinates: Coordinates);
        protected handleEnd(event: Event);
        protected handleCancel(event: Event);
        protected detach();
        static activators: Activators<KeyboardSensorOptions>;
    }
}

clauderic / dnd-kit