Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Use scrollHeight instead of offsetHeight #14

Merged
merged 1 commit into from
Jan 5, 2014

Conversation

itsPG
Copy link
Contributor

@itsPG itsPG commented Dec 17, 2013

The original script seems not to work for my website.

After I traced the source code, I found that the offsetHeight may be replaced with scrollHeight and it works for me now.

@cferdinandi
Copy link
Owner

I'm glad this fixed the bug for you. This is the first time I've seen it reported though, and I'd like to better understand scrollHeight caused an issue for you—and why offsetHeight is therefore a better choice— before accepting this pull request.

@itsPG
Copy link
Contributor Author

itsPG commented Dec 17, 2013

In my opinion, offsetHeight means how much pixel you can see in your browser, while scrollHeight means how much pixel you can scroll. You may simplely check the change of offsetHeight by toggling the console in Chrome. However, I have no idea why sometime offsetHeight changes but sometimes not.

http://i.imgur.com/iZ5jGXx.png
http://i.imgur.com/uCWcZS6.png

The offsetHeight will change if I adjust the size of console.

@cferdinandi
Copy link
Owner

I'm going to hold off on merging this until I can get more clarity on why one approach is preferable over another. Thanks.

@itsPG
Copy link
Contributor Author

itsPG commented Dec 17, 2013

Thanks. Hope you can find out the answer for us.

@cferdinandi
Copy link
Owner

Another user has reported a similar bug, and your pull request seems to solve the problem. I'll be merging shortly. Thanks!

@cferdinandi cferdinandi reopened this Jan 5, 2014
cferdinandi pushed a commit that referenced this pull request Jan 5, 2014
Change `offsetHeight` to `scrollHeight` to fix fixed/absolute positioning issues.
@cferdinandi cferdinandi merged commit 0a522c4 into cferdinandi:master Jan 5, 2014
cferdinandi pushed a commit that referenced this pull request Jan 5, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants