Skip to content

Fix scrolling of markdown preview. Close #65504 #111094

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
Merged
mjbvz merged 5 commits into from
Jan 15, 2021
Merged

Fix scrolling of markdown preview. Close #65504 #111094

mjbvz merged 5 commits into from
Jan 15, 2021

Conversation

@tamuratak
Copy link
Contributor

@tamuratak tamuratak commented Nov 21, 2020 •
edited
Loading

Fix scrolling of markdown preview. This PR fixes #65504.

  • Ensure that window.scrollTo is called after images loaded.
  • Ensure that window.scrollTo called from onceDocumentLoaded and onUpdateView does not lead to messaging.postMessage('revealLine', { line }).

I think that it is not guaranteed that scrollDisabled = true and scrollDisabled = false are alternately executed in order when users type very quickly. So, we have to use the counter scrollDisabledCount instead of the boolean scrollDisabled.

I have tested this PR with the document https://github.com/jlelong/LaTeX-Workshop-wiki/blob/master/Hover.md.

Before:

Nov-21-2020 21-57-02

After:
Nov-22-2020 07-10-51

@tamuratak
Copy link
Contributor Author

tamuratak commented Nov 23, 2020 •
edited
Loading

With the approach of this PR, we have to make scrollTo larger than 1 to ensure that the scroll event occurs:

vscode/extensions/markdown-language-features/preview-src/scroll-sync.ts

Line 146 in 457acf8

scrollTo = Math.abs(scrollTo) < 1 ? Math.sign(scrollTo) : scrollTo;

Otherwise, scrollDisabledCount continues to increase.

Another approach is setting scrollDisabled = false always after a while without setting scrollDisabled = false on the scroll event:

vscode/extensions/markdown-language-features/preview-src/index.ts

Lines 59 to 68 in 5a5afac

function disableScrollWhileDoing(cb: () => void) {
scrollDisabled = true;
try {
cb();
} finally {
setTimeout(() => {
scrollDisabled = false;
}, 100);
}
}

@mjbvz mjbvz added this to the December/January 2021 milestone Dec 1, 2020
mjbvz
mjbvz reviewed Jan 8, 2021
View reviewed changes
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look. Just a quick question with the implementation. Once that looks good, we can test this out in insiders

extensions/markdown-language-features/preview-src/index.ts
return Promise.resolve();
} else {
return new Promise<void>((resolve) => {
e.addEventListener('load', () => resolve());
Copy link
Collaborator

@mjbvz mjbvz Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking all images, should this use the window.load event? That should only be fired when all resources are loaded

If we continue checking for each image, should there also be a check for the error event in case the image fails to load?

Copy link
Contributor Author

@tamuratak tamuratak Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using window.addEventListener('load', cb); does not work. When webview.html = content executed on the extension host, the load event does not occur on the webview.

I have edited the PR to treat cases when the error event occurs.

@jamesr2k
Copy link

jamesr2k commented Jan 10, 2021

I'm a supreme newbie trying to learn Unity when this problem started happening for no apparent reason in Visual Studio Code. Could you explain the instructions: "Ensure that window.scrollTo is called after images loaded." ?

ohgodohno.mp4

@mjbvz mjbvz merged commit a34e751 into microsoft:master Jan 15, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Jan 15, 2021

Thanks! This will be in the next insiders build. Let's see if it causes any problems in insiders. If everything looks good, it will go out with Jan VS Code stable release

@tamuratak

This comment has been minimized.

Sign in to view
lszomoru pushed a commit that referenced this pull request Jan 15, 2021
@tamuratak @lszomoru
Fix scrolling of markdown preview. Close #65504 (#111094)
aeba2de 
* Fix scrolling of markdown preview.

* Use scrollDisabledCount.

* Stop initializing scrollDisabledCount.

* Make scrollTo enough large to occur scroll events.

* Should resolve when the error event occurs.
@mjbvz mjbvz mentioned this pull request Feb 2, 2021
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mjbvz mjbvz mjbvz left review comments

Assignees

@mjbvz mjbvz

Labels

None yet

Projects

None yet

Milestone

January 2021

Development

Successfully merging this pull request may close these issues.

Code scrolls when typing with markdown preview open

3 participants

@tamuratak @jamesr2k @mjbvz