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.
Created attachment 145334 [details] 140 x 50 px image for tests
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.
Created attachment 145472 [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
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
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>
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?
Looking here, noting that there was no feedback on last comment.
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 ?
Created attachment 148658 [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
(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.
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
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>
Will resume qa shortly
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.
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 ;)
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.
> 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).
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
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.
Created attachment 149249 [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> https://bugs.koha-community.org/show_bug.cgi?id=32624
Created attachment 149250 [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>
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>
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.
Revisiting
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.
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.
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>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Backported to 22.05.x for upcoming 22.05.13
applied to 21.11 for 21.11.21