stevenwanderski / bxslider-4

Responsive jQuery content slider
Other
4.22k stars 1.85k forks source link

Links in sliders aren't working in firefox 59 #1188

Open stephansch321 opened 6 years ago

stephansch321 commented 6 years ago

With the last Firefox update to version 59 links displayed inside a slide aren't working anymore. Right click on links and selecting "open in new tab" is still working, CTRL-click doesn't.

speedbomb commented 6 years ago

same here

sadsat commented 6 years ago

+1, also the inspector selection tool can not select any element inside the slider

tinogo commented 6 years ago

We are also using this library. We've tracked down issue to the PointerEvents, which got enabled for the Firefox 59 release by default. A possible workaround is the set touchEnabled: false, but this has the drawback, that you can no longer use swiping...

stephansch321 commented 6 years ago

Because of the mousedown event is the only one that is still working, we are using the following code as a quick workaround:

if (navigator.userAgent.search("Firefox") >= 0) {
    var ff_version = navigator.userAgent.match(/Firefox\/([\d]+\.[\d])+/);
    ff_version = parseFloat(ff_version[1]);
    if(ff_version == 0 || ff_version >= 59) {
        $('body').on('mousedown', '.bx-viewport a', function() {
            var ff_link = $(this);
            var ff_href = ff_link.attr('href');
            if(ff_href) {
                location.href = ff_href;
                return false;
            }
        });
    }
}
u-Azathoth commented 6 years ago

It seems that in Firefox 59.0 - when you click on slide -nothing is happened. Console log is still empty.

ifelse- commented 6 years ago

Same here. Now it's working for me. I've added condition to check slider.pointerId. Chrome returns 1 and Firefox returns 0. Once I added this statement it started working on Firefox 59.0

File: jquery.bxslider.js

2018-03-15_1459

if(slider.pointerId === 1){ slider.viewport.get(0).setPointerCapture(slider.pointerId); }

Or you can also detect Firefox browser and set slider.pointerId = 1

timholz commented 6 years ago

@ifelse – thanks a lot, that saved me a lot of trouble.

loorlab commented 6 years ago

@ifelse- Marvin Thank You ! Really !

We downloaded the minified file of this commit and it worked! --> https://github.com/Intera/bxslider-4/commit/09045b08a6176018314366ffc15cb2aa8564d440

Note : This was happening since last year but only for Firefox Developer, we reported it to the FF community https://bugzilla.mozilla.org/show_bug.cgi?id=1432363 but they could not reproduce the error.

💯 👍 🥇

Danila1985 commented 6 years ago

Due to https://www.w3.org/TR/pointerevents/#widl-PointerEvent-pointerId and https://www.w3.org/Bugs/Public/show_bug.cgi?id=21746 Compare pointerId with 1 isn't correct. Check for pointerId > 0 is the right way. My suggestion: https://github.com/stevenwanderski/bxslider-4/compare/master...Danila1985:fix/firefox59

smaug---- commented 6 years ago

FWIW, please refer to the latest versions of specs, so in this case https://w3c.github.io/pointerevents/#dom-pointerevent-pointerid

NavidZ commented 6 years ago

I'm not very familiar with this code. Can someone explain a little what that slider.pointerId is and what is so special about pointerId == 1? If something needs to be done for mouse pointerevents only shouldn't the check be pointerType == "mouse"?

lzanuz commented 6 years ago

not worked for me with firefox 59.0.1

Danila1985 commented 6 years ago

"pointerId" is identifier of pointer that can produce events. Pointer has type: "pointerType" which could be "mouse", "pen" (Pen Stylus) , "touch" (Touch Contact) or something unknown to users browser. So if you want to detect mouse events, pointerType === "mouse" is correct check. But for current purpose "pointerId" must match any active pointer: https://w3c.github.io/pointerevents/#extensions-to-the-element-interface And my tests shown that if pointerId > 0 it was active. Equal 2 or 3 for touch and 1 for mouse. So if you use pointerId === 1 you could miss touch or pen, but it will be fine with mouse. And of course value of pointerId is NOT relate to the type of pointer and it only must be type of "long" and unique.

NavidZ commented 6 years ago

@Danila1985 I see. So you want to see whether a given pointer is active or not. Based on the spec setPointerCapture API is only supposed to work on active button state pointers. So yeah it makes sense what you are doing here. But you probably want to check the buttons field and if it is not 0 then the pointer is capturable. In general you should not make any assumption on pointerId's (not even pointerId>0 as spec doesn't say anything about them and browsers can even use negative numbers). Edge is using the pointerId that is coming from the OS and in Chrome we are generating them with some incremental algorithm that we have. But either way you cannot make the assumption of >0 for any of that even though they might happen to satisfy that for now.

