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

Dragging and link issue #71

Closed
armantaherian opened this issue Aug 6, 2017 · 40 comments
Closed

Dragging and link issue #71

armantaherian opened this issue Aug 6, 2017 · 40 comments
Labels
bug

Comments

@armantaherian
Copy link

@armantaherian armantaherian commented Aug 6, 2017

Just drag here:
https://s.codepen.io/armantaherian/debug/eEgRGM/nqMwvedyKpXk
You will see the issue, I’m dragging, but the page will go to Google.

Codepen: https://codepen.io/armantaherian/pen/eEgRGM

@armantaherian armantaherian changed the title Drag and link issue Dragging and link issue Aug 6, 2017
@yidemir

This comment has been minimized.

Copy link

@yidemir yidemir commented Sep 10, 2017

Is there another solution to this?

@pawelgrzybek pawelgrzybek added the bug label Oct 17, 2017
@WhereJuly

This comment has been minimized.

Copy link

@WhereJuly WhereJuly commented Oct 17, 2017

@pawelgrzybek
I cannot open the pen but I suspect this is not the bug. As I understand, the issue is that a user is bothered by swipe AND click effect at the same time so that click leads to the page change while the user expects to swipe the slider.

The solution is to split the space of the slider item wrapper into two parts. One should react to swiping the other to click.

In practice:

  1. avoiding to wrap the entire slider item element into a link and
  2. add the second element to the item wrapper that would be a link.
    Can you give me the public pen on the issue? I could have look and probably find the solution.
@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Oct 17, 2017

Hi all.

There are 2 possible solutions:

  1. Ignore swiping / sliding / dragging event when mousedown occurred on clickable element like link, form input etc.
  2. Totally agreed with @WhereJuly — avoid wrapping whole slides in carousel items. This is anti-pattern and can drive to lots of confusions and UX quirks (not Siema specific but generally)

I'll look at the possible solutions and decide about it later.

Thanks for surrestions @WhereJuly and reporting @armantaherian

@WhereJuly

This comment has been minimized.

Copy link

@WhereJuly WhereJuly commented Oct 17, 2017

@pawelgrzybek thank you for the Siema 👍

Shortly I will add the pen to show how to get the both (or even more) behaviours at the same time.

Meanwhile I spent good three hours trying to separate the mouseup (pointerup) from click events triggered by the same Siema item wrapper element.

However I tried to set my own event listener above or at the same element as Siema item wrapper. However I tried to catch the click on bubbling or captturing phase I could not make to separate it from Siema's mouseup event. So if somebody would resolve this efficiently (I mean with minimum amount of code) for Siema case not touching Siema's code, just let me know. I am really interested if this is possible at all.

@WhereJuly

This comment has been minimized.

Copy link

@WhereJuly WhereJuly commented Oct 17, 2017

Here is the pen combining 3 behaviors on each Siema slide:

  • drag
  • click to open a popup
  • click to download a file.

One can combine as many behavors as required not sacrificing the main slider's functionality.
@pawelgrzybek if this was the issue of the current thread and the pen solves it, please feel free to rework the pen to add to the Siema's CodePen how-to library.

@matass

This comment has been minimized.

Copy link

@matass matass commented Oct 30, 2017

Hi all. @armantaherian, @yidemir, @WhereJuly I have made a simple workaround.
@pawelgrzybek I could make a Pull request. let me know if this workaround is valid.
pen - https://codepen.io/anon/pen/NwPjPa

@r3wt

This comment has been minimized.

Copy link

@r3wt r3wt commented Nov 2, 2017

@pawelgrzybek please respond immediately sir. i have to push an update into prod by saturday night or its my a$$... i bet the farm on siema(great project btw, good job man) and now i've ran into this bug. will you accept a pr????? like today????

@r3wt

This comment has been minimized.

Copy link

@r3wt r3wt commented Nov 2, 2017

version of @matass workaround that should work with multiple sliders and slides appended at anytime ('assuming they all are named like "siema-")

    var dragging, href, el;
        dragging = false;
        href = '';

    $(document).on('mousedown','[class*="siema-"] a',function() {
        el = $(this);
        dragging = false;
        href = $(this).attr('href');
    })

    $(document).on('mousemove','[class*="siema-"] a',function() {
        dragging = true;
    })

    $(document).on('mouseup','[class*="siema-"] a',function() {
        var wasDragging;
        wasDragging = dragging;
        dragging = false;
        if (wasDragging) {
            $(this).attr('href', null);
            setTimeout((function() {
            el.attr('href', href);
            }), 10);
        }
    });
@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Nov 3, 2017

I won't accept any PR today for sure. This is going to be fixed in 1.5 and I am planing to release it very soon (but not today or tomorrow). Sorry.

@manuman94

This comment has been minimized.

Copy link

@manuman94 manuman94 commented Nov 22, 2017

Hi! Thanks for Siema, has become part of my favourite lightweight libraries.

I'm interested in this too, if you try to implement it into a product slider shop, it's important to handle the click event in order to work only when the slider is not moving.

Awesome work pawelgrzybek ;)

