Bug 30975 - Framework plugins do not work on cloned fields/subfields
Summary: Framework plugins do not work on cloned fields/subfields
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 29155
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-16 09:15 UTC by Julian Maurice
Modified: 2022-07-29 13:50 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30975: Fix framework plugins not working on cloned fields/subfields (92.33 KB, patch)
2022-06-16 09:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 30975: Fix framework plugins not working on cloned fields/subfields (92.40 KB, patch)
2022-07-29 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2022-06-16 09:15:11 UTC
Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event handlers to the cloned elements, and that was removed in jQuery 1.8.0 (I did not confirm but it's possible it continued to work after that thanks to jQuery Migrate)
Comment 1 Julian Maurice 2022-06-16 09:16:43 UTC
Created attachment 136135 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) Only 'click', 'focus' and 'blur' events are listened to now. They are
   the only ones we use, and the only ones we actually need.
   Events removed: change, mouseover, mouseout, mousedown, mouseup,
   mousemove, keydown, keypress, keyup.
   If you ever need one of those, you can still add event listeners in
   the 'focus' event handler, and remove them in the 'blur' event
   handler
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated. All framework plugins
   have been updated
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields
Comment 2 Caroline Cyr La Rose 2022-06-30 12:38:16 UTC
Hi Julian,

I would appreciate a more detailed test plan. barcode.pl is linked to an item field for me, so I can't clone it. All the other plugins for specific fields (e.g. 000, 005, 008) are on non-repeatable fields too. 

Do we have to modify the framework in order to test this?

Thanks,

Caroline
Comment 3 Julian Maurice 2022-07-11 07:00:46 UTC
> Do we have to modify the framework in order to test this?

Yes, but I'd say it's no big deal if you cannot do these tests on a plugin if that plugin is always used in a non-repeatable field/subfield.
Testing that the plugin still works in its single field should be enough.
Comment 4 Julian Maurice 2022-07-29 13:50:13 UTC
Created attachment 138318 [details] [review]
Bug 30975: Fix framework plugins not working on cloned fields/subfields

Since bug 29155 (Upgrade jquery version to 3.6.0) framework plugins do
not work on a cloned field/subfield.
That is because we rely on jQuery's `.data('events')` to clone event
handlers to the cloned elements, and that was removed in jQuery 1.8.0
(I did not confirm but it's possible it continued to work after that
thanks to jQuery Migrate)

It is apparently still possible to access these event handlers by using
`jQuery._data(element, "events")` but here's what jQuery 1.8.0 release
notes says about it:
"this is not a supported public interface; the actual data structures
may change incompatibly from version to version."
So we should not rely on it.

Instead, what this patch does is use event delegation [1].
Events are bound to a parent container, so when elements are added
dynamically inside that container, we don't need to re-attach event
handlers manually

This patch also comes with a bit of cleanup, and introduce "breaking
changes" (they are breaking changes only if you happen to have custom
framework plugins):
1) Only 'click', 'focus' and 'blur' events are listened to now. They are
   the only ones we use, and the only ones we actually need.
   Events removed: change, mouseover, mouseout, mousedown, mouseup,
   mousemove, keydown, keypress, keyup.
   If you ever need one of those, you can still add event listeners in
   the 'focus' event handler, and remove them in the 'blur' event
   handler
2) Event handlers now takes a single parameter that is an Event object
   It just makes the code a lot less complicated. All framework plugins
   have been updated
3) Event handlers do not pollute the global scope anymore

[1] https://learn.jquery.com/events/event-delegation/

Test plan:
- Go to every page that has a MARC editor and verify that plugins still
  work. This includes addbiblio.pl, additem.pl, authorities.pl,
  neworderempty.pl, orderreceive.pl
- Test plugins that use 'focus' event (for instance barcode.pl), 'blur'
  event (callnumber.pl) and 'click' event (almost all the others)
- Test that plugins work on cloned fields/subfields