Bug 18183 - jQuery append error related to script tags in cloneItemBlock
Summary: jQuery append error related to script tags in cloneItemBlock
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-28 14:50 UTC by Marcel de Rooy
Modified: 2019-10-14 19:56 UTC (History)
4 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:


Attachments
Bug 18183: jQuery append error related to script tags in cloneItemBlock (2.23 KB, patch)
2017-02-28 14:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18183: jQuery append error related to script tags in cloneItemBlock (2.25 KB, patch)
2017-10-08 11:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 18183: jQuery append error related to script tags in cloneItemBlock (2.30 KB, patch)
2018-01-02 10:59 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-02-28 14:50:51 UTC
This is a strange javascript error that I experienced on a 3.22 system and could not reproduce with the current (modified) framework in master. But since the code is essentially the same, it might still be possible.

The js error is:
TypeError: document.getElementById(...) is null
The line in koha-tmpl/intranet-tmpl/prog/js/additem.js that causes this error is:
$(clone).append(data);
While running append, jQuery seems to call .domManip and .globalEval internally.
The result on the interface is that you do not have an item block when receiving an order (AcqCreateItem=receiving).

Note that data comes from services/itemrecorddisplay.pl and is a very long string containing all item fields and even script tags for plugins. Between the ACQ framework in 3.22 and in master (copied from Default), I noticed some smaller changes in the lines for plugins.
Since the js variable "clone" contains a jQuery object for an element that is not yet added to the page, I could imagine a getElementById error. But it does not always occur, so some other condition must be triggered too. This condition appears to be connected to the script tags, since removing all plugins from the ACQ framework in 3.22 made the error disappear (no solution however).

Some interesting comments on jQuery's handling of appending script tags are found in http://stackoverflow.com/questions/610995/cant-append-script-element

Triggered by the getElementById error, I rearranged a few lines in cloneItemBlock so that I appended clone (the 'floating' div field for the new item) first to the outeritemblock div on the page and after that I appended the data string (item fields and script tags) to clone.
This resolved the js error on 3.22 and did not affect master.

Conclusion: We seem to stumble over a problem with our plugin script tags in jQuery's script evaluation when clone is not bound to the page yet.
The proposed patch just adds this workaround touching three lines. If we would like to change (again) the way we handle script tags for plugins, we will need much more effort.
Comment 1 Marcel de Rooy 2017-02-28 14:57:05 UTC
Created attachment 60731 [details] [review]
Bug 18183: jQuery append error related to script tags in cloneItemBlock

See the detailed explanation of the error on Bugzilla.
As a workaround, we move the append of clone to outeritemblock before the
append of data (long string with item fields and script tags) to clone.

This patch also moves two vars inside the success function, since they are
not used outside of it.

Test plan:
[1] Set AcqCreateItem to receiving.
[2] Receive an order. Verify that adding, updating or deleting an item block
    still works as expected.
Comment 2 Katrin Fischer 2017-10-08 11:52:38 UTC
Created attachment 67821 [details] [review]
Bug 18183: jQuery append error related to script tags in cloneItemBlock

See the detailed explanation of the error on Bugzilla.
As a workaround, we move the append of clone to outeritemblock before the
append of data (long string with item fields and script tags) to clone.

This patch also moves two vars inside the success function, since they are
not used outside of it.

Test plan:
[1] Set AcqCreateItem to receiving.
[2] Receive an order. Verify that adding, updating or deleting an item block
    still works as expected.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Marcel de Rooy 2017-10-09 07:02:03 UTC
(In reply to Katrin Fischer from comment #2)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Thanks. This was pending for some time already.
Comment 4 Josef Moravec 2018-01-02 10:59:19 UTC
Created attachment 70224 [details] [review]
Bug 18183: jQuery append error related to script tags in cloneItemBlock

See the detailed explanation of the error on Bugzilla.
As a workaround, we move the append of clone to outeritemblock before the
append of data (long string with item fields and script tags) to clone.

This patch also moves two vars inside the success function, since they are
not used outside of it.

Test plan:
[1] Set AcqCreateItem to receiving.
[2] Receive an order. Verify that adding, updating or deleting an item block
    still works as expected.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Jonathan Druart 2018-01-02 15:12:46 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 6 Nick Clemens 2018-01-15 16:47:22 UTC
Awesome work all, pushed to stable for 17.11.02
Comment 7 Fridolin Somers 2018-01-22 10:56:38 UTC
Pushed to 17.05.x for v17.05.08