loorlab commented 6 years ago

@lzanuz We have this version and if it worked FF 59.0.1 👍

Danila1985 commented 6 years ago

So pointerId at least must be defined.

The pointer MUST be in its active buttons state for this method to be effective, otherwise it fails silently. Throws a DOMException with the name InvalidPointerId when the provided method's argument does not match any of the active pointers.

Does this mean that this block should be run in try-catch block and this solves the problem?

NavidZ commented 6 years ago

Again we come back to my original question that I'm not sure what the code does in this library. How do we get there in that code? Is that a event listener added for pointerdown event for example or is it touchstart? At the very least I assume that orig is a pointerevent object and when we get there we are somehow reached there from a pointer event handler otherwise how would you have that object. If all I said above is correct then the pointer is always active (not necessary active state button). If we happen to get there as a result of pointerdown listener then surely it is also in active state button as it has at least one button pressed. Either way try-catch and also checking for buttons field sounds like a good idea. Something like this:

if (slider.viewport.get(0).setPointerCapture && orig.buttons > 0) {
  slider.pointerId = orig.pointerId;
  try {
    slider.viewport.get(0).setPointerCapture(slider.pointerId);
  } catch (error) {
    console.log(error); // I don't expect to come here if we got to this function from a pointerevent listener.
  }
}
croftman commented 6 years ago

Same here, can try on this link : https://jsfiddle.net/4b3opf13/17/

Patch of ifelse works nice ! Thanks dude !

lzanuz commented 6 years ago

Maybe i'm doing something wrong. I got bxSlider 4.2.12 and changed this lines of code (line 1108) like are suggested here, but still not working on firefox 59.

smaug---- commented 6 years ago

could you link to which code you changed and how? I hope it isn't using any explicitly pointerId values.

lzanuz commented 6 years ago

My firefox is 59.0.1 (64-bit) linux version. I tried the NavidZ code solution and I still can't click on image links on bxSlider

croftman commented 6 years ago

@smaug---- Juste here

smaug---- commented 6 years ago

ahaa, I think the issues is not really in pointerevents, but about click handling. After you set the capture, mouseup, which is used for triggering click, doesn't go to the same element as mousedown. For some reason Chrome fires the click event on element, even though per spec that shouldn't happen. https://jsfiddle.net/g1z8eamu/ (see JS console. In Chrome click has same event path as mousedown, but not the same as mouseup. The anchor element isn't even in the mouseup path!)

@NavidZ we can take this particular issue elsewhere, but I'm curious to know why Chrome behaves that way. Almost as if it takes pointer capture into account when dispatching mouseup, but doesn't when handling click. That is... unexpected.

NavidZ commented 6 years ago

@smaug---- you are right. I was playing with the code and found the same problem. Here is a small code that reproduces the problem without this library: https://output.jsbin.com/geliweq

I agree that Chrome is inconsistent in sending the click when pointer capture is in place but FF doesn't send any click at all in the above example. I tried make Chrome consistent as part of this issue w3c/pointerevents#75 as we agreed on the behavior there but we got some regressions with some existing websites and I had to revert it. However, I expect to get some sort of click in FF in one of the targets at least. Don't you think?

smaug---- commented 6 years ago

Given current Gecko, where its click handling follows the old UIEvent spec model, (still vaguely in the spec "The click event MAY be preceded by the mousedown and mouseup events on the same element,"), click isn't really expected.

NavidZ commented 6 years ago

I see. I remember reading the first common parent of mousedown/up targets for click. I don't seem to find it anywhere in uievents spec though. That's what Chrome does. Edge does the same as well I believe (I don't have a Windows now to test this). But Edge and Chrome were sending click to different targets if I recall correctly when pointer capture was in place. But they still sent the click event to somewhere. So what do you think the way forward is here? Can you imagine a workaround for this library?

smaug---- commented 6 years ago

Obviously it can't rely on blink's behavior, since that is really odd. Anchor under an element which has pointer captured really shouldn't get click, and shouldn't be ever activated. So, I guess the first question is why the library uses pointer capture in this case.

If it really must do so, it could store composedPath on pointer/mousedown and on pointer/mouseup try to activate a link using the path. The click even blink dispatches should be preventDefaulted.

And to clarify, even if UA dispatched a click event, it should go to the element capturing the pointer, not to the anchor element. So click event wouldn't know anything about the anchor element.

Danila1985 commented 6 years ago

You right, the first question is why the library uses pointer capture in this case. I removed code connected with pointer capture and library still works fine except one moment: Firefox59 cannot scroll slides with mouse drag. But with touch on mobile version it works perfect. Also it works on Chrome65, Opera51, Opera52, IE10. And from the beginning: 1) Master branch of library works fine on all browsers except FF59 2) FF59 on line 1111 has orig.pointerId with 0 value 3) This doesn't throw any exceptions 4) Also click on anchor doesn't any effect like after preventDefault 5) Sliding with mouse drag works fine.

