Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need to prevent re-performing some JavaScript page setup actions when they've already been performed #943

Open
makyen opened this issue Aug 21, 2022 · 2 comments
Assignees
Labels
area: ui Anything to do with the user interface, particularly UX concerns status: needs investigation This needs more investigation. status: planned We like the idea and might get around to it at some point. type: bug Buggy or otherwise incorrect behaviour.

Comments

@makyen
Copy link
Contributor

makyen commented Aug 21, 2022

The underlying issue is that the $(document).on('turbolinks:load', ); event can fire more than once on pages where it's previously fired. This mostly happens when using the browser's forward and back capability.

On metasmoke, we typically use the utility function onLoad() to add functions to be run on that event. There are a considerable number of places throughout the JavaScript where it's assumed that the operations which are being performed within the onLoad() callback are operating on HTML on which they have never been performed. When these are run a second time on the same DOM/HTML, it can result in a variety of unintended consequences, including multiple manipulations and adding multiple event listeners (when the event listener being added is not the same function Object; e.g. when it's anonymous) which are performing the same action.

There are, of course, also things which do need to be run again when the same DOM/HTML is presented to the user. As a result, it's not a case where we just need to prevent everything added through onLoad from being re-executed on the same DOM/HTML.

@makyen makyen added type: bug Buggy or otherwise incorrect behaviour. status: planned We like the idea and might get around to it at some point. area: ui Anything to do with the user interface, particularly UX concerns status: needs investigation This needs more investigation. labels Aug 21, 2022
@makyen makyen self-assigned this Aug 21, 2022
@makyen
Copy link
Contributor Author

makyen commented Aug 21, 2022

There are a variety of ways by which this can be addressed. We probably want a combination of approaches. As a first swag at how to address this, we should probably change the onLoad() semantics such that by default callbacks are not called a second time for DOMs which have already had onLoad() callbacks called on them. We could have as an option when adding the callback through onLoad() that the callback will always be called, which would then place the burden of making sure not to perform double actions on the callback.

It's unclear to me if we need to handle the case where the turbolinks:load event is fired on HTML which has already been manipulated, but which is a new DOM. In such a case, HTML manipulations would have occurred, but any event listeners would need to be (re-)added to the newly recreated DOM. I know we do experience the case where it's the same DOM, which retains the previously added event listeners.

@makyen
Copy link
Contributor Author

makyen commented Aug 21, 2022

On Firefox 103, testing indicated that the browser's forward and back buttons resulted in similar replacement of the document.body as is normally done by Turbolinks, but the HTML which is used for the replacement is the already modified HTML from the recent visit. In other words, the JavaScript context is kept and events on the window and document are kept, but all events on the document.body, or lower, are removed. This is very similar to a normal Turbolinks transition to a new page, just that the already modified HTML is used for the document.body replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ui Anything to do with the user interface, particularly UX concerns status: needs investigation This needs more investigation. status: planned We like the idea and might get around to it at some point. type: bug Buggy or otherwise incorrect behaviour.
Development

No branches or pull requests

1 participant