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.

Sign up for GitHub

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

Jump to bottom

Bugfixes (hopefully :)) #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bugfixes (hopefully :)) #95

wants to merge 2 commits into from

Conversation

koczkatamas
Copy link

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 :)

@gblazex
Copy link
Owner

gblazex commented Aug 25, 2014

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

@koczkatamas
Copy link
Author

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).

@gblazex
Copy link
Owner

gblazex commented Aug 27, 2014

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)

@koczkatamas
Copy link
Author

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...).

@gblazex
Copy link
Owner

gblazex commented Aug 27, 2014

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

@koczkatamas
Copy link
Author

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).

@gblazex
Copy link
Owner

gblazex commented Aug 27, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koczkatamas @gblazex