Skip to content

fix: scrollToView not working properly if images exist #723

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

Closed
wants to merge 2 commits into from

Conversation

cheng-kang
Copy link
Contributor

Closes: #351


Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you are merging your commits to master branch.
  • Add some descriptions and refer relative issues for you PR.
  • DO NOT include files inside lib directory.

@dawit-elias
Copy link

Any updates on this @QingWei-Li? This fix will alleviate the confusion that people who currently read our docs experience.

@timaschew
Copy link
Member

Please run these commands to trigger netlify deployment for this Pull Request:

git commit --amend --no-edit
git push --force

@timaschew
Copy link
Member

Would be nice to have a test case for the problem which is solved by this PR

@dawit-elias
Copy link

@timaschew There's a repro case at the top of this issue: #351 (comment).

@rp-applied
Copy link

Anything preventing this from being merged? Would love this change as well! :)

@timaschew
Copy link
Member

@rp-applied a test case is still missing in this PR

@nickneilsonblack
Copy link

@timaschew - what's the best way to deliver a test case?

@nickneilsonblack
Copy link

@timaschew

Here's a test case. If you hit this URL directly you should be scrolled to the Test 3 section- but due to the images you see the Test 2 section.

https://nnblack1980.github.io/#/?id=test-3

Here's the repo: https://github.com/nnblack1980/nnblack1980.github.io

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Jan 30, 2020

Hey folks.

It's worth mentioning that while this PR may fix (I haven't tested it) scrollToView behavior with regards to image loading, it does not address the fact that the same issue exists for docsify plugins that modify the DOM (which can alter scroll positions by changing the content area dimensions). This bug was brought to my attention via jhildenbiddle/docsify-tabs#8. As I mentioned in a reply to that issue:

The short explanation is that docsify begins scrolling to the section specified in the URL before plugins (like docsify-tabs) or external content (image, video, AJAX content, etc.) have loaded. When plugins or external content change the content dimensions, they also change the scroll position of each section. Docsify fails to account for this, and as a result it scrolls to the original (incorrect) scroll position of the section specified in the URL.

An actual fix would involve using ResizeObserver (and MutationObserver as a fallback) to continuously monitor for content height changes, then scrolling (if the section has not already been scrolled to) or jumping (if the section was previously reached) to the section until a user-initiated scroll is detected. Monitoring for content height changes is the only way to detect all scroll position changes regardless of how or why they happen. Once a user-initiated scroll is detected, observers used to detect content changes and event listeners used to detect user-initiated scrolling can be removed so there's no left over cruft. Most importantly, this solution allows users to land on a page, have the correct section scrolled into view, and have that section remain in view even as content loads above the target section (which would normally cause the content in view to shift down).

The above solutions handles all cases (instead of just images) and it seems easier to implement that the image-only fix proposed here.

Thoughts?

@stale
Copy link

stale bot commented Mar 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 30, 2020
@stale stale bot closed this Apr 6, 2020
@anikethsaha anikethsaha deleted the fix/351 branch April 27, 2020 08:27
@kleinjm
Copy link

kleinjm commented Jul 30, 2021

Would it be possible to offer the option to disable the scrollTo behavior on page load? That way a custom scroll implementation can be written by those using the library

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants