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.

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

Text inputs within sortable element #972

Closed
BurninLeo opened this issue Oct 7, 2016 · 19 comments
Closed

Text inputs within sortable element #972

BurninLeo opened this issue Oct 7, 2016 · 19 comments

Comments

@BurninLeo
Copy link

BurninLeo commented Oct 7, 2016

There seems (still) to be an issue with text inputs in sortable elements, when using Firefox: When clicking a text input within a sortable element, the cursor won't jump to the position where I clicked.

Example: https://jsfiddle.net/3njqxn7k/

This issue relates to another two issues that have been closed: #629, #462

@BurninLeo
Copy link
Author

BurninLeo commented Oct 7, 2016

A possible solution would be to stop _onTapStart if the sender is a (text) input (this is code as of line 314):

        // Don't trigger start event when an element is been dragged, otherwise the evt.oldindex always wrong when set option.group.
        if (dragEl) {
            return;
        }

        // BEGIN
        var sender = (evt && evt.target) || (window.event && window.event.srcElement);
        if (sender.nodeName == "INPUT") {
            return;
        }
        // END

        if (type === 'mousedown' && evt.button !== 0 || options.disabled) {
            return; // only left button or enabled
        }

This makes the text input usable as one would expect. Of course, this makes it impossible to drag the element by clicking into the text input. Maybe it would be sensible to add a switch to Sortable() to choose the behavior that is more appropriate for the situation.

Please note that contentEditable Elements suffer the same issue.

RaneClowd added a commit to RaneClowd/Sortable that referenced this issue Nov 4, 2016
@RaneClowd RaneClowd mentioned this issue Nov 4, 2016
@crbelaus
Copy link

crbelaus commented Jan 3, 2017

I am facing the same problem. I have a form with a series of subforms that are sortable. On Chrome, when I click on a subform , it gains the focus correctly and I can write inmediatly. On Firefox, the does not gain the focus.

@rafwell
Copy link

rafwell commented Jan 6, 2017

Try my answer: #997

@AStoker
Copy link

AStoker commented Jan 26, 2017

Do we have any update here?

@AStoker
Copy link

AStoker commented Jan 27, 2017

Could you boil down the issue to saying that a filter is applied to an element (such as a text input), that click events on that "filtered" item shouldn't trigger any drag and drop functionality, but should allow the default behavior to continue (which would be the act of selecting text)? I think @BurninLeo that would solve your problem, and be more general than just a fix for inputs.

@AStoker
Copy link

AStoker commented Jan 27, 2017

