Bug 32642 - Loading spinner always visible when cover image is short (OPAC)
Summary: Loading spinner always visible when cover image is short (OPAC)
Status: RESOLVED FIXED
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-12-28 20:44 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06,22.05.13,21.11.21


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
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.55 KB, patch)
2023-04-06 23:39 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.65 KB, patch)
2023-04-07 02:10 UTC, David Nind
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.65 KB, patch)
2023-04-07 02:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.60 KB, patch)
2023-04-07 02:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 32642: Loading spinner always visible when cover image is short (OPAC) (2.96 KB, patch)
2023-04-21 10:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32642: (QA follow-up) Activate spinner early (3.87 KB, patch)
2023-04-21 10:39 UTC, Marcel de Rooy
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.
Comment 17 Hammat wele 2023-04-06 23:36:41 UTC
> 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.)

Hi Marcel,

For the images with a src attribut already set in the html code ( images like localcover, CmazonCoverImages ...), they will have time to locally complete the downloading because of the `$(window).load` it will wait for all images to be load. But for the others they may not have enough time.

I think it's better to just remove spinner by removing the "background-image", the class 'cover-slides' is used in opac.css file .cover-slides a.nav-active:visited {color: #85CA11;}, ( specially when we have severals images for the same notice).
Comment 18 Hammat wele 2023-04-06 23:39:31 UTC
Created attachment 149247 [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 19 Hammat wele 2023-04-06 23:42:28 UTC
I've added a load event to check if images are completed and then the spinner is removed. this way the spinner is always removed once the image is complete.
Comment 20 David Nind 2023-04-07 02:10:19 UTC Comment hidden (obsolete)
Comment 21 David Nind 2023-04-07 02:12:14 UTC Comment hidden (obsolete)
Comment 22 David Nind 2023-04-07 02:13:35 UTC
Created attachment 149251 [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: David Nind <david@davidnind.com>
Comment 23 David Nind 2023-04-07 02:16:47 UTC
Apologies for the noise!

I initially signed this bug off using the wrong bug number, and it added a link to Bugzilla at the end of the patch with the incorrect bug number.

Ended up amending the commit message to remove that line, then reattached to the bug.
Comment 24 Marcel de Rooy 2023-04-21 07:02:20 UTC
Revisiting
Comment 25 Marcel de Rooy 2023-04-21 08:30:58 UTC
The more I look at this cover images code in javascript, the more I wonder about it ;) I think the whole code (not talking about this adjustment) needs more attention and has been scattered over various js files for specific cover image providers etc.
We are starting wait_for_images in window.load. It all looks quite suboptimal.

What this adjustment is doing now, is adding the spinner after everything has been loaded already. And it therefore immediately removes it again.
Comment 26 Marcel de Rooy 2023-04-21 10:39:20 UTC
Created attachment 150022 [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: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
This needs a follow-up; the code in this template activates the spinner
actually when the job has been done already and did not remove it.
And we did not see that but only with a short image like above.
The construction with .one, .each is not needed given the execution
context.
Comment 27 Marcel de Rooy 2023-04-21 10:39:23 UTC
Created attachment 150023 [details] [review]
Bug 32642: (QA follow-up) Activate spinner early

See comment on former patch too. We need to activate the spinner
early and we can simplify the code to remove it.

Note: unfortunately this is kind of a workaround, since using
load would be nicer but currently hard to implement since the cover
image code is scattered/widespread.

This code still checks the complete attribute, we could argue that
it should just remove the spinner. Note too that the spinner belongs
to a div where multiple img may be part of. If we do not remove the
spinner now, something went wrong at load time, but if you can
actually see the spinner is another thing.

Test plan:
See former patch.
Check few cover images on detail page, enable various providers.
Bonus: I tested with a sleep statement in opac-image.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Tomás Cohen Arazi 2023-04-21 13:37:21 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 29 Jacob O'Mara 2023-05-10 12:53:47 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 30 Lucas Gass 2023-05-15 21:09:09 UTC
Backported to 22.05.x for upcoming 22.05.13
Comment 31 Arthur Suzuki 2023-05-23 14:26:00 UTC
applied to 21.11 for 21.11.21