feat: update to vue-virtual-scroller 1.1.2

[ThEditor]

Summary

• Updating to vue-virtual-scroller 1.1.2
• Implementing RecycleScroller
• Using DynamicScroller instead of RecycleScroller
• Fix issue with keeping nearby pages loaded (buffer properly?)
• [ ] Fit to width dynamically/provide a button (Out of scope)

preview.mp4

References

Closes #11663

Dependency diff:

• vue-virtual-scroller

Reviewer guidance

Open any pdf document inside kolibri

---

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

PR process

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[AlexVelezLl]

Hi @ThEditor. Thanks for your contribution! 🎉

Could you please retarget the PR and rebase to the release-v0.16.x branch. Here is a guide of how you can do it: https://kolibri-dev.readthedocs.io/en/develop/howtos/rebasing_a_pull_request.html.

Also, you can add your name and GH User on the AUTHORS.md file. 🎉

[ThEditor]

I think that should have done it. Thanks again @AlexVelezLl !

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b13.dev0_git.20.g559ff199.pex
Windows Installer (EXE)	kolibri-0.16.0b13.dev0+git.20.g559ff199-unsigned.exe
Debian Package	kolibri_0.16.0b13.dev0+git.20.g559ff199-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b13.dev0+git.20.g559ff199-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b13.dev0+git.20.g559ff199-0.1.0-debug.apk
TAR file	kolibri-0.16.0b13.dev0+git.20.g559ff199.tar.gz
WHL file	kolibri-0.16.0b13.dev0+git.20.g559ff199-py2.py3-none-any.whl

[AlexVelezLl]

Hi! Thank you @ThEditor! Code looks good, but I have encountered some problems:

• The pdf renderer now presents some performance issues since it now loads some more pages and it renders the same page several times.

  • The loading of several pages could be due to the change on the buffer prop, and probably it could be easily handled and discussed what can be the optimal value.
  • But what has the biggest impact on performance is that the list renders each page 3 times on the first render and on every new item that is shown while the user scrolls. This leads to jumps and flashes on the document.

  I measured performance of the page rendered task before and after the update, and after the change there was some situations where it shows some overflow on some pages although this pdf has the same height for all pages:

  Before:

  PdfRendererBefore.mp4

  After:

  PdfRendererAfter.mp4

  It seems it is because vue-scroller internally mount each page Component some additional times to get the item height. But we dont need to mount the pdf page Component to get its height, since its height is defined by the page height, the zoom and margin and it can be calculated without mounting the component.

  We can maybe look at the library to see if we can provide the height of an item without mounting the component multiple times, since this operation is expensive. Another solution that comes to my mind is to try to render like a "centinel node" that can provide to the DynamicScroller the height without doing the expensive tasks that the PdfPage Component does on mount.

• Also, it seems that now the RecycleList component does not expose the updateVisibleItems methods, and this method fails.

[AlexVelezLl]

Seems like this active and index attributes are not defined here. You need to define them in the template above:

<template #default="{ item, index, active }">
  ...
</template>

[AlexVelezLl]

I think we can live without this additional buffer prop for now. Pdf pages are usually full of content, and probably the user will keep viewing the current page for some time, so we dont need to load 2 pages below meanwhile.

[AlexVelezLl]

Item.height is undefined here, as the items of the is are the pdfPages array, and each item has this structure:
{ page: null, resolved: false, index: i, }

[ThEditor]

But we dont need to mount the pdf page Component to get its height, since its height is defined by the page height, the zoom and margin and it can be calculated without mounting the component

I'm not sure if I understand correctly. We have a pdf document of pages with unknown heights and that's why we try to utilize DynamicScroller. If the heights for each page are known but are variable, RecycleScroller exposes a variable height mode. Although it also states that that mode is performance heavy for a lot of items.

I also don't understand how we know the height of an item without actually rendering it? Does the page property expose a height field? Aren't zoom, margin set according to the firstPageHeight? I don't see how we would be getting the individual page height through that. @AlexVelezLl

[ThEditor]

So, I looked into this further. I tried searching for other libraries, tried implementing infinite scrolling on my own, but ended up back to vue-virtual-scroller. I have come up with a solution with doesn't have the mounting issue, however, there are some things to note:

• I've modified the original vue-virtual-scroller's source (basically modified RecycleScroller to accept size as a function instead of a value)
• I'm using variable-size mode of RecycleScroller which is stated as performance heavy in case of large lists.
• I'm pre-computing the heights of the pdf pages, however, I'm not sure if this has a performance impact because I'm not mounting/rendering the pages

I haven't made this into a commit yet because I'm not sure if this is even the right implementation (it works though). I would be interested in a meet, if possible, to discuss this issue, @AlexVelezLl . Thanks again! (also, merry christmas! :D)

