auto.bazos.cz - Slideshow skips frames

OUTDOOOR commented 4 years ago

URL: https://auto.bazos.cz/inzerat/109598416/seat-leon-st-20-tdi-cr--fr--135kw--dsg--rv022016-.php

Browser / Version: Firefox Mobile 70.0 Operating System: Android Tested Another Browser: Yes

Problem type: Something else Description: Omitted slides Steps to Reproduce: Tap to the arrow and check how many slides it moves. Unfortunately every second slide is omitted.

Browser Configuration

None

From webcompat.com with ❤️

OUTDOOOR commented 4 years ago

Bug

softvision-sergiulogigan commented 4 years ago

Thanks for the report! I'm able to reproduce the issue on Fenix only, and only when tapping on the Next and Previous slideshow buttons. Swiping left and right, the images are not skipped.

Tested with: Browser / Version: Firefox Fenix (Preview) Nightly 190930 (🦎: 71.0a1-20190928211735) Operating System: OnePlus 6 (Android 9) - 1080 x 2280 pixels, 19:9 ratio (~402 ppi pixel density)

<button class="flickity-button flickity-prev-next-button previous" type="button" aria-label="Previous">
    <svg class="flickity-button-icon" viewBox="0 0 100 100">
        <path d="M 10,50 L 60,100 L 70,90 L 30,50  L 70,10 L 60,0 Z" class="arrow"></path>
    </svg>
</button>
<button class="flickity-button flickity-prev-next-button next" type="button" aria-label="Next">
    <svg class="flickity-button-icon" viewBox="0 0 100 100">
        <path d="M 10,50 L 60,100 L 70,90 L 30,50  L 70,10 L 60,0 Z" class="arrow" transform="translate(100, 100) rotate(180) "></path>
    </svg>
</button>

Moving to Needsdiagnosis.

karlcow commented 4 years ago

This reproduce on RDM. And when we log what is happening on t.type https://www.bazos.cz/flickity2.js

  i.handleEvent = function (t) {
    var e = 'on' + t.type;
    this[e] && this[e](t)
  },

pointerdown flickity2.js:formatted:262
pointerup flickity2.js:formatted:262
pointerdown flickity2.js:formatted:262
pointermove flickity2.js:formatted:262
pointerup flickity2.js:formatted:262
click flickity2.js:formatted:262

on mobile the same thing is happening, but with a different sequence.

pointerdown flickity2.js:1:5293
pointerup flickity2.js:1:5293
click flickity2.js:1:5293

@wisniewskit an idea why it goes twice over it.

karlcow commented 4 years ago

I also noticed that the first translate of the image (when we set breakpoint) seems to be incomplete. So that could be a reason.

karlcow commented 4 years ago

This doesn't reproduce anymore on RDM but still on the device.

The log is consistent on both RDM and the device.

pointerdown { target: svg.flickity-button-icon , buttons: 1, clientX: 363, clientY: 450, layerX: 5, layerY: 15 } flickity2.js:1:5293
pointerup { target: svg.flickity-button-icon , buttons: 0, clientX: 363, clientY: 450, layerX: 5, layerY: 15 } flickity2.js:1:5293
mousedown { target: svg.flickity-button-icon , buttons: 0, clientX: 363, clientY: 450, layerX: 5, layerY: 15 } xgemius.js:1030:60
click { target: svg.flickity-button-icon , buttons: 0, clientX: 363, clientY: 450, layerX: 5, layerY: 15 } flickity2.js:1:5293

And on the mobile client

But there's a difference in the way the activeElement is selected in

  s.prototype.onclick = function (t) {
    var e = document.activeElement;
    e && e == this.element && this.onTap(t, t)
  },

which can be re-interpreted as:

function blah(type) {
  var focused = document.activeElement;
  if (focused && focused == this.element) {
    this.onTap(type, type);
  }
}

RDM

<div class="carousel flickity-enabled is-draggable" data-flickity="{ \"fullscreen\": true, …daptiveHeight\": true }" tabindex="0">

Mobile

<button class="flickity-button flickity-prev-next-button next" type="button" aria-label="Next">

so the scope is different on this markup.

