Bug 34613 - Remove onclick event attributes from Verovio midiplayer.js
Summary: Remove onclick event attributes from Verovio midiplayer.js
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: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 34709
  Show dependency treegraph
 
Reported: 2023-08-25 01:02 UTC by David Cook
Modified: 2023-10-12 16:00 UTC (History)
2 users (show)

See Also:
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


Attachments
Bug 34613: Replace onclick attributes in midiplayer.js (2.62 KB, patch)
2023-08-25 01:30 UTC, David Cook
Details | Diff | Splinter Review
Bug 34613: Replace onclick attributes in midiplayer.js (2.68 KB, patch)
2023-09-05 11:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34613: Replace onclick attributes in midiplayer.js (2.74 KB, patch)
2023-09-17 14:39 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-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 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