-
Notifications
You must be signed in to change notification settings - Fork 536
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
Flicker free refresh #4423
Conversation
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 |
d6a89ee
to
c1e1969
Compare
Yeah I forgot that we are using CSS-based trimming. This latest commit should fix the problem 🤣 |
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 |
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 |
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 |
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. |
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 |
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. |
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
I checked the DOM but the mask seems to be correctly removed already. So not sure what the cause is. |
That's strange. Anyways this commit should prevent the error log. I think the new implementation should be ready. Good to push to users. |
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. :) |
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.