<div class="carousel flickity-enabled is-draggable" data-flickity="{ &quot;fullscreen&quot;: true, &quot;lazyLoad&quot;: 1, &quot;wrapAround&quot;: true, &quot;adaptiveHeight&quot;: true }" tabindex="0">
  <div class="flickity-viewport" style="height: 296.25px;">
    <div class="flickity-slider" style="left: 0px; transform: translate3d(-2.56%, 0px, 0px);">
      <div class="carousel-cell" style="position: absolute; left: 0%;" aria-selected="false">
        <img class="carousel-cell-image flickity-lazyloaded" src="https://www.bazos.cz/img/1/576/123087576.jpg?t=1595701436" alt="SEAT LEON ST 2.0TDI 110kw, r.v.2015, edice STYLE, NAVI, PARK">
      </div>

        <!- removed the other images for brevity -->

    </div>
  </div>
  <button class="flickity-button flickity-prev-next-button previous" type="button" aria-label="Previous">
    <svg class="flickity-button-icon" viewBox="0 0 100 100">
      <path d="M 10,50 L 60,100 L 70,90 L 30,50  L 70,10 L 60,0 Z" class="arrow"></path>
    </svg>
  </button>
  <button class="flickity-button flickity-prev-next-button next" type="button" aria-label="Next">
    <svg class="flickity-button-icon" viewBox="0 0 100 100">
      <path d="M 10,50 L 60,100 L 70,90 L 30,50  L 70,10 L 60,0 Z" class="arrow" transform="translate(100, 100) rotate(180) "></path>
    </svg>
  </button>
  <ol class="flickity-page-dots">
    <li class="dot" aria-label="Page dot 1"></li>
    <li class="dot is-selected" aria-label="Page dot 2" aria-current="step"></li>
    <li class="dot" aria-label="Page dot 3"></li>

        <!- removed the code for brevity -->

  </ol>
  <button class="flickity-button flickity-fullscreen-button flickity-fullscreen-button-view" aria-label="View full-screen" title="View full-screen">
    <svg class="flickity-button-icon" viewBox="0 0 32 32">
      <path d="M15,20,7,28h5v4H0V20H4v5l8-8Zm5-5,8-8v5h4V0H20V4h5l-8,8Z"></path>
    </svg>
  </button>
  <button class="flickity-button flickity-fullscreen-button flickity-fullscreen-button-exit" aria-label="Exit full-screen" title="Exit full-screen">
    <svg class="flickity-button-icon" viewBox="0 0 32 32">
      <path d="M32,3l-7,7h5v4H18V2h4V7l7-7ZM3,32l7-7v5h4V18H2v4H7L0,29Z"></path>
    </svg>
  </button>
</div>

this.element on both RDM and mobile is <button class="flickity-button flickity-prev-next-button next" type="button" aria-label="Next">

so on RDM, we get a difference. and it exit the function. while on mobile it goes to

  s.prototype.onTap = function () {
    if (this.isEnabled) {
      this.parent.uiChange();
      var t = this.isPrevious ? 'previous' : 'next';
      this.parent[t]()
    }
  },

karlcow commented 4 years ago

I wonder why the document.activeElement is different on desktop RDM and mobile. @wisniewskit do you know why? given what I have written in the previous comment.

OUTDOOOR commented 4 years ago

It is almost one year from my first post about this bug and is still not solved. Now is this bug also in regular Firefox browser for Android...

karlcow commented 4 years ago

@OUTDOOOR yes it is. note that it doesn't necessary mean it's an issue with Firefox.

karlcow commented 4 years ago

(ignore this)

on RDM

on the device

On RDM we have slightly different numbers which are carried away in the computation of slidewidth, etc. This is the case for the button like here, but also for the slides. The numbers in general on RDM are better rounded than the number on the device.

RDM

Device

karlcow commented 4 years ago

Ah no. This was my mistake. I had a different simulated size. The numbers are the same with the right size.

wisniewskit commented 3 years ago

Hmm. I see the same image-skipping in the RDM, or at least sometimes? (edit: it might be because I have pointer events enabled on my desktop build in about:config).

But at any rate, when I tried adding logpoints to verify, I see these results in my console (including GET requests):

