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

When dragging the scroll bar, reported direction ist always "right" and stays "right" afterwards #362

Open
odysseuscm opened this issue Oct 30, 2021 · 8 comments
Assignees

Comments

@odysseuscm
Copy link

odysseuscm commented Oct 30, 2021

Hello 👋

Describe the bug
I use the on "scroll" event to determine the scroll direction. When scrolling with the mouse wheel the direction is correctly reported as "up" and "down". When scrolling by dragging the scroll bar, the direction is always reported as "right" and from now on stays "right" whichever way you scroll. Even if you scroll by mouse wheel again.

To Reproduce
I built a test case here: https://kunden.cmcm.info/ls-direction-bug/index.html
Once you drag the scroll bar, the reported direction is always "right".

Expected behavior
Scroll direction should be reported correctly whatever way you scroll

Desktop (please complete the following information):
confirmed for Chrome 95 and Firefox 93

@odysseuscm
Copy link
Author

odysseuscm commented Oct 30, 2021

It might be fixed by changing line 2400:

FROM:

if (this.instance.delta.x > this.instance.scroll.x) {
   if (this.instance.direction !== 'right') {
      this.instance.direction = 'right';
   }
}

TO:

else if (this.instance.delta.x > this.instance.scroll.x) {
   if (this.instance.direction !== 'right') {
      this.instance.direction = 'right';
   }
}

But I'm not sure if that causes problems with detection of horizontal scroll direction.

odysseuscm added a commit to odysseuscm/locomotive-scroll that referenced this issue Oct 30, 2021
@odysseuscm
Copy link
Author

odysseuscm commented Oct 30, 2021

I made another test case with horizontal scrolling and it worked fine with my fix. So I'm proposing this change.
Here are the tests with the small fix applied:
https://kunden.cmcm.info/ls-direction-bug/fixed-vertical.html
https://kunden.cmcm.info/ls-direction-bug/fixed-horizontal.html

Another way would be to limit the check of delta.y / scroll.y respectively delta.x / scroll.x depending on what scroll direction is set upon instantiation.

Of course the real question is why dragging the scroll bar reports delta.x / scroll.x difference at all when scrolling is set to vertical. But I'd have to first understand the library better, to find the cause.

@odysseuscm
Copy link
Author

odysseuscm commented Oct 30, 2021

I took a deeper look. The real problem is here:

if (this.isDraggingScrollbar) {
  requestAnimationFrame(function () {
    var x = (e.clientX - _this5.scrollbarBCR.left) * 100 / _this5.scrollbarWidth * _this5.instance.limit.x / 100;
    var y = (e.clientY - _this5.scrollbarBCR.top) * 100 / _this5.scrollbarHeight * _this5.instance.limit.y / 100;

    if (y > 0 && y < _this5.instance.limit.y) {
      _this5.instance.delta.y = y;
    }

    if (x > 0 && x < _this5.instance.limit.x) {
      _this5.instance.delta.x = x;
    }
  });

x is always smaller than _this5.instance.limit.x when vertical scrolling is used.

Everywhere else horizontal values are only processed when this.direction == 'horizontal', so I guess the correct fix would look like this - which might even be the tiniest bit more performant:

if (this.isDraggingScrollbar) {
    requestAnimationFrame(function () {
    if (_this5.direction == 'horizontal')
    {
      var x = (e.clientX - _this5.scrollbarBCR.left) * 100 / _this5.scrollbarWidth * _this5.instance.limit.x / 100;
      if (x > 0 && x < _this5.instance.limit.x) {
        _this5.instance.delta.x = x;
      }
    }
    else
    {
      var y = (e.clientY - _this5.scrollbarBCR.top) * 100 / _this5.scrollbarHeight * _this5.instance.limit.y / 100;
      if (y > 0 && y < _this5.instance.limit.y) {
        _this5.instance.delta.y = y;
      }
    }
});

odysseuscm added a commit to odysseuscm/locomotive-scroll that referenced this issue Oct 30, 2021
Before, when used in vertical scrolling, x was always smaller than this.instance.limit.x resulting in the problem described here: locomotivemtl#362
From a performance point of view it is not necessary to do both calculations, as only one scroll direction can apply. Most other function use the same distinction between this.direction === 'horizontal' and 'vertical',
@Jerek0 Jerek0 self-assigned this Apr 28, 2022
@xavierfoucrier
Copy link
Contributor

Hello @Jerek0,

Just discovered this bug, and can confirm it's always the case.
Thanks for fixing this as soon as possible 😉

@odysseuscm
Copy link
Author

I posted a patch 1 year ago. #364
I don't know why it was never used - or at least commented on ...

@xavierfoucrier
Copy link
Contributor

@odysseuscm I will have a look 😉

@xavierfoucrier
Copy link
Contributor

@odysseuscm @Jerek0 I can confirm, the patch fix the issue 🎉 , would be nice to merge!

@RafaelKr
Copy link

@Jerek0 would be nice if the fix could be merged and published.

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

4 participants