Bug 32642 - Loading spinner always visible when cover image is short (OPAC)
Summary: Loading spinner always visible when cover image is short (OPAC)
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Hammat wele
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-16 15:45 UTC by Caroline Cyr La Rose
Modified: 2023-03-29 15:55 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
140 x 50 px image for tests (1.61 KB, image/png)
2023-01-16 15:46 UTC, Caroline Cyr La Rose
Details
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.43 KB, patch)
2023-01-19 13:04 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.34 KB, patch)
2023-01-19 13:37 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.42 KB, patch)
2023-02-01 18:05 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.43 KB, patch)
2023-03-24 12:36 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.37 KB, patch)
2023-03-24 12:55 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.44 KB, patch)
2023-03-27 12:27 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2023-01-16 15:45:55 UTC
Same as bug 28314, but for the OPAC

We noticed that if the cover image is somewhat short (i.e. the height is under 80-90 px), the loading spinner indicating that the image is currently loading still appears after the image is loaded.

The spinner should be hidden when the image is loaded, regardless of the image size.

To test:
1. Make sure the OPACLocalCoverImages system preference is enabled
   1.1. Go to Administration > Global system preferences
   1.2. Search for Coverimages
   1.3. Set OPACLocalCoverImages to 'Show'
   1.4. Click 'Save all Enhanced content preferences'
2. Add a short local cover image to a record (I added one which is just a grey rectangle with the size printed on it, to facilitate the test)
   2.1. Download the attached image
   2.2. Find a record without an image
   2.3. Click the 'Images' tab
   2.4. Click 'Upload'
   2.5. Click 'Drop files here or click to select a file' and choose the downloaded image
   2.6. Click 'Process images'
3. View the record in the OPAC 
   3.1. From the image upload page, click on the title of the record in the page heading to access the detailed record in the staff interface
   3.2. Click on 'OPAC view: Open in new window.'
   --> Note that the image is displayed normally, but the spinner is also displayed

In the OPAC, the problem is only on the detailed record page. The search results are OK.
Comment 1 Caroline Cyr La Rose 2023-01-16 15:46:13 UTC
Created attachment 145334 [details]
140 x 50 px image for tests
Comment 2 Olivier Hubert 2023-01-16 16:07:33 UTC
I see three options for solving this issue:

1. Remove the spinner by removing the background CSS declaration in both style sheets.
2. Add a minimum height to all images so that the spinner is always hidden. This is an issues with smaller images as they will appear "zoomed in" and lose in picture quality.
3. Add javascript code to hide / remove the spinner once an image is loaded.
Comment 3 Hammat wele 2023-01-19 13:04:41 UTC Comment hidden (obsolete)
Comment 4 Hammat wele 2023-01-19 13:37:22 UTC
Created attachment 145473 [details] [review]
Bug 32642: Loading spinner always visible when cover image is short (OPAC)

We noticed that if the cover image is somewhat short (i.e. the height is under 80-90 px), the loading spinner indicating that the image is currently loading still appears after the image is loaded.

this patch remove the spinner when the image is loaded,.

To test:
1. Make sure the OPACLocalCoverImages system preference is enabled
   1.1. Go to Administration > Global system preferences
   1.2. Search for Coverimages
   1.3. Set OPACLocalCoverImages to 'Show'
   1.4. Click 'Save all Enhanced content preferences'
2. Add a short local cover image to a record (I added one which is just a grey rectangle with the size printed on it, to facilitate the test)
   2.1. Download the attached image
   2.2. Find a record without an image
   2.3. Click the 'Images' tab
   2.4. Click 'Upload'
   2.5. Click 'Drop files here or click to select a file' and choose the downloaded image
   2.6. Click 'Process images'
3. View the record in the OPAC
   3.1. From the image upload page, click on the title of the record in the page heading to access the detailed record in the staff interface
   3.2. Click on 'OPAC view: Open in new window.'
   --> Note that the image is displayed normally, but the spinner is also displayed
4. Apply the patch
5. View the record in the OPAC
    --> Note that the spinner is no more displayed
Comment 5 Matt Blenkinsop 2023-02-01 18:05:06 UTC
Created attachment 145976 [details] [review]
Bug 32642: Loading spinner always visible when cover image is short (OPAC)