16:17:04.614 pointerdown { target: path.arrow, buttons: 1, clientX: 326, clientY: 438, layerX: 19, layerY: 13 }
16:17:04.717 pointerup { target: path.arrow, buttons: 0, clientX: 326, clientY: 438, layerX: 19, layerY: 13 }
16:17:04.720 click { target: path.arrow, buttons: 0, clientX: 326, clientY: 438, layerX: 19, layerY: 13 }
16:19:53.847 GET https://www.bazos.cz/img/7/194/128242194.jpg?t=1605810177 [HTTP/2 200 OK 510ms]
16:19:53.864 GET https://www.bazos.cz/img/8/194/128242194.jpg?t=1605810177 [HTTP/2 200 OK 623ms]
16:17:05.381 load { target: img.carousel-cell-image, isTrusted: true, srcElement: img.carousel-cell-image, currentTarget: img.carousel-cell-image, eventPhase: 2, bubbles: false, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }
16:17:05.481 load { target: img.carousel-cell-image, isTrusted: true, srcElement: img.carousel-cell-image, currentTarget: img.carousel-cell-image, eventPhase: 2, bubbles: false, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }

So two images are definitely being loaded. These are their initiator's stack traces:

First one:
n.prototype.load flickity2.js:formatted:1942
n flickity2.js:formatted:1905
s.lazyLoad/< flickity2.js:formatted:1932
s.lazyLoad flickity2.js:formatted:1931
e.emitEvent flickity2.js:formatted:99
p.dispatchEvent flickity2.js:formatted:787
p.select flickity2.js:formatted:804
p.next flickity2.js:formatted:827
s.prototype.onTap flickity2.js:formatted:1571
e.emitEvent flickity2.js:formatted:99
n.pointerUp flickity2.js:formatted:1492
n._pointerUp flickity2.js:formatted:1081
n.onpointerup flickity2.js:formatted:1073
i.handleEvent flickity2.js:formatted:263

Second one:
n.prototype.load flickity2.js:formatted:1942
n flickity2.js:formatted:1905
s.lazyLoad/< flickity2.js:formatted:1932
s.lazyLoad flickity2.js:formatted:1931
e.emitEvent flickity2.js:formatted:99
p.dispatchEvent flickity2.js:formatted:787
p.select flickity2.js:formatted:804
p.next flickity2.js:formatted:827
s.prototype.onTap flickity2.js:formatted:1571
s.prototype.onclick flickity2.js:formatted:1577
i.handleEvent flickity2.js:formatted:263

So logically, they are handling pointer and mouse events separately, and so are seeing two "taps". Indeed, if I disable pointer events, the problem goes away.

But why isn't it happening on Chrome, that's the question. I can't test on a live device, as their remote devices tab is currently broken. But Chrome also gets the same sequence of events in their RDM, according to logpoints:

pointerdown
pointerup
click
load

The network request is initiated by the code-path as Firefox does for the pointerup, but a similar click-event path isn't happening. It turns out that the devil is here:

    s.prototype.onclick = function(t) {
        var e = document.activeElement;
        e && e == this.element && this.onTap(t, t)
    }

In Chrome, document.activeElement happens to be the div.carousel.flickity-enabled.is-draggable element. In Firefox, it's the button itself, button.flickity-button flickity-prev-next-button next.

So that's why onTap is being called twice in Firefox, and why it's cycling twice on one tap.

The event in both cases is the mouse click event with event.srcElement being the svg graphic inside the button. But Chrome still sets the activeElement differently. I think we might need a reduced test-case here.

wisniewskit commented 3 years ago

Interesting. During pointerdown, they end up calling this function, which focuses on the carousel div they expect:

    p.focus = function() {
        var e = t.pageYOffset;
        this.element.focus({
            preventScroll: !0
        }),
        t.pageYOffset != e && t.scrollTo(t.pageXOffset, e)
    }

In Chrome, once that's set to the carousel, it doesn't change to the button during the rest of the event-cycle until the click haandler. But in Firefox it is changing to the button before the click handler is hit.

But in a reduced test-case, Chrome is doing the exact same thing - it changes the activeElement back to the button at some point.

I also don't see their JS doing any other focus() or blur() calls which could cause issues, so I'm still not sure what's going on here, and making a reduced test-case might not be easy.

wisniewskit commented 3 years ago

@denschub, @karlcow, any ideas here?

karlcow commented 3 years ago

@wisniewskit does it help https://bugzilla.mozilla.org/buglist.cgi?quicksearch=activeElement&list_id=15509442

wisniewskit commented 3 years ago

