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.

Sign up for GitHub

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

Jump to bottom

[DO NOT MERGE] feat: allow single pointer drag #53

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

[DO NOT MERGE] feat: allow single pointer drag #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c855e62
feat: allow single pointer drag
CecoMilchev Jul 25, 2023
c2aaca2
feat: add cancel event
CecoMilchev Aug 10, 2023
9ecdca6
feat: add cancel event when mouseonly
CecoMilchev Aug 11, 2023
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
2 changes: 2 additions & 0 deletions src/main.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ interface DraggableOptions {
press?: Function;
drag?: Function;
release?: Function;
cancel?: Function;
mouseOnly?: boolean;
clickMoveClick?: boolean;
Comment on lines +5 to +7
Copy link

Choose a reason for hiding this comment

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

Some suggetions for naming changes are posted in the spec:
#52 (comment)

}

export class Draggable {
Expand Down
94 changes: 82 additions & 12 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const touchRegExp = /touch/;
// 300ms is the usual mouse interval;
// // However, an underpowered mobile device under a heavy load may queue mouse events for a longer period.
const IGNORE_MOUSE_TIMEOUT = 2000;
const ESC = 27;
Copy link

Choose a reason for hiding this comment

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

We better extract this to an enum-like entity like KeyCode:

https://github.com/telerik/blazor/blob/4f624d14e1ed064dc0ce769b154f258667beb769/js/telerik-blazor/src/common/navigation/keys.ts#L6

Other keys can be added to this enum and it can be reused.


function normalizeEvent(e) {
if (e.type.match(touchRegExp)) {
Expand Down Expand Up @@ -51,16 +52,19 @@ export class Draggable {

get document() {
return this._element
? this._element.ownerDocument
: document;
? this._element.ownerDocument
: document;
}

constructor({ press = noop, drag = noop, release = noop, mouseOnly = false }) {
constructor({ press = noop, drag = noop, release = noop, cancel = noop, mouseOnly = false, clickMoveClick = false }) {
this._pressHandler = proxy(normalizeEvent, press);
this._dragHandler = proxy(normalizeEvent, drag);
this._releaseHandler = proxy(normalizeEvent, release);
this._cancelHandler = proxy(normalizeEvent, cancel);
this._ignoreMouse = false;
this._isDragging = false;
this._mouseOnly = mouseOnly;
this._clickMoveClick = clickMoveClick;

this._touchstart = (e) => {
if (e.touches.length === 1) {
Expand Down Expand Up @@ -92,12 +96,31 @@ export class Draggable {
const { which } = e;

if ((which && which > 1) || this._ignoreMouse) {
Copy link

Choose a reason for hiding this comment

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

Note that event.which is marked as deprecated, we can think about alternatives like https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons and https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

bind(this.document, "contextmenu", preventDefault);
Copy link

Choose a reason for hiding this comment

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

We better add a comment on why this is necessary (e.g. add info that we need the right mouse button for canceling the drag action).

It is also worth it to consider if there is a very very specific case if you stop dragging on something, but expect to open the context menu, e.g. if customers have added a ContextMenu component and expect it to be opened.


this.cancelDrag(e);

this._cancelHandler(e);
Comment on lines +101 to +103
Copy link

Choose a reason for hiding this comment

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

It seems like we better reverse the order and act depending on whether the dragCancel event is prevented or not - consider the scenario where consumers of the package are notified for the dragCancel event, but they prevent it - then the dragCancel event should allow not cancelling the dragCancel event, thus the cancelDrag function should not be executed.

The above is only true if we actually want to allow preventing events, which I don't think is currently supported in the kendo-draggable package. Still, it is still valid to think if reversing the order is better as we notify for the occurrence of the dragCancel and then we actually cancel it. It will also help if we introduce something like dragCancelEventArgs.preventDefault someday.


return;
}

bind(this.document, "mousemove", this._mousemove);
bind(this.document, "mouseup", this._mouseup);
this._pressHandler(e);
if (!this._clickMoveClick) {
bind(this.document, "mouseup", this._mouseup);
bind(this.document, "mousemove", this._mousemove);
bind(this.document, "contextmenu", preventDefault);
Copy link

Choose a reason for hiding this comment

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

This event wasn't prevented in the original version (and will prevent the "contextmenu", which might be interpreted as a regression) - it seems better not to have it here, but only when the lick-move-click dragging option is enabled. Am I missing something?

this._pressHandler(e);
} else {
if (this._isDragging) {
bind(this.document, "mouseup", this._mouseup);
} else {
bind(this.document, "mousemove", this._mousemove);
Comment on lines +114 to +117
Copy link

Choose a reason for hiding this comment

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

We can add a comment about why isDragging can be true here - in the general case, it is not possible for the "start" event to be triggered after a "move" action has taken place, but we want exactly this here, so an explanation would be helpful.

bind(this.document, "keydown", this._keydown);
this._pressHandler(e);
}
Comment on lines +108 to +120
Copy link

Choose a reason for hiding this comment

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

We can reverse the condition:

            if (this._clickMoveClick) {
                if (this._isDragging) {
                    bind(this.document, "mouseup", this._mouseup);
                } else {
                    bind(this.document, "mousemove", this._mousemove);
                    bind(this.document, "keydown", this._keydown);
                    this._pressHandler(e);
                }
            } else {
                bind(this.document, "mouseup", this._mouseup);
                bind(this.document, "mousemove", this._mousemove);
                bind(this.document, "contextmenu", preventDefault);
                this._pressHandler(e);
             }

By doing this we ask for a positive flag - whether dragging is enabled in a click-move-click manner instead of asking if it is not enabled. We can add other specific conditions in the future and they will appear at the top - so it is better for you to see all specific cases - and then the "default" behavior is left as a fallback at the end.


this._isDragging = !this._isDragging;
}
};

this._mousemove = (e) => {
Expand All @@ -107,17 +130,41 @@ export class Draggable {
this._mouseup = (e) => {
unbind(this.document, "mousemove", this._mousemove);
unbind(this.document, "mouseup", this._mouseup);

if (this.clickMoveClick){
unbind(this.document, "keydown", this._keydown);
unbind(this.document, "contextmenu", preventDefault);
}
Comment on lines +135 to +137
Copy link

Choose a reason for hiding this comment

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

Do we need to unbind other events, e.g.:

https://github.com/telerik/kendo-draggable/pull/53/files#diff-27653c212e1cfe533e4eb2f7d0d3f89604c9de48a09583b4cbbbcbd08a07da79R152


this._releaseHandler(e);
};

this._pointerdown = (e) => {
if (e.isPrimary && e.button === 0) {
bind(this.document, "pointermove", this._pointermove);
bind(this.document, "pointerup", this._pointerup);
bind(this.document, "pointercancel", this._pointerup);
bind(this.document, "contextmenu", preventDefault);

this._pressHandler(e);
if (!this._clickMoveClick) {
bind(this.document, "pointerup", this._pointerup);
bind(this.document, "pointercancel", this._pointerup);
bind(this.document, "contextmenu", preventDefault);
bind(this.document, "pointermove", this._pointermove);
this._pressHandler(e);
} else {
if (this._isDragging) {
bind(this.document, "pointerup", this._pointerup);
bind(this.document, "pointercancel", this._pointerup);
} else {
bind(this.document, "pointermove", this._pointermove);
bind(this.document, "keydown", this._keydown);
this._pressHandler(e);
Comment on lines +146 to +158
Copy link

Choose a reason for hiding this comment

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

It seems to me that it is easy for us to attach more than 1 event handler here for the same event as we have some logic for binding and unbinding events.

As we do not want to have more than 1 handler - what about unbinding the event in the bind function and re-attaching it? This should save us from potential duplications - it would have been nice if we can check if the event has been attached already, but this will require more code.

}

this._isDragging = !this._isDragging;
}
}

if (this._clickMoveClick && e.button == 2) {
Copy link

Choose a reason for hiding this comment

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

We can extract an enum like MouseButton.Left | Right | Middle as numbers like 2 are not very semantic.

this.cancelDrag(e);
this._cancelHandler(e);
}
};

Expand All @@ -134,14 +181,25 @@ export class Draggable {
unbind(this.document, "pointercancel", this._pointerup);
unbind(this.document, "contextmenu", preventDefault);

if (this.clickMoveClick){
unbind(this.document, "keydown", this._keydown);
Copy link

Choose a reason for hiding this comment

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

Do we need to unbind other events, e.g.:

https://github.com/telerik/kendo-draggable/pull/53/files#diff-27653c212e1cfe533e4eb2f7d0d3f89604c9de48a09583b4cbbbcbd08a07da79R152

}

this._releaseHandler(e);
}
};

this._keydown = (e) => {
if (e.keyCode === ESC) {
this.cancelDrag(e);
this._cancelHandler(e);
}
Comment on lines +193 to +196
Copy link

Choose a reason for hiding this comment

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

It seems like this behavior can be an option - navigable: true.

We discussed this offline - some suites like Kendo Angular/React might want to have their own events or specific navigation and want to cancel the drag on Escape optionally.

};
}

bindTo(element) {
if (element === this._element) {
return;
return;
}

if (this._element) {
Expand All @@ -152,6 +210,18 @@ export class Draggable {
this._bindToCurrent();
}

cancelDrag() {
unbind(this.document, "pointermove", this._pointermove);
unbind(this.document, "pointerup", this._pointerup);
unbind(this.document, "pointercancel", this._pointerup);

if (this.clickMoveClick) {
unbind(this.document, "contextmenu", preventDefault);
}

this._isDragging = !this._isDragging;
Copy link

Choose a reason for hiding this comment

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

Suggested change
this._isDragging = !this._isDragging;
this._isDragging = false;

}

_bindToCurrent() {
const element = this._element;

Expand Down
Loading