Bug 34613

Summary:	Remove onclick event attributes from Verovio midiplayer.js
Product:	Koha	Reporter:	David Cook <dcook>
Component:	OPAC	Assignee:	David Cook <dcook>
Status:	CLOSED FIXED	QA Contact:	Katrin Fischer <katrin.fischer>
Severity:	minor
Priority:	P5 - low	CC:	fridolin.somers, oleonard
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20397 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22581
Change sponsored?:	---	Patch complexity:	Trivial 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
Circulation function:
Bug Depends on:
Bug Blocks:	34709
Attachments:	Bug 34613: Replace onclick attributes in midiplayer.js Bug 34613: Replace onclick attributes in midiplayer.js Bug 34613: Replace onclick attributes in midiplayer.js

Description David Cook 2023-08-25 01:02:45 UTC

Content-Security-Policy isn't compatible with onclick event handlers, and generally speaking we're trying to remove event attributes from all HTML anyway.

This is a third-party library from RISM, but it hasn't been updated in years, so we might need to patch this one ourselves. 

I'm willing to look at doing a patch and pull request for RISM but we'll see if they accept it...

Comment 1 David Cook 2023-08-25 01:03:44 UTC

It's interesting to note that RISM doesn't appear to use the MIDI player on their own site although they do seem to use Verovio to display incipits: https://opac.rism.info/metaopac/singleHit.do?methodToCall=showHit&curPos=1&identifier=251_SOLR_SERVER_63559013

Comment 2 David Cook 2023-08-25 01:30:08 UTC

Created attachment 154767 [details] [review]
Bug 34613: Replace onclick attributes in midiplayer.js

This patch replaces the onclick attributes in midiplayer.js
with Javascript click handlers.

To test:
0. Apply patch
1. Clear browser cache (it may be necessary to go into the
Network tab and explicitly disable cache)
2. Add 031 subfields 2gopnu to Default framework
3. Create a catalogue record with a 031 like follows:
2: pe
g: G-2
o: 4/4
p: 4bB''C2bE/2F4GbB/'bB2''C4D/F2.bE/4GG2bB/4'''C2C4''bB/4bE2G4bB/4bE2.F/
n: xFCGD
4. Enable the following sysprefs:
OPACShowMusicalInscripts
OPACPlayMusicalInscripts
5. Go to OPAC record view and click "Play this sample"
6. Play with the "Pause/Start" and "Stop" buttons
7. If the buttons work, it means the patch worked

Comment 3 David Cook 2023-08-25 01:45:41 UTC

I've got an upstream pull request at https://github.com/rism-digital/midi-player/pull/32

Comment 4 David Cook 2023-08-28 00:06:55 UTC

(In reply to David Cook from comment #3)
> I've got an upstream pull request at
> https://github.com/rism-digital/midi-player/pull/32

By the way, my pull request was merged into the master branch of midi-player.

So we could alternatively just pull down the latest midi-player. Both this Koha patch and that upstream patch are the exact same, although the upstream version has some other differences with our version I think...

Comment 5 David Cook 2023-08-28 00:09:27 UTC

(In reply to David Cook from comment #4)
> So we could alternatively just pull down the latest midi-player. Both this
> Koha patch and that upstream patch are the exact same, although the upstream
> version has some other differences with our version I think...

It looks like mostly whitespace differences, and two other lines changed. I'll leave up to QA/RM what they'd prefer here...

Comment 6 David Cook 2023-09-05 02:36:32 UTC

This is an easy signoff. Test plan gives you everything you need.

Comment 7 Owen Leonard 2023-09-05 11:08:24 UTC

Created attachment 155215 [details] [review]
Bug 34613: Replace onclick attributes in midiplayer.js

This patch replaces the onclick attributes in midiplayer.js
with Javascript click handlers.

To test:
0. Apply patch
1. Clear browser cache (it may be necessary to go into the
   Network tab and explicitly disable cache)
2. Add 031 subfields 2gopnu to Default framework
3. Create a catalogue record with a 031 like follows:
2: pe
g: G-2
o: 4/4
p: 4bB''C2bE/2F4GbB/'bB2''C4D/F2.bE/4GG2bB/4'''C2C4''bB/4bE2G4bB/4bE2.F/
n: xFCGD
4. Enable the following sysprefs:
   OPACShowMusicalInscripts
   OPACPlayMusicalInscripts
5. Go to OPAC record view and click "Play this sample"
6. Play with the "Pause/Start" and "Stop" buttons
7. If the buttons work, it means the patch worked

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Comment 8 Katrin Fischer 2023-09-17 14:39:25 UTC

Created attachment 155761 [details] [review]
Bug 34613: Replace onclick attributes in midiplayer.js

This patch replaces the onclick attributes in midiplayer.js
with Javascript click handlers.

To test:
0. Apply patch
1. Clear browser cache (it may be necessary to go into the
   Network tab and explicitly disable cache)
2. Add 031 subfields 2gopnu to Default framework
3. Create a catalogue record with a 031 like follows:
2: pe
g: G-2
o: 4/4
p: 4bB''C2bE/2F4GbB/'bB2''C4D/F2.bE/4GG2bB/4'''C2C4''bB/4bE2G4bB/4bE2.F/
n: xFCGD
4. Enable the following sysprefs:
   OPACShowMusicalInscripts
   OPACPlayMusicalInscripts
5. Go to OPAC record view and click "Play this sample"
6. Play with the "Pause/Start" and "Stop" buttons
7. If the buttons work, it means the patch worked

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 9 Tomás Cohen Arazi (tcohen) 2023-09-18 18:54:52 UTC

Pushed to master for 23.11.

Nice work everyone, thanks!

Comment 10 Fridolin Somers 2023-09-18 21:30:52 UTC

Pushed to 23.05.x for 23.05.04

Comment 11 Matt Blenkinsop 2023-10-12 16:00:52 UTC

Nice work everyone!

Pushed to oldstable for 22.11.x