[AlexVelezLl]

Hi! @ThEditor. Hope you had a merry christmas! and a happy new year 👐.

Ill let there some responses but probably will see deeply on new year:

---

I also don't understand how we know the height of an item without actually rendering it? Does the page property expose a height field?

The height of a page can be obtained with (here we calculate it):

pageHeight * this.scale + MARGIN

pageHeight can be obtained using the viewport object like this.

So, as you can see, we dont need the page to be rendered (page.render()) to obtain the height of the node, but just load the page which is a much cheaper operation.

---

I'm using variable-size mode of RecycleScroller which is stated as performance heavy in case of large lists.

For what I see, I think variable-size mode can do the work, we can see the performance implications.

You can push your changes and after the holidays I can review it, and have a meet if needed! :)

---

I'm pre-computing the heights of the pdf pages, however, I'm not sure if this has a performance impact because I'm not mounting/rendering the pages

We could probably fill all pages with the firstPage height and update after when each page loads.

[ThEditor]

Oh, that reply sounds exactly like what I implemented in the most recent commit(s). I'm not home right now, I'll push the code as soon as possible. @AlexVelezLl (hope you had an awesome christmas and new year too!)

[AlexVelezLl]

In general it is looking amazing! The vue-virtual-scroller update has bring much more smooth scrolling, and it now renders the items in order in the DOM, just let some thoughts on things that can impact the performance.

[AlexVelezLl]

Also, there are some falied gh checks: this and this. Could you fix them please? For formatting files you can run yarn run lint-frontend:format and for running tests you can run yarn run test

[ThEditor]

I wasn't sure if these changes are "acceptable", and hence, I didn't implement some changes/ideas I had. Now that I know these are okay, I can proceed. I'll get back to you on these changes as soon as possible.

[ThEditor]

I've made the requested changes, this should be ready for review again.

[AlexVelezLl]

Perfect! It is pretty exicitng 🎉🎉🎉 . We are almost done! Just one last very little thing from my side is that this node from the internals of the recycle list is taking some space and it shouldn't. We need to avoid it, and also fix a failed check in the code formatting, but everythnig else is looking fine!

Code looks good and some refactor you have done makes the code more readable! Thanks! This PR also solves a bug we had in the pages order in the DOM 👐. I will let the final approve to @radinamatic.

[ThEditor]

The space is due to ResizeObserver which is a part of vue-resize (which is used by vue-virtual-scroller). The space occurs because of about:blank in the code :

    const object = document.createElement('object')
    this._resizeObject = object
    object.setAttribute('aria-hidden', 'true')
    object.setAttribute('tabindex', -1)
    object.onload = this.addResizeHandlers
    object.type = 'text/html'
    if (isIE) {
      this.$el.appendChild(object)
    }
    object.data = 'about:blank'
    if (!isIE) {
      this.$el.appendChild(object)
    }

Making the change would require me to also pull in these files into the project. @AlexVelezLl

[pcenov]

Hi @ThEditor, thank you for your contribution. While testing I noticed the following issues:

1. Scrolling is no longer working when viewing the pdf-s in a mobile browser (tested both with actual device and Chrome's dev tools).

2024-01-08_18-14-27.mp4

2. Sometimes the completion modal is not being displayed after having completed reading the pdf (perhaps that's because of the extra padding at the bottom?).

2024-01-08_18-19-21.mp4

[ThEditor]

I am a bit busy for a few days, I'll get back to you as soon as possible.

[ThEditor]

Investigated the scrolling issue a bit, broken since ec1d3a3. Looking into it.

[ThEditor]

Does this look correct? or did I mess up? 😅 @AlexVelezLl

[AlexVelezLl]

Yes! That's exactly how it should look @ThEditor 😃. Just noted that a linting check failed hehe, Would be good If you can pass the lint again and update the pr ^^.

[ThEditor]

Yeah, I'll try fixing the linting issues. I can't seem to get pre-commit to install locally, hence why code with lint issues.

[ThEditor]

Please @ me when you guys have reviewed this, thanks!

[AlexVelezLl]

Hello @ThEditor! Thanks for your patience, we will do a final QA review but it won't be merged until 0.16 Kolibri version is released (which should happen in a few weeks). We will keep in touch 👐.

[pcenov]

Thanks @ThEditor - the scrolling issue in mobile devices is fixed now and I didn't observe any regression issues - good to go!

[radinamatic]

Manual QA passed, good to go! 💯 👏🏽

[AlexVelezLl]

Thanks for all your hard work here! @ThEditor 🎉 👐. I've given the code a final check and it looks good! It is ready to go now that 0.16.0 have been released! 🎉 🎉 🎉

[ThEditor]

Thanks a lot for merging! Glad to be able to contribute.