We noticed that if the cover image is somewhat short (i.e. the height is under 80-90 px), the loading spinner indicating that the image is currently loading still appears after the image is loaded.

this patch remove the spinner when the image is loaded,.

To test:
1. Make sure the OPACLocalCoverImages system preference is enabled
   1.1. Go to Administration > Global system preferences
   1.2. Search for Coverimages
   1.3. Set OPACLocalCoverImages to 'Show'
   1.4. Click 'Save all Enhanced content preferences'
2. Add a short local cover image to a record (I added one which is just a grey rectangle with the size printed on it, to facilitate the test)
   2.1. Download the attached image
   2.2. Find a record without an image
   2.3. Click the 'Images' tab
   2.4. Click 'Upload'
   2.5. Click 'Drop files here or click to select a file' and choose the downloaded image
   2.6. Click 'Process images'
3. View the record in the OPAC
   3.1. From the image upload page, click on the title of the record in the page heading to access the detailed record in the staff interface
   3.2. Click on 'OPAC view: Open in new window.'
   --> Note that the image is displayed normally, but the spinner is also displayed
4. Apply the patch
5. View the record in the OPAC
    --> Note that the spinner is no more displayed

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 6 Jonathan Druart 2023-02-23 11:15:18 UTC
It works, but that does not make the code more robust or understandable.

We could try and improve it by renaming the class, maybe. Owen what do you think?
Comment 7 Marcel de Rooy 2023-03-24 07:43:50 UTC
Looking here, noting that there was no feedback on last comment.
Comment 8 Marcel de Rooy 2023-03-24 08:04:22 UTC
The whole code around these images indeed requires some code digging to understand whats going on ;)

I have the impression that the new lines:
            if( $(".cover-slider").find(".cover-image:visible").length > 0 ){
                $(".cover-slider").removeClass("cover-slides");
            }
should be placed elsewhere. Now they are executed after the recursive calls or the callback. But this does not yet guarantee that all images are already loaded. And it also means that the new lines could be executed repeatedly which is unneeded.

In the callback verify_cover_images we activate the spinner actually:
                if( $(coverSlide).find(".cover-image:visible").length < 1 ){
                    $(coverSlide).remove();
                } else {
                    $(coverSlide).addClass("cover-slides");
                }

We should only remove the spinner when the image has been loaded locally. So shouldnt we check if the local loading has been completed?
So remove the spinner in image onload and check image.complete ?
Comment 9 Hammat wele 2023-03-24 12:36:35 UTC Comment hidden (obsolete)
Comment 10 Hammat wele 2023-03-24 12:53:21 UTC
(In reply to Marcel de Rooy from comment #8)
> The whole code around these images indeed requires some code digging to
> understand whats going on ;)
> 
> I have the impression that the new lines:
>             if( $(".cover-slider").find(".cover-image:visible").length > 0 ){
>                 $(".cover-slider").removeClass("cover-slides");
>             }
> should be placed elsewhere. Now they are executed after the recursive calls
> or the callback. But this does not yet guarantee that all images are already
> loaded. And it also means that the new lines could be executed repeatedly
> which is unneeded.
> 
> In the callback verify_cover_images we activate the spinner actually:
>                 if( $(coverSlide).find(".cover-image:visible").length < 1 ){
>                     $(coverSlide).remove();
>                 } else {
>                     $(coverSlide).addClass("cover-slides");
>                 }
> 
> We should only remove the spinner when the image has been loaded locally. So
> shouldnt we check if the local loading has been completed?
> So remove the spinner in image onload and check image.complete ?

I've changed the patch, now some control on the image is made before removing the spinner and it is in the verify_cover_images, it's much better this way.
Comment 11 Hammat wele 2023-03-24 12:55:27 UTC
Created attachment 148661 [details] [review]
Bug 32642: Loading spinner always visible when cover image is short (OPAC)

We noticed that if the cover image is somewhat short (i.e. the height is under 80-90 px), the loading spinner indicating that the image is currently loading still appears after the image is loaded.

this patch remove the spinner when the image is loaded,.

To test:
1. Make sure the OPACLocalCoverImages system preference is enabled
   1.1. Go to Administration > Global system preferences
   1.2. Search for Coverimages
   1.3. Set OPACLocalCoverImages to 'Show'
   1.4. Click 'Save all Enhanced content preferences'