@jacobdubail

This comment has been minimized.

Copy link

@jacobdubail jacobdubail commented Nov 29, 2017

+1 for this feature. Looking forward to 1.5!

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 9, 2018

Hi guys.

You were waiting long enough, sorry about it :)
https://github.com/pawelgrzybek/siema/releases/tag/v1.4.10

And this is a CodePen link with a presented solution. Hopefully it is what you expected.
https://codepen.io/pawelgrzybek/pen/dJdOyj

Have a lovely day you all 🥑

@manuman94

This comment has been minimized.

Copy link

@manuman94 manuman94 commented Jan 9, 2018

Awesome! Thank you man ;)

@manuman94

This comment has been minimized.

Copy link

@manuman94 manuman94 commented Jan 9, 2018

I'm testing it in this widget:
http://test.weecomments.com/tester/test_widget/vertical/1/PDgYyY4Cpu

Why is it behaving like in the previous version?

Siema is updated and is taking users to the link even if they are dragging.

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 9, 2018

I see what is going on. You are not dragging a link. You have tons of markup inside a link (spans and divs), which can be easily prevented by user (you). Siema internally checks just for a a tags:

https://github.com/pawelgrzybek/siema/blob/master/src/siema.js#L425-L427

You can ignore pointer events on spans and divs even with pure CSS via pointer events: none. There you go, an example :-)

https://codepen.io/pawelgrzybek/pen/aEqpMO

Hopefully it makes sense for you :)

@manuman94

This comment has been minimized.

Copy link

@manuman94 manuman94 commented Jan 9, 2018

Ok! Let me try and I tell you. Thank you so much, I didn't think about that fact.

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 10, 2018

I tried a codepen from #71 (comment) and it still redirects user to the link, even when you drag it and slide gets changed.

@matass

This comment has been minimized.

Copy link

@matass matass commented Jan 10, 2018

@aczekajski, have you tried this pen – https://codepen.io/anon/pen/NwPjPa ?

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 10, 2018

Please make sure that CodePen fetched the updated version of Siema from GitHub Pages please. Just rechecked it and everything is working fine, as expected.

dragging link on Siema

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 10, 2018

I've updated the link to siema library in the codepen to current minified file in master branch and now it cannot be clicked at all:
https://codepen.io/anon/pen/KZoXzp

You can drag it but you cannot navigate to the link. It's the same with 1.4.10.

(tested in Chrome 63)

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 10, 2018

Hi @aczekajski.

Just tested your CodePen on 2 machines. On iOS and Windows. Everything is working as expected for me. Just for the sake of it I tested it on IE11 — works like a charm.

What OS are you on, Do you use some custom input devices that may trick the order of event invocations?

screenflow

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 10, 2018

It sounds strange but it started working after restarting Chrome...

However, I've found a scenario where first click doesn't work. If you drag the slide but the drag ends outside the Siema main wrapper. After that, first click doesn't navigate, subsequent clicks do.

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 11, 2018

I see what is going on. There isa prop drag.prevenetClick that I added to resolve this issue. I don't handle this one well on moseleeaveHandler() method. Great spot @aczekajski.

Thanks for involvement, and I'll address next fix to this scenario.

aczekajski added a commit to x-kom/siema that referenced this issue Jan 11, 2018
@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 11, 2018

By the way this logic behind preventing click event sounds really fishy for me. Why only preventing navigate when the target is a link? There are tons of scenarios where someone might want to have something clickable on the slide, but not neccesarily a link.

Now I'm only fiddling with the code but I believe it might be cool if there was an option to prevent events when the slider was dragged. For example if I did

if (this.drag.preventClick) {
      e.stopPropagation();
      e.preventDefault();
}

inside clickHandler function, and modified attachEvents function changing addEventListener for click event like this:

this.selector.addEventListener('click', this.clickHandler, true); // useCapture parameter is changed to "true"

then I can prevent the event handler provided by the user from firing. But it of course needs some more testing since changing useCapture of event might have some serious consequences. Of course this behaviour should be opt-in.

You can see the work-in-progress version with the effect here:
https://codepen.io/anon/pen/jYxPeq
(now it's hardcoded, without opt-in/opt-out)

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 11, 2018

Hi.

It is not a true that click is prevented only for links. It checks for links, textareas, options, inputs and select boxes.

Your solution with conditional event firing and events capturing will break when you work with nested carousels (wich is fully tested and well supported back to IE10).

By the way, your issue with prevented click when dragging left the selector has been fixed: 634ab1c

Thanks 🥑

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jan 11, 2018

Quick comment to your CodePen:

a.addEventListener('click', () => console.log('klik a'));
b.addEventListener('click', () => console.log('klik b'));
c.addEventListener('click', () => console.log('klik c'));

const mySiema = new Siema();
// when you drag, it won't trigger event listeners for a, b, c elements

This is absolutely expected and intentional. On a, b, c elements the click event shouldn't be triggered when you drag it. Mosedown — yeep.

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 11, 2018

Please comapre behaviour:
https://codepen.io/anon/pen/PEeWNw <- Siema 1.4.12
https://codepen.io/anon/pen/jYxPeq <- my modification of Siema 1.4.11

Which behaviour is more expected? For me, it is the latter, where click event doesn't get triggered after dragging the slide.

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Jan 11, 2018

According to #71 (comment) - is there a codepen with example of nested carousels so I can test it?

@aczekajski

This comment has been minimized.

Copy link

@aczekajski aczekajski commented Feb 13, 2018

@pawelgrzybek ping ;)

