pawelgrzybek / siema

Siema - Lightweight and simple carousel in pure JavaScript
https://pawelgrzybek.github.io/siema/
Other
3.49k stars 408 forks source link

Dragging and link issue #71

Closed armantaherian closed 6 years ago

armantaherian commented 7 years ago

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

yidemir commented 7 years ago

Is there another solution to this?

WhereJuly commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@pawelgrzybek thank you for the Siema :+1:

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 commented 7 years ago

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

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

+1 for this feature. Looking forward to 1.5!

pawelgrzybek commented 6 years ago

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 commented 6 years ago

Awesome! Thank you man ;)

manuman94 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

aczekajski commented 6 years ago

I tried a codepen from https://github.com/pawelgrzybek/siema/issues/71#issuecomment-356257424 and it still redirects user to the link, even when you drag it and slide gets changed.

matass commented 6 years ago

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

pawelgrzybek commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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: https://github.com/pawelgrzybek/siema/commit/634ab1c30436ced092c7e8ce219f13f2bc05db95

Thanks šŸ„‘

pawelgrzybek commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

According to https://github.com/pawelgrzybek/siema/issues/71#issuecomment-356923218 - is there a codepen with example of nested carousels so I can test it?

aczekajski commented 6 years ago

@pawelgrzybek ping ;)

paptito commented 6 years ago

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 commented 6 years ago

Up!

rafaelderolez commented 6 years ago

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

pawelgrzybek commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

PS: i believe your functions isnt really working for you

wiseoldman commented 5 years ago

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 commented 5 years ago

Is this solved?

pawelgrzybek commented 5 years ago

@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

tkroll commented 4 years ago

I resolved this issue with:

a > * {
 pointer-events: none;
}

On my links inside carousel panels.

mattxo commented 4 years ago
this.options = 
{
    onChange: () => this.siema.drag.preventClick = true
} 

this.siema = new Siema(this.options);

Worked for me