UI: Fix Loading of Optimal Image on Invisible

[kergomard]

Hi @thibsy

This is a possible fix for: https://mantis.ilias.de/view.php?id=47613 .

I did not add any tests, because I felt that the pain-gain ration was not really favorable (we would have to mock the Observer and we would probably have to mock the getVisiblity()-function and then see if the observer gets enabled) ...but maybe you have a good idea.

Best,
@kergomard

[kergomard]

Hi @thibsy

As the tests failed anyway (sorry, my brain yesterday seems to already have been a little mushy) I fixed the test and added an additional one that checks that the observer is initialized.

Best,
@kergomard

[corro]

LGTM, this solves the issue according to my testing. Thank you very much @kergomard!

[thibsy]

Hi @kergomard,

Thx a lot for looking into this!

This turned out to be a very interesting issue =). I think I finally understand the cause: loadHighResolutionSource() is called at a time, where the HTMLImageElement.width is wrong because the element is hidden. This leads to an incorrect resolution being determined, which appears to do nothing if its the same as the initially provided one.

So your proposal to solve this using an IntersectionObserver only makes sense. However, after reading the documentation carefully, I found that your solution most likely doesn't work like you expect it would. Let me explain:

When IntersectionObserver.observe() is used, the registered callback is invoked immediately, regardless of any observer options. In your case this leads to a potential infinite loop, which only ever stops if the element does in fact becomes visible, which then results in its last call using the appropriate width. That's why it works, but at great cost =).

Therefore, please implement the following changes:

• Observer options: this object can be omitted, because root and rootMargin are already defaults, and threshold should be 0, which is also default. The reason is because we want to fire the callback at the earliest moment the element enters the viewport, not just after 10%. The actual check should be performed on isIntersecting instead (see below).
• Observer instances: we should embrace the observer pattern and only use one instance to observe all hidden images. This way our callback can interact with each IntersectionObserverEntry like its supposed to. The properties to work with here are isIntersecting and target.
• Single responsibility: we should leave loadHighResolutionSource() as is, because it does exactly what it says. We should instead not let images call loadHighResolutionSource() directly, by wrapping it behind a createImage() (or more concrete) initialisation function. This function should then take care of when to call loadHighResolutionSource, using the observer if need be.
• Unit tests: since the behaviour of IntersectionObserver.observe() is not so obvious, I think it would make sense to ensure that the registered callback handles isIntersecting correctly.

I hope this all makes sense.

Kind regards,
@thibsy (as UI coordinator)