@paptito

This comment has been minimized.

Copy link

@paptito paptito commented Apr 23, 2018

I'm having this issue when wrapping images inside of anchors, any solution to this?

EDIT: Seems like it happened when the slider was wrapped in a floated div.

@grig0ry

This comment has been minimized.

Copy link

@grig0ry grig0ry commented May 4, 2018

Up!

@rafaelderolez

This comment has been minimized.

Copy link

@rafaelderolez rafaelderolez commented Jul 9, 2018

@pawelgrzybek this still seems to break as soon as there is another tag inside the anchor tag.
https://codepen.io/rafaelderolez/pen/JBPZKJ

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Jul 9, 2018

Hi @rafaelderolez.

I am aware of it and this issue is going to be addressed in next release.

Thanks a lot for reporting and using Siema :)

@creativecat

This comment has been minimized.

Copy link

@creativecat creativecat commented Sep 3, 2018

I still had the same problem with v1.5.1. I had to bind a link on the click event...
I fixed the problem for me by checking how far the user moved the mouse on click.

$el.find('a')
	.mousedown(function(e) {
		$(this).data({top: e.pageX, left: e.pageY});
	})
	.mouseup(function(e) {
		var top   = e.pageX,
			left  = e.pageY,
			ptop  = $(this).data('top'),
			pleft = $(this).data('left');
		$(this).data('dragged', Math.abs(top - ptop) > 15 || Math.abs(left - pleft) > 15);
	});

And at the beginning of the binding:

	$('.class').on( 'click', function(e)
	{
		e.preventDefault();
		var $cur = $(this);
		if ( $cur.data('dragged') )
			 return;
		...
	}
@wiseoldman

This comment has been minimized.

Copy link

@wiseoldman wiseoldman commented Nov 14, 2018

I also had some problems with this ended up writing this code (might not work in all cases though) and added it to the onInit function:

Siema.prototype.preventLinkClick = function (selector) {
  const CONTAINER = this.selector.parentNode;
  const LINKS = CONTAINER.querySelectorAll(selector);

  LINKS.forEach(el => {
    let firstMouseX = 0;

    el.addEventListener('mousedown', (e) => firstMouseX = e.clientX);

    el.addEventListener('click', (e) => {
      let lastMouseX = e.clientX;
      let diffMouseX = firstMouseX - lastMouseX;

      if (diffMouseX > 1 || diffMouseX < -1) {
        e.preventDefault();
      }
    });
  });
}
new Siema({
  onInit() {
    this.preventLinkClick('.mtt-post-slider__slide > a');
  }
});
@xinxilas

This comment has been minimized.

Copy link

@xinxilas xinxilas commented Dec 18, 2018

I made this removing HREF from A if the mousediff is bigger than 3
And adding again 50 milliseconds later:

Did some changes to work on IE11 too:

Siema.prototype.preventLinkClick = function (selector) {
  const CONTAINER = this.selector.parentNode;
  const LINKS = CONTAINER.querySelectorAll(selector);
  
	Array.prototype.forEach.call(LINKS, function(el) { 

		let firstMouseX = 0;

		el.addEventListener('mousedown', function(e) {
			firstMouseX = e.clientX;
		});

		el.addEventListener('mouseup', function(e) {
			let lastMouseX = e.clientX;
			let diffMouseX = firstMouseX - lastMouseX;
			if (diffMouseX > 3 || diffMouseX < -3) {
				let href = el.getAttribute("href")
				el.removeAttribute("href");
				setTimeout(function () {el.setAttribute("href",href);}, 50);
			}
		});
		
	});
}

Finnally! :D

@xinxilas

This comment has been minimized.

Copy link

@xinxilas xinxilas commented Dec 18, 2018

PS: i believe your functions isnt really working for you

@wiseoldman

This comment has been minimized.

Copy link

@wiseoldman wiseoldman commented Dec 18, 2018

PS: i believe your functions isnt really working for you

It's working fine in all cases I've tested it, you shouldn't need to anything else than prevent the default behavior of the link.

@vielhuber

This comment has been minimized.

Copy link

@vielhuber vielhuber commented Dec 22, 2018

Is this solved?

@pawelgrzybek

This comment has been minimized.

Copy link
Owner

@pawelgrzybek pawelgrzybek commented Dec 22, 2018

@vielhuber I don't maintain this lib anymore. I consider it an anti-pattern. Feel free to resolve it yourself and I am more than happy to accept you PR.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.