Skip to content

Conversation

@emberfiend
Copy link
Contributor

📝 Summary

Hi all, first PR here. I'm trying to keep this small and self-contained. I'm new to both this codebase and Vue, please don't hesitate to give corrections or hints :)

This adds #4519 @provokateurin via an IntersectionObserver, which loads images only when they intersect the viewport.

@mejo- 's well-timed #7201 means we don't have to worry about the viewport jumping around at load time, causing unwanted loads.

🖼️ Screenshots

Tricky to screenshot!

🚧 TODO

My notes for follow-ups:

  • Find a way to do this with margin or even acceleration modelling so images start loading well before they hit the viewport.
  • Reusing one IntersectionObserver across multiple components would be faster but potentially less tidy.
  • As @juliusknorr said in Image loading moves text #4242, image metadata is the more complete solution to jumpiness in general. This PR makes things worse on slow connections in a way, because scrolling beyond images to look at text below them always means a new (interrupting) wait for the document to stop changing length, instead of one longer wait when the document first loads.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@juliusknorr juliusknorr added bug Something isn't working 3. to review labels May 9, 2025
@juliusknorr juliusknorr self-requested a review May 9, 2025 09:22
@codecov
Copy link

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Project coverage is 52.18%. Comparing base (9213d8f) to head (75c99a2).
Report is 63 commits behind head on main.

Files with missing lines Patch % Lines
src/nodes/ImageView.vue 0.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7209      +/-   ##
==========================================
- Coverage   52.25%   52.18%   -0.08%     
==========================================
  Files         479      479              
  Lines       41758    41767       +9     
  Branches     1024     1024              
==========================================
- Hits        21822    21795      -27     
- Misses      19834    19870      +36     
  Partials      102      102              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@emberfiend emberfiend force-pushed the feature/load-images-on-demand branch from 48533ab to 59b19f9 Compare May 9, 2025 09:50
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Welcome @emberfiend and thanks a lot for your contribution.

You are registering the IntersectionObserver in the mounted function. Is there any cleanup needed when say the editor is closed and all the nodes disappear?

I noticed one thing when trying this:
When I create a document with a lot of images at the end and no other content. The document is initially scrolled to the end where a LOT of spinning wheels are shown (each 16px high). Then all the corresponding images are loaded. As they now take more space and the browser stays aligned with the topmost one (i assume) it effectively scrolls up.
I don't know how to completely prevent this.

But it seems like the .image__loading css directive that sets it to '100px' height has no effect anymore. Would be great if you could fix this as it reduces the effect of the many small nodes that trigger many images to get loaded.

I'll approve right away as these are just two minor remarks.

@emberfiend
Copy link
Contributor Author

Thanks @max-nextcloud for the kind words and guidance :)

Added some cleanup in beforeUnmount.

Your second point is a rabbit-hole haha. So it seems a file with only images (like, not a single text node) behaves differently to one with a single line of text in it. An image-only file cannot be copied (well, it produces an empty copy) and exhibits the behaviour you describe: it's scrolled to the bottom of the document, whatever length it is, so whichever loading spinners are visible load, and scroll ends up aligned with the topmost of those images. I'm struggling to reproduce the "them being 16px high" thing though.

In contrast, adding one line of text causes the file to be scrolled to the top when opened, with much less janky behaviour following.

So (apart from the 16px thing) I think the real question is why image-only files behave differently. My React-y gut has some guesses but I need to learn more about the whole editor structure before I can be more useful I think.

@emberfiend
Copy link
Contributor Author

Tangentially, can anyone explain why DCO is unhappy again? I was pretty sure I did my last commit in the style it liked.

@juliusknorr
Copy link
Member

Thanks for the update. The DCO is the remaining one to fix before we can merge this.

https://github.com/nextcloud/text/runs/42043204677 shows that the email does not match:

Commit sha: ac766cf, Author: emberfiend, Committer: emberfiend; Expected "emberfiend [email protected]", but got "Andrew Backhouse [email protected]".

Maybe you can wrap this up and squash your commits into one. Good to get merged from my perspective otherwise as well 👍

@emberfiend emberfiend force-pushed the feature/load-images-on-demand branch from ac766cf to 75c99a2 Compare May 12, 2025 11:37
@emberfiend
Copy link
Contributor Author

@juliusknorr Thanks, I missed that it was logging errors. Squashed.

@juliusknorr juliusknorr enabled auto-merge May 12, 2025 11:59
@juliusknorr juliusknorr merged commit 225d625 into nextcloud:main May 12, 2025
64 of 66 checks passed
@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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.

3 participants