So another solution for solving issue is removing code with pointer capture. At first sight browsers works fine without it. Except desktop FF59 but this is another or new problem. I think this code used to handle manual sliding and it works well enough. But desktop FF59 prevents click event of anchors. Is something wrong with FF59? And my strange question: Is the pointerId === 0 is normal?

smaug---- commented 6 years ago

pointerId values can be anything, per specification. -1, -1000, 0, 100000. Scripts shouldn't assume anything about the id value. Id is just an identifier for the pointer.

FF59 doesn't prevent click event for anchor if down/up events happen on it.

And the fact that master branch works on other browsers which have pointer events supported is because they don't follow the specification. Click event should not be fired to the anchor element under capturing element.

Danila1985 commented 6 years ago

Thanks for the answer. But pointerdown event binded to patent div of anchor. Does this mean that if I bind pointerdown/up event to parent element theirs childs anchors click will be silently ignored?

smaug---- commented 6 years ago

Click event shouldn't go to their child nodes yes, because capture is preventing them to get pointer/mouseup events.

Danila1985 commented 6 years ago

This code shows order of events, difference between blink and gecko : http://output.jsbin.com/xeyuves Also shows importance of preventing default behaviour in pointer events callbacks with pointer capture. With preventDefault sequence of callback is much more expectable. Without preventing even Firefox may does something strange. For example: after pointer capture in pointerdown and pontermove, child element fire pointercancel after pointercancel of parent and before parents lostpointercapture . Of course code shows that Chrome, Opera and etc. fire click after pointer events and Firefox doesn't. So my new attempt consist from preventing default behaviour, catching and triggering click event with touch event support. https://github.com/stevenwanderski/bxslider-4/compare/master...Danila1985:fix/firefox59

allesan commented 6 years ago

This is still not working for me on FF 59.0.2, Linux.

EliasKotlyar commented 6 years ago

Can confirm . Its not working on FF60 on Linux & FF59 on Windows 10.

smaug---- commented 6 years ago

Hmm, FF60 should have the same broken behavior what Chrome and Edge have. (which is temporary. It will be removed from all the browsers)

derhansen commented 6 years ago

The new version 4.2.15 fixes the problem for me~

With Firefox 60, links now get opened double. Anyone with the same problem?

bcreeves commented 6 years ago

4.2.15 fixes some links but not others. Links that contain an image work but a plain text link still does not.

demaier commented 6 years ago

Any update on this? I still have the manually tweaked file as a temporary fix. Thanks!

ashish-steg commented 5 years ago

We are also using this library. We've tracked down issue to the PointerEvents, which got enabled for the Firefox 59 release by default. A possible workaround is the set touchEnabled: false, but this has the drawback, that you can no longer use swiping...

Thanks, but its not working in small device(mobile & tablet)

gabrielevo commented 5 years ago

We are also using this library. We've tracked down issue to the PointerEvents, which got enabled for the Firefox 59 release by default. A possible workaround is the set touchEnabled: false, but this has the drawback, that you can no longer use swiping...

Thanks, but its not working in small device(mobile & tablet)

I am using if (window.matchMedia("(max-width: 767px)").matches) { } and set the touchEnabled value "true" for max-width 767px and "false" for min-width 767px. It's working that way.

Gabriel

prateekrpandey commented 5 years ago

touchEnabled with if-else worked for everything except iOS Safari. Was anyone able to figure out the reason/solution?

ayyilmaz commented 5 years ago

Remove or comment this line [ at 1074 ] or change. My problem has solved by this. slider.viewport.bind('touchstart MSPointerDown pointerdown', onTouchStart); Click to see screenshot: 4.1.12 click link problem solvement

Gorjelin commented 4 years ago

` var onTouchMove = function(e) { var orig = e.originalEvent, touchPoints = (typeof orig.changedTouches !== 'undefined') ? orig.changedTouches : [orig], // if scrolling on y axis, do not prevent default xMovement = Math.abs(touchPoints[0].pageX - slider.touch.start.x), yMovement = Math.abs(touchPoints[0].pageY - slider.touch.start.y), value = 0, change = 0; // this is swipe slider.hasMove = true;

  if(yMovement === 0) slider.hasMove = false;` // < ----- solution
fueint commented 2 months ago

This blog solved my problem: https://blog.buggerspot.com/2024/07/bxslider-links-not-working-as-expected.html