2. Add a short local cover image to a record (I added one which is just a grey rectangle with the size printed on it, to facilitate the test)
   2.1. Download the attached image
   2.2. Find a record without an image
   2.3. Click the 'Images' tab
   2.4. Click 'Upload'
   2.5. Click 'Drop files here or click to select a file' and choose the downloaded image
   2.6. Click 'Process images'
3. View the record in the OPAC
   3.1. From the image upload page, click on the title of the record in the page heading to access the detailed record in the staff interface
   3.2. Click on 'OPAC view: Open in new window.'
   --> Note that the image is displayed normally, but the spinner is also displayed
4. Apply the patch
5. View the record in the OPAC
    --> Note that the spinner is no more displayed
Comment 12 ByWater Sandboxes 2023-03-27 12:27:03 UTC
Created attachment 148762 [details] [review]
Bug 32642: Loading spinner always visible when cover image is short (OPAC)

We noticed that if the cover image is somewhat short (i.e. the height is under 80-90 px), the loading spinner indicating that the image is currently loading still appears after the image is loaded.

this patch remove the spinner when the image is loaded,.

To test:
1. Make sure the OPACLocalCoverImages system preference is enabled
   1.1. Go to Administration > Global system preferences
   1.2. Search for Coverimages
   1.3. Set OPACLocalCoverImages to 'Show'
   1.4. Click 'Save all Enhanced content preferences'
2. Add a short local cover image to a record (I added one which is just a grey rectangle with the size printed on it, to facilitate the test)
   2.1. Download the attached image
   2.2. Find a record without an image
   2.3. Click the 'Images' tab
   2.4. Click 'Upload'
   2.5. Click 'Drop files here or click to select a file' and choose the downloaded image
   2.6. Click 'Process images'
3. View the record in the OPAC
   3.1. From the image upload page, click on the title of the record in the page heading to access the detailed record in the staff interface
   3.2. Click on 'OPAC view: Open in new window.'
   --> Note that the image is displayed normally, but the spinner is also displayed
4. Apply the patch
5. View the record in the OPAC
    --> Note that the spinner is no more displayed

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 13 Marcel de Rooy 2023-03-27 12:34:01 UTC
Will resume qa shortly
Comment 14 Marcel de Rooy 2023-03-29 13:54:16 UTC
Thx for this new attempt.
We now have:

$(coverSlide).addClass("cover-slides");
var img = $(coverSlide).find(".cover-image:visible").find("img")[0];
if( $(img).length > 0 && img.complete && img.naturalHeight > 0 ){
    $(coverSlide).css({"background-image":"none"});
}

What I wonder, is: will there always be enough time to locally complete the downloading of the image before you remove the spinner?
Now you add the spinner by adding the class. And directly you check if there is one visible image and you remove the spinner. (You could btw remove the class again instead of the css change.)

So should we remove the spinner class in image onload and check image.complete there?

See next comment.
Comment 15 Marcel de Rooy 2023-03-29 14:01:04 UTC
Please check javascript for various types of cover images:
    [% Asset.js("js/amazonimages.js") | $raw %]
    [% Asset.js("js/bakertaylorimages.js") | $raw %]
    [% Asset.js("js/coce.js") | $raw %]
    [% Asset.js("js/google-jackets.js") | $raw %]
    [% Asset.js("js/localcovers.js") | $raw %]
Hope I found them all ;)

For example, look at this in coce.js
img.onload = function() {
// image dimensions can't be known until image has loaded
if (img.height == 1 && img.width == 1) {
$(this).closest(".coce-coverimg").remove();
}
};

Or look here for localcovers:
            var img = $("<img />").attr('src',
                '/cgi-bin/koha/opac-image.pl?thumbnail=1&biblionumber=' + $(mydiv).attr("class"))
                .load(function () {
If you would add into this callback function after load completion, remove the spinner.

ETC,

Perhaps we can find a way to do this in one place for all types of covers? Sorry to make this seemingly simple change a bit more complicated ;)
Comment 16 Olivier Hubert 2023-03-29 15:55:47 UTC
Yes, I think we should check for covers outside of each service's script, and remove the spinner there. Each service can set a variable indicating if its internal processing has completed or not. When all services have completed their processing or an image is found (whichever comes first), then we remove the spinner.