Issue I believe stems from this checkin: #171
It looks like the preventDefault was added to keep the filter function from firing twice (by preventing default on the first event, you cancel out any other events).
The trouble with this is that it also cancels any default browser actions, like selecting an input. So the problem to solve is, ensure that only one event is fired. The reason we have multiple events firing is due to this block of code (https://github.com/RubaXa/Sortable/blob/master/Sortable.js#L286), where we are adding event listeners for mousedown, touchstart, and pointer down. This is probably an attempt to make sure that we have click events caught for any kind of scenario (mobile or otherwise).

Ideally, we would just nix the mousedown in favor of pointer events, since pointer events are the HTML5 spec and provide much more functionality. However, pointer events aren't fully implemented on every browser yet (sad day). So the problem to solve then, is to make sure that we only fire one kind of "mouse down" event.
With a simple enough check, we can see if pointer events are enabled:

// Bind events
if(window.PointerEvent){
   _on(el, 'pointerdown', this._onTapStart);			
} else {
   _on(el, 'mousedown', this._onTapStart);			
}

With this block, we now only have one click event fired at a time, and we still support all the browsers, and we no longer need to have the preventDefault on each trigger.
@RubaXa, do you see a problem with this? Can I go ahead and submit a PR for your review?

AStoker added a commit to AStoker/Sortable that referenced this issue Jan 27, 2017
Resolves: SortableJS#972
Related: SortableJS#1000
Removes duplicate “down” events and `preventDefault` operation on click
events on “filter” items. This allows selection of things like text
boxes and input elements that are inside a sortable element.
Repro of bug: http://jsbin.com/cerumanoma/edit?html,js,output
AStoker added a commit to AStoker/Sortable that referenced this issue Jan 27, 2017
Resolves: SortableJS#972
Related: SortableJS#1000
Removes duplicate “down” events and `preventDefault` operation on click
events on “filter” items. This allows selection of things like text
boxes and input elements that are inside a sortable element.
Repro of bug: http://jsbin.com/cerumanoma/edit?html,js,output
@BurninLeo
Copy link
Author

BurninLeo commented Jan 30, 2017

I am afraid, the solution to use pointer instead of mousdown/touchstart would create a new problem: Users of older browsers would still have the old issue, and it would become harder to find the reason. And as you said: Mousedown/touchstart is only used to maintain compatibility. I am also not sure: Does prevention of bubbling really stop the other event from firing? It should not.

  • I had a similar issue in another context recently and came up with the solution that the first event sets a "deadtime" (e.g. 100ms) within which any other related event handler just returns. If doing so, it's important to use the time stamp from the event, not the Date object, because when using Date I found delays of 300 ms between touchstart and mousedown.
  • Another solution would be to disable (detach events) touchdown when mousedown was used and the other ways around
  • A third solution would be to find something unique for the event (e.g., sort order, element contents), and to check if this has already been handled. If yes (with a timeout of e.g. 1 sec.) the other events are ignores. Yet, this quickly becomes a complicated approach as well.

@AStoker
Copy link

AStoker commented Jan 30, 2017

How would that solution create a new problem @BurninLeo? Specifically wrapping the pointer listener in a feature check if ensures that the pointer events will only be used if the browser supports it, otherwise it'll fall back to the mousedown event.
If you look right now, two events are being fired, one for pointer and one for mouse. That in and of itself is a problem, and the reason why this issue (#171) was reported. The fix for that issue though (according to the commit referenced) was to stop any other events from happening after (by using preventDefault), which was a hacky fix that causes this problem.
In either case, preventDefault causes the browser to not use the default behavior, which is incorrect when you want to click on an input. Ultimately, the preventDefault must be removed to solve the root issue of not being able to click on an input, and then a fix needs to be in place to stop multiple events from firing. Using a timer is a very inefficient approach, and does not always guarantee correct outcome. Forever attaching and detaching events is also a poor performance choice. And the third option, yes, a bit more complex answer to a simpler problem.

Thoughts?

@AStoker
Copy link

AStoker commented Jan 30, 2017

@BurninLeo, in regards to click events and preventDefault, take a look at this JSFiddle (https://jsfiddle.net/theandybob/95nksu6c/). It does appear that using preventDefault causes only one event to fire (uncomment the preventDefault to see this in action).

@BurninLeo
Copy link
Author

BurninLeo commented Jan 30, 2017

@AStoker, in my opinion, the fallback would (still) suffer the same issue like the current version. I think that generally it's a very good idea to use pointer events, but the fallback should be "safe" as well. Otherwise, you get crazy, tracking down bug reports that you cannot replicate.

Also thanks for the JSFiddle demo. It works in Firefox and Chromium, but causes two events as soon as you use the iPhone Emulator in Chromium -> developer tools (I assume, it's the same in Chrome). And I guess that other old browser versions may behave the same way. And isn't it illogical that preventing one event from bubbling prevents another event from being fired? ;)

@AStoker
Copy link

AStoker commented Jan 30, 2017

@BurninLeo, how would the fallback (which is to use mousedown) suffer the same issue? Mouse events and Pointer events are very similar, but pointer is newer and has more features on it, but not fully usable, that's why pointer has the check, and mouse is the fallback (which is universally usable).

And not entirely. If the default event is a click, I could see how you'd want that to not use any other click events after. But in either case, it doesn't matter necessarily if I think it's illogical, it's the current behavior :)

@BurninLeo
Copy link
Author

Possibly, I got the fallback wrong. I understood it that way that the fallback would include mousedown and touchstart, not only mousdown. If you removed touchstart (most touch devices support pointer events), I withdraw my point :)

To my experience it's risky to rely on the "current behavior". As I wrote, it took me no longer then 2 minutes to find a browser configuration that worked differently. I assume that other browsers and mobile devices will show a different preventDefault behavior as well. This, however, does not affect the pointer solution. I would very much apprechiate the Sortable to receive that update!

@RubaXa
Copy link
Collaborator

RubaXa commented Feb 16, 2017

Try master and use filter: 'input' + preventOnFilter: false
http://jsbin.com/tediyivoko/edit?html,css,js,output

@RubaXa RubaXa closed this as completed Feb 16, 2017
@wass08
Copy link

wass08 commented Mar 17, 2017

@RubaXa My Hero !

@pedroadame
Copy link

Thank you @RubaXa, Firefox is a pain in the ass.

@pierrewtLanca
Copy link

Hi I'm not getting it working (with vuedraggable that uses SortableJS). Applying the solution

Try master and use filter: 'input' + preventOnFilter: false
http://jsbin.com/tediyivoko/edit?html,css,js,output
didn't help, event makes things worse that no cursor at all appear. Note that I have the initial problem with Firefox only.

@pierrewtLanca
Copy link

pierrewtLanca commented Feb 24, 2021

Hi it was a mistake from my side. Setting the attributes in Draggable worked. cf SortableJS/Vue.Draggable#841 (comment)

@JPustkuchen
Copy link

Update & helpful addition to #972 (comment) for anyone who runs into this:

It might make sense to filter all HTML form elements:

// Fix form handling, see https://github.com/SortableJS/Sortable/issues/972:
filter: 'input,select,textarea,label,button,fieldset,legend,datalist,output,option,optgroup',
preventOnFilter: false,

https://www.w3schools.com/html/html_form_elements.asp

'input' otherwise only fixes this for real inputs, not the other important cases.

@JPustkuchen
Copy link

@RubaXa would it perhaps make sense to add this to the README.md as "Form handling" FAQ or something like that?

Also when reading this line in documentation:
filter: ".ignore-elements", // Selectors that do not lead to dragging (String or Function) I was not totally sure how the elements are separated, perhaps "String" should be replaced by "CSS Selector String" or something like that?
And maybe add more than one value in the example, like .ignore-elements, input?

Just some thoughts to perhaps reopen this for enhanced documentation?
Seems like there are several people running into this form case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants