pkp/pkp-lib — comment on commit 50620d60a4c9e9563e9dd85f79150640115e1f22
File: js/controllers/form/FormHandler.js line 60
By beghelli:

hi Jason. Nice to see this feature implemented. I don't know if you are going to work more on this, but I found some issues and I will describe them here ok? So here it is:

1 - Why bind unload event, if users will not be able to stay on page when this event is triggered?

2 - I think you have to consider that you are creating a callback and binding it to an element that it's not inside the html element handled by the form handler. In practice, this can result into the callback being called even if the html form element is removed. You can check what I mean doing this:
- go to any workflow stage page;
- open the submission metadata form (catalog entry link, or information center link, whatever);
- change something in any field, but don't save. Either cancel or press esc to close the modal;
- then try to go to another page and you will see the callback being executed.

This happens because the role of the callbackWrapper method that was implemented in Handler.js is to call the passed method in the context of its handler object. So, when you created a beforeunload and unload handler attached to the window element, you passed the form handler object too. When the form html is removed (when you close the modal), its handler object, that was supposed to be destroyed, is still attached to the above events handlers.

3 - Maybe another requirement for what you are implementing is the advice of exiting the page being shown when a modal (that have a form with changed data inside of it) is closed. The cancel button inside forms will do the same thing (close the modal), so I think if you resolve the modal case you are ok with the form cancel too.

4 - And now, the last problem I saw, I promisse: if you have a form, and inside of it a grid with link actions to download a file, the beforeunload event is being triggered when you click on those actions (to download a file). I saw in the beforeunload event spec that it can be triggered when the location href is changed, which I think is the case of our RedirectRequest.js. This js is the one that it's used by the download files link actions.

If you want, I can try to help you with those issues. I don't have a clear solution for all those issues, but I have been working with our js framework at least for 6 months, and I am familiar with it now. Also, I think it will help if you take a look at our js documentation in PKP wiki. It's not too much content and it helped me a lot to better understand the concepts behind it.

Sorry for the long text. Regards.