smoothscroll icon indicating copy to clipboard operation
smoothscroll copied to clipboard

Published 20 hours ago • verified publisher icon gblazex

Bugfixes (hopefully :))

Open koczkatamas opened this issue 10 years ago • 7 comments

I had two bugs:

  • sometimes somehow rootScrollHeight and el.scrollHeight became 0, in which case the body was scrolled instead of the overflowing ancestor
  • the other bug involves the pending and queue mechanism: it only works if the scrolling elem stays the same. But that not always the case: for example the user scrolling an overflowing div then moves the cursor outside of the div and wants to scroll the body, but the div is scrolled (if it can, if it is already at the end then no scrolling happens)

I fixed them, hopefully :)

koczkatamas avatar Aug 24 '14 00:08 koczkatamas

Thanks for the PR. Can you provide test case for the first one?

gblazex avatar Aug 25 '14 07:08 gblazex

I created a simplified test case (originally the code was a part of a complex, currently non-public web application):

<!DOCTYPE html>
<html>
    <body style="position:fixed">
        <div style="overflow: auto; position: relative; width: 781px; height: 414px;">
            <div style="position: absolute; top: 0px; left: 0px; width: 1px; height: 30000px;"></div>
            <span>BADBADBADBADBADBADBADBADBAD</span>
        </div>
    </body>
</html>

The problem happens when you move the mouse over the BADBAD... text and try to scroll with the mouse wheel (sometimes you have to click on the text before too). With disabled SmoothScroll the scrolling never breaks.

Oh and maybe my description of the problem is inaccurate as in this example the body is fixed, it cannot scroll, so simply nothing happens (but it turns out by debugging SmoothScroll's code that it tries to scroll the body instead of the div).

koczkatamas avatar Aug 27 '14 19:08 koczkatamas

Thanks for taking the time and coming up with a simplified use case.

The problem is that it's not realistic. position:fixed on the body may be allowed, but logically doesn't make any sense to me.

Can you show me one where the body doesn't have position:fixed ?

(in addition to the simplified use case, you could also show me the website where you encountered the problem, if there was any)

gblazex avatar Aug 27 '14 21:08 gblazex

I don't know any other site with same problem.

I copied the example from the demo page of DockSpawn: http://www.dockspawn.com/demos/ide/dock_spawn_demo_ide.html

But then I will remove the position:fixed from my code, I don't know why it is set in the first place (but I am not too pro in HTML, so...).

koczkatamas avatar Aug 27 '14 21:08 koczkatamas

Yeah position:fixed basically says: stick to a certain point when the body scrolls. [1]

Doesn't really make sense to use it on the body element itself. (That's why it never came up during all these years)

I'll review your second suggestion in this PR as well.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/position

gblazex avatar Aug 27 '14 21:08 gblazex

The bug for the second pull request happened most frequently on StackOverflow where a code snippet was in an overflowed box.

It is a more radical change and I don't know the performance and other implications. So I'd propose you rewrite to make it more fitting for SmoothScroll (but meanwhile I could use it for myself without the annoying scroll stopping effect).

Btw probably I will create a third PR: if an overflowed div is scrolled to the bottom, no more scrolling happens, but the default operation of Chrome is that then its parent (or the body) is scrolled (so you don't have to move your mouse to continue the scrolling).

koczkatamas avatar Aug 27 '14 22:08 koczkatamas

Yup, I remember the "StackOverflow effect". SS does behave differently than the native scrolling now, you have a point there.

I'll think about it how to best implement this.

gblazex avatar Aug 27 '14 23:08 gblazex