Bug 34724 - Remove use of onclick for opac-imageviewer.pl
Summary: Remove use of onclick for opac-imageviewer.pl
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 34723
Blocks: 34709
  Show dependency treegraph
 
Reported: 2023-09-06 06:33 UTC by David Cook
Modified: 2023-10-12 16:01 UTC (History)
2 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.11.00,23.05.04,22.11.11


Attachments
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl (3.10 KB, patch)
2023-09-06 06:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl (3.29 KB, patch)
2023-09-07 13:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34724: (QA follow-up): (3.24 KB, patch)
2023-09-07 13:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34724: (QA follow-up): (2.38 KB, patch)
2023-09-08 16:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34724: (QA follow-up): (2.43 KB, patch)
2023-09-11 04:54 UTC, David Cook
Details | Diff | Splinter Review
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl (3.34 KB, patch)
2023-09-17 14:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34724: (QA follow-up): (2.48 KB, patch)
2023-09-17 14:29 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-09-06 06:33:36 UTC
Like bug 34570, we want to get rid of "onclick".
Comment 1 David Cook 2023-09-06 06:42:29 UTC
Created attachment 155255 [details] [review]
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl

This patch moves the click handler for thumbnails for opac-imageviewer.pl
out of the inline HTML and out to an existing <script> element.

Test plan:
0. Apply the patch (including dependent patches)
1. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=localcover
2. Change both *LocalCoverImages sysprefs to "Show"
3. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AllowMultipleCovers
4. Change "AllowMultipleCovers" to "Allow"
5. Click "Images (0)" tab
6. Download or make two JPEG images
7. Upload the images
8. Go to http://localhost:8080/cgi-bin/koha/opac-imageviewer.pl?biblionumber=29
9. See one large image in the centre and two smaller thumbnails on the right side
10. Click on the thumbnail of the image that isn't displayed in the centre
11. Note that the image changes
12. Click the other thumbnail and note that the image changes back
Comment 2 Pedro Amorim 2023-09-07 13:11:36 UTC
Created attachment 155299 [details] [review]
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl

This patch moves the click handler for thumbnails for opac-imageviewer.pl
out of the inline HTML and out to an existing <script> element.

Test plan:
0. Apply the patch (including dependent patches)
1. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=localcover
2. Change both *LocalCoverImages sysprefs to "Show"
3. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AllowMultipleCovers
4. Change "AllowMultipleCovers" to "Allow"
5. Go to
http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29
6. Click "Images (0)" tab
7. Download or make two JPEG images
8. Upload the images
9. Go to http://localhost:8080/cgi-bin/koha/opac-imageviewer.pl?biblionumber=29
10. See one large image in the centre and two smaller thumbnails on the right side
11. Click on the thumbnail of the image that isn't displayed in the centre
12. Note that the image changes
13. Click the other thumbnail and note that the image changes back

PA amended patch: Added missing test plan step

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 3 Pedro Amorim 2023-09-07 13:11:38 UTC
Created attachment 155300 [details] [review]
Bug 34724: (QA follow-up):

Replace href from thumbnail link with a # to keep pointer event. The href link gets reconstructed by showCover anyway
Remove return false from on click event, its not triggering a location change anymore because of the above
Remove check for imagenumber, its a non-null primary key, if the image exists, then the imagenumber must also exist
The link does not contain translatable strings, it doesnt need to be repeated twice in if-else just for a css class change

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 4 Pedro Amorim 2023-09-07 13:12:07 UTC
Hi David, I've submitted a follow-up patch.
Please take a look and SO if you agree, or make obsolete if you don't.
Comment 5 David Cook 2023-09-08 02:11:44 UTC
Comment on attachment 155300 [details] [review]
Bug 34724: (QA follow-up):

The "src" and Javascript change looked all right, but not the change to "loop.first".

I think the translation tools don't like Template Toolkit syntax embedded in HTML except for things like [% image.imagenumber | html %]. 

So even though there's a lot of duplicated HTML in the current template, it's for reasons. And I think that is the main reason.
Comment 6 David Cook 2023-09-08 02:12:25 UTC
(In reply to Pedro Amorim from comment #4)
> Hi David, I've submitted a follow-up patch.
> Please take a look and SO if you agree, or make obsolete if you don't.

If you want to redo your patch to not include that loop.first change, then I'd SO it.
Comment 7 Owen Leonard 2023-09-08 12:18:59 UTC
(In reply to David Cook from comment #5)

> I think the translation tools don't like Template Toolkit syntax embedded in
> HTML except for things like [% image.imagenumber | html %]. 

Correct: https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:_Template_Toolkit_markup_inside_HTML
Comment 8 Pedro Amorim 2023-09-08 16:01:19 UTC
Created attachment 155445 [details] [review]
Bug 34724: (QA follow-up):

Replace href from thumbnail link with a # to keep pointer event. The href link gets reconstructed by showCover anyway
Remove return false from on click event, its not triggering a location change anymore because of the above
Remove check for imagenumber, its a non-null primary key, if the image exists, then the imagenumber must also exist
Comment 9 Pedro Amorim 2023-09-08 16:01:45 UTC
Cool! Thank you both!
Comment 10 David Cook 2023-09-11 04:54:12 UTC
Created attachment 155472 [details] [review]
Bug 34724: (QA follow-up):

Replace href from thumbnail link with a # to keep pointer event. The href link gets reconstructed by showCover anyway
Remove return false from on click event, its not triggering a location change anymore because of the above
Remove check for imagenumber, its a non-null primary key, if the image exists, then the imagenumber must also exist

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 David Cook 2023-09-11 04:56:56 UTC
(In reply to Pedro Amorim from comment #9)
> Cool! Thank you both!

Np!
Comment 12 Katrin Fischer 2023-09-17 14:29:13 UTC
Created attachment 155758 [details] [review]
Bug 34724: Remove inline onclick handler for opac-imageviewer.pl

This patch moves the click handler for thumbnails for opac-imageviewer.pl
out of the inline HTML and out to an existing <script> element.

Test plan:
0. Apply the patch (including dependent patches)
1. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=localcover
2. Change both *LocalCoverImages sysprefs to "Show"
3. Go to
http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=AllowMultipleCovers
4. Change "AllowMultipleCovers" to "Allow"
5. Go to
http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29
6. Click "Images (0)" tab
7. Download or make two JPEG images
8. Upload the images
9. Go to http://localhost:8080/cgi-bin/koha/opac-imageviewer.pl?biblionumber=29
10. See one large image in the centre and two smaller thumbnails on the right side
11. Click on the thumbnail of the image that isn't displayed in the centre
12. Note that the image changes
13. Click the other thumbnail and note that the image changes back

PA amended patch: Added missing test plan step

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2023-09-17 14:29:16 UTC
Created attachment 155759 [details] [review]
Bug 34724: (QA follow-up):

Replace href from thumbnail link with a # to keep pointer event. The href link gets reconstructed by showCover anyway
Remove return false from on click event, its not triggering a location change anymore because of the above
Remove check for imagenumber, its a non-null primary key, if the image exists, then the imagenumber must also exist

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Tomás Cohen Arazi 2023-09-18 15:04:22 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 15 Fridolin Somers 2023-09-18 20:44:48 UTC
Pushed to 23.05.x for 23.05.04
Comment 16 Matt Blenkinsop 2023-10-12 16:01:03 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x