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.

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

Flicker free refresh #4423

Merged
merged 9 commits into from
Oct 5, 2024
Merged

Flicker free refresh #4423

merged 9 commits into from
Oct 5, 2024

Conversation

James-Yu
Copy link
Owner

@James-Yu James-Yu commented Oct 3, 2024

This is a re-implementation of #4295 , following the discussion from #4415 (comment), and more importantly, migrating (reads: copy-and-paste) codes from https://github.com/tamuratak/latex-toybox/

In our previous implementation, we inject into PDF.js viewer refreshing logic to prevent flickering during PDF refresh. It caused a few issues, for example, spreading mode bounding box computation as in #4415 .

@tamuratak proposed a very elegant and non-intrusive solution for the problem in his maintaining branch by first creating images of visible PDF pages and masking them on top of canvas, then removing them once all pages are rendered. We "shamelessly" copy the idea and code.

@quoc-ho may you take a look and have a test on this branch and see if there are issues to fix? Many thanks.

@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 3, 2024

Wow. So fast! Thanks!!!

There's one problem though: when trimming is on, the mask has the wrong dimension.

Screen.Recording.2024-10-03.at.8.20.10.PM.mov

@James-Yu James-Yu force-pushed the flicker-free-refresh branch from d6a89ee to c1e1969 Compare October 3, 2024 13:33
@James-Yu
Copy link
Owner Author

James-Yu commented Oct 3, 2024

Yeah I forgot that we are using CSS-based trimming. This latest commit should fix the problem 🤣

@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 3, 2024

Thanks! That fixes the previous problem. But on some pages, the mask is off by a couple of pixels. The problem seems to appear (mostly) on every other pages. See the attached video.

Screen.Recording.2024-10-03.at.9.47.01.PM.mov

@James-Yu
Copy link
Owner Author

James-Yu commented Oct 3, 2024

Done!

It turns out that the offset values can only be integer values, but the center position of dom can be sub-pixel. In the latest commit, we use getBoundingClientRect() to obtain the floating values of the position and size of dom.

@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 4, 2024

Unfortunately, when we scroll all the way to the top, then the position of DOM is calculated correctly. But when we are in the middle of a page, then it's wrong. See video below.

Screen.Recording.2024-10-04.at.11.59.51.AM.mov

@James-Yu
Copy link
Owner Author

James-Yu commented Oct 4, 2024

This ... should work? Checked a few use cases.

I noticed one issue: when spreading is on, auto-width seems to be a bit wider than should be. It's not related to this PR, yet I will work on it later.

@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 4, 2024

Very awesome!

One last thing (hopefully): if you scroll all the way down and then refresh pdf, the position of the mask is wrong, resulting in the view being shifted briefly. This doesn't happen when the scroll position is not completely bottom.

Screen.Recording.2024-10-04.at.9.43.05.PM.mov

@James-Yu
Copy link
Owner Author

James-Yu commented Oct 4, 2024

Nice catch! This should be caused by pdfjs randomly destroying the last page and add it back with a small period when the page bottom margin of 10px is not added. The latest commit adds an invisible anchor div to cover the margin space.

Indeed, there is a better but behavior-changing solution: remove the 10px bottom margin of the last page, so that there is no margin in the top and at the bottom. If so, there's no need to use this anchor workaround. However, I have not settled the thought on the margin change.

@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 5, 2024

The experience is so smooth! Best pdf viewing experience ever!

I noticed one issue: when spread mode is on and the tab being refreshed is not visible, synctex fails after a couple of refreshes. This does not happen when spread mode isn't on. The error (from the webview developer tools) is

Uncaught TypeError: Cannot read properties of undefined (reading 'getPagePoint')
    at callSynctex (synctex.js:32:65)
    at pageDom.onclick (synctex.js:46:21)

I checked the DOM but the mask seems to be correctly removed already. So not sure what the cause is.

@James-Yu
Copy link
Owner Author

James-Yu commented Oct 5, 2024

That's strange. Anyways this commit should prevent the error log.

I think the new implementation should be ready. Good to push to users.

@James-Yu James-Yu merged commit c6cbb8d into master Oct 5, 2024
9 of 10 checks passed
@James-Yu James-Yu deleted the flicker-free-refresh branch October 5, 2024 03:56
@quoc-ho
Copy link
Contributor

quoc-ho commented Oct 5, 2024

I want to clarify that reverse synctex (pdf -> tex) was the problem. It's still a problem, at least for me. I don't use spread mode so I'm not affected by it. But just so you know. :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2024
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