SlickGrid icon indicating copy to clipboard operation
SlickGrid copied to clipboard

Horizontal scrollbar scrolls in very small steps

Open Edminsson opened this issue 7 years ago • 13 comments

Since version 2.3.9 when clicking the scrollbar arrows in the horizontal scrollbar the grid scrolls in very small steps. This happens in Chrome and Firefox. You can compare version 2.3.8 and 2.3.12 in these plunks.

  • Version 2.3.8 https://run.plnkr.co/plunks/Re0PZ8Q5BO55bL1XtKCk/
  • Version 2.3.12 https://run.plnkr.co/plunks/WA7wgm8amsEEiPtvtJ24/

Update

  • Version 2.3.13 https://run.plnkr.co/plunks/sT1aJ5kqXsRJ5djiZaUn/

Edminsson avatar Feb 16 '18 13:02 Edminsson

Have you tried with latest version 2.3.13? It seems like there was some scrolling commits lately. If that still doesn't work, it might be related to this commit, I only searched through the latest commits, I don't have any clue if that is related or not.

ghiscoding avatar Feb 27 '18 04:02 ghiscoding

I have been working through 2.3.8 and 2.3.9. Looks like scrolling handling was introduced in 2.3.9, but I'm not quite sure what the problem is yet. It's not CSS, and I'm having trouble working through the different features in the scrolling commits to slick.grid.js, I keep breaking it!

6pac avatar Feb 27 '18 04:02 6pac

@6pac When you say scrolling handling, do you mean Slick Events? Could that be performance issues? and/or memory leak with some of these new events?

ghiscoding avatar Feb 27 '18 04:02 ghiscoding

    $headerScroller
        .on("scroll", handleHeaderScroll)

was introduced. also

function measureScrollbar() 

was modified significantly, can't imagine that would be the problem though.

6pac avatar Feb 27 '18 05:02 6pac

So, the culprit is handleHeaderScroll in this issue: https://github.com/6pac/SlickGrid/issues/136 Not sure even why it was implemented. I have a Mac with a touch pad; perhaps if I had a scroll wheel I could scroll left over the header?

6pac avatar Feb 27 '18 13:02 6pac

If I click the scroll wheel on my mouse then I see a circle with four arrows and I can scroll horizontally, but it only works when the cursor is over the content, not in the header. It's the same in 2.3.8 and 2.3.13. I don't know how to recreate the issue that #137 solves.

Edminsson avatar Feb 28 '18 17:02 Edminsson

I've made some jasmine scroll tests and run the same tests in version 2.3.8 and 2.3.13. All tests pass when I run them inte the older version 2.3.8, But when I run the tests in version 2.3.13 not only do some of them fail, I often get different results when I reload the page . https://run.plnkr.co/plunks/MbYpYkS12LL1secUo7CG/ https://run.plnkr.co/plunks/RZIbdZ1tVlUvdSiNrUuL/

Edminsson avatar Mar 05 '18 20:03 Edminsson

@Edminsson, that's great. I'm trying to diff 2.3.8 and 2.3.9 and add one feature at a time, testing each step. Can you allow me to edit the plunks rather than start from scratch? Or can I run the jasmine tests locally?
If so, can you add the test to the TESTS folder? There are some tests there, but I think they are all to do with vertical scrolling.

Or if you're keen to do this yourself, the process I use is:

  • download the 2.3.8 repo
  • download the 2.3.9 repo
  • use WinMerge to view the differences (lines can be copied left or right one line at a time to mod and test)
  • would need to update the plunk for each mod

It's pretty straightforward, if you have WinMerge. It's a great tool and well worth installing

6pac avatar Mar 07 '18 02:03 6pac

This is what I think is the source of the problem and what I wanted to prove with my tests. When clicking the scrollbar the browser fires not one but multiple scroll events which are handled asynchronously by scroll event handlers.

HandleScroll is called since it's the event handler of the scroll event on the viewport element. Handlescroll sets the scrollLeft property of the headerScroller element, which triggers a new scroll event since the headerScroller element is bound to the scroll event aswell, but the event handler of this event is not handled immediately, but asynchronously. When handleScroll is done there are several events waiting to be handled. The events triggered by the browser and the event triggered by handleScroll. I believe that sometimes $viewport[0].scrollLeft is changed by the browser (this change is the natural incrementation of scrollLeft by the browser) between the time handleScroll changes $headerScroller[0].scrollLeft and the time handleHeaderRowScroll is called. When this happens handleElementScroll detects a difference between $viewport[0].scrollLeft and $headerScroller[0].scrollLeft and therefore sets $viewport[0].scrollLeft which triggers another scroll event. When handleElementScroll sets $headerScroller[0].scrollLeft it disturbes the natural incrementation of scrollLeft.

Edminsson avatar Mar 07 '18 19:03 Edminsson

You can of course have access to the tests if you want but they are not specially well constructed. I only made them to try to prove the problem I've described. I basically run the same simple test multiple times.

Edminsson avatar Mar 07 '18 19:03 Edminsson

I've made some modifications on the tests and forked them to a public plunk. https://plnkr.co/edit/VuPta6?p=info https://plnkr.co/edit/6WXzDc?p=info

Edminsson avatar Mar 08 '18 20:03 Edminsson

Thanks, that's great! I found jsFiddle wasn't quite up to fiddles with the complexity of SlickGrid, but it looks like Plunkr might be the new standard!

Before I go any further, I note that the timeout in FireTwoScrollEvents() is zero rather then 200 like the single scroll test. When I run the tests with a 200 timeout there, I don't seem to be getting any failures. Can you check that?

6pac avatar Mar 10 '18 00:03 6pac

That's correct. It seems that a delay of 200 ms is enough to make the FireTwoScrollEvents tests pass. My guess is in that case handleScroll and then handleHeaderRowScroll have time to run before then next event is fired. In the real case scenario there are problably no delays. You can ran the single scroll tests with 0 timeout and they will still pass. So it's probably a good idea to set both tests to 0 timeout.

Edminsson avatar Mar 10 '18 12:03 Edminsson

I think this was addressed a long time ago in https://github.com/6pac/SlickGrid/releases/tag/2.3.16 so it should be safe to close

ghiscoding avatar May 17 '23 14:05 ghiscoding