@karlcow no unfortunately I don't see anything obvious listed on Bugzilla or Chromium's issue tracker which seems to apply. But thanks.

denschub commented 3 years ago

The library in use here is https://github.com/metafizzy/flickity, and it appears to still be maintained. Unfortunately, simply injecting the current, unminified version of that library into the site for easier diagnosis isn't working. Also, the official library demos appear to be working just fine, so they're either using an outdated version, or it's related to something bazos.cz did on their side, possibly even modifications to the library itself.

I'm trying to build a local testcase with he code from bazos.cz site but a un-minified flickity library. Maybe having access to some non-obfuscated sources leads somewhere interesting. I'm also hoping I can figure out which version of the library bazos.cz is using, so I can test that specific version in a lab environment and see if that breaks.

I'll spend a bit more time on that, but if that isn't productive, we should probably just reach out.

karlcow commented 3 years ago

So I wanted to move to outreach so we can do things in parallel

fwiw… I just tested this morning with 90.0a1 (2021-05-05) (64-bit) and It does not reproduce… on desktop. 😱 And if you activate RDM with a mobile UA and touch on, it reproduces. BUT if you remove touch events with the same mobile layout, it doesn't reproduce anymore.

And this seems close enough https://github.com/metafizzy/flickity/issues/839

webcompat-bot commented 3 years ago

Generate outreach template

karlcow commented 3 years ago

Their email address is podpora@bazos.cz

karlcow commented 3 years ago

As for flickity it might be interesting to discover why https://github.com/search?l=JavaScript&q=flickity&type=Code

karlcow commented 3 years ago

Also https://github.com/metafizzy/flickity/issues/1076

denschub commented 3 years ago

With a bit of local MitM'ing, I could convince the site into accepting my local version of flickity. The issue reproduces with v2.1.2, and does not reproduce with v2.2.0. So something between those two versions changed and actually fixed it. The site appears to be working fine with the latest v2.2.2, so for outreach, we can absolutely suggest them to upgrade their version, which would fix the error (and a bunch of others, probably).

I still want to understand the cause here, though. Looking at the diff between the two mentioned versions, there are multiple commits that come to mind. Luckily, it's only 16 commits, so bisecting that is possible.

The commit that ends up fixing the issue for Firefox is 5da41ea. However, knowing that isn't too helpful: Here, the Flickity team replaced the tap-listener library with something else. However, in the issue tracking the removal, https://github.com/metafizzy/flickity/issues/764, the team says:

tap-listener was designed to resolve the 300ms delay, like fastclick. Since that's no longer an issue, tap-listener can be removed and click event can be used instead.

And sure enough, the library is doing more or less the same as FastClick: it emits an event immediately after pointerUp, circumventing a delay that is no longer there.

This is pretty much the FastClick issue, but without it being FastClick. As such, we know all about the internal causes, and we know that there is nothing we can really do about it besides "get rid of it". We should reach out to the site and see if they're willing to update the library. If not, we could use our shimming capabilities to update the library for them, but I'd rather not do that if we can avoid it. :)

denschub commented 3 years ago

I sent an email to the address Karl discovered, so moving this to sitewait. However, I'll also add the sitepatch-needed action, just to keep track of this - but as I said, we should probably give them a little bit of time to respond. :)

denschub commented 2 years ago

So, the only way we could sitepatch this is by shipping a new/fixed version of Flickity. As I mentioned in https://github.com/webcompat/web-bugs/issues/40939#issuecomment-834373694, this would work. While checking the feasibility of this, I was confused that that libraries repo has no LICENSE file, and on their website, I found this: https://flickity.metafizzy.co/license.html - and this causes an issue:

Shipping the library as part of the GPLv3 would require us to add the license to about:license. This would require legal review and lots of work - and it's not going to happen for this relatively "unimportant" site (I do realize that for users of the site, it might feel super important. My statement isn't meant as an attack - it's just that the amount of work we'd have to go through is not justified here.)
Shipping the library as part of their commercial license would require us to, well, buy a commercial license for the scale of Firefox. This is also not going to happen.

Partially rewriting the library like we did in other cases is possible, but we'd have to rewrite so much code that I'm not too sure how the licensing situation looks - so we'd be back at a legal review.

As much as I don't like doing so - I'll remove the needssitepatch label here.

webcompat / web-bugs

auto.bazos.cz - Slideshow skips frames #40939