Opened 3 years ago

Closed 3 years ago

#2176 closed defect (fixed)

SusiMail > > onbeforeunload (Doesn't work as intended) / Compose.js (not needed)

Reported by: Pimp Trizkit Owned by:
Priority: trivial Milestone: 0.9.34
Component: apps/susimail Version: 0.9.33
Keywords: compose.js,, onbeforeunload Cc:
Parent Tickets: #2087 Sensitive: no


First off, I'm new to using GitHub and messed up my commits. This new commit is unrelated to my last one and I added directly to my fork. Which had a pull request pending, so it added this commit to that PR, unfortunately. I don't know how to undo all that. So I'm just going to post it here and maybe it will work itself out.

Please read the full write-up here: GitHub-onbeforeunload


On the compose page of susimail. It sets windows.onbeforeunload to a function which only returns a string. Supposedly the browser will use this string, however, this has not been the case for a while now in all major browsers. I have snapshots of Chrome/FF/Edge if anyone wants to see. And I see Opera is the same. So first off, the actual string is not needed. And the translation in the message database can be removed. See my full write up about that.

Long story… short-ish. onbeforeunload in susimail is only used when someone hits the back button or closes the tab. It will warn you that your edits were not saved. However, for some reason this feature is not on the cancel button, which is where i would think it would be.

Anyhow, I would just suggest to remove the darn thing entirely. And that will clean up a lot of stuff, more about that in my write-up.

But if it must be kept. Then my proposed commits on GitHub-onbeforeunload are a solution.

Either way, the file compose.js is not needed. Its contents were only one line that was not actually needed on any other pages. A waste of linking a resource. I just replaced it with some boolean action. That boolean action actually fixes a crash with the cancel button when used on the Susi Settings screen because there was no onbeforeunload event set. And therefore the coder didn't link in compose.js.



Change History (9)

comment:1 Changed 3 years ago by zzz

Parent Tickets: 2087
Status: newopen

Sure, the message isn't displayed. We knew that already. But it works exactly as intended, in that it stops the user from leaving the page without clicking LeavePage? or StayOnPage? (in firefox that's what it's labeled). "Removing the darn thing entirely" as you propose would remove that protection of either sending or trashing the draft.

We don't have a save-as-draft function now because we have no place to store it.

When we add support for a draft folder - see #2087 - we will have a third option, to save as a draft. This will maybe need to change or be enhanced for background saving of drafts via js. That's probably for 0.9.35 or 36.

I don't see anything in the github link that changes compose.js to be a boolean. Perhaps you can paste that diff here?

comment:2 Changed 3 years ago by Pimp Trizkit

Sorry if I was unclear. My proposal was to remove the file… entirely. compose.js is not required at all in my proposal, so its contents need not to be changed, but rather the file deleted. My commits were in and I removed the link to compose.js,(..late, i forgot to put that in my commit originally, i just fixed it here..) thus the boolean logic is in there, as you see in my changes on GitHub?. Would you like to see the difference here? or you can just follow the link, its right there, just a few lines.

On my write-up on GitHub?, you can skip the first paragraph if the intent is to keep using the onbeforeunload event… (even tho it sounds like it might be ripped out anyhow for a more flavorful option branching off the folder/background saving/etc update)

I apologize if I misinterpreted the intent. I figured if i2p was storing a translation for the message then the intent was to display that message. That string seems to just be mud in the system, if it never gets displayed, imho.

My a) misinterpretation of intent plus I b) personally feel that the end user experience with this is rough to say the least, plus the fact that c) its not used on the cancel button for some reason… led me to feel that "the darn thing" could be ripped out entirely, lol. Sorry for my french.

But anyhow, my commits were for staying with onbeforeunload usage. So here, I fixed up a few things while keeping the intent you just mentioned, more detail on the what-and-why on my write-up on GitHub?.


Last edited 3 years ago by Pimp Trizkit (previous) (diff)

comment:3 Changed 3 years ago by Pimp Trizkit

I just re-read your response. And I'm thinking that maybe my write-up wasn't clear. Let me try it a different way.

The intent is to (or to not) pop up a warning about abruptly leaving the page (a warning that might get suppressed on a per-browser basis). So that is boolean logic, to pop it up, or to not pop it up. Stored in the new variable beforePopup (line 2169 from commit/PR). The default is that it is true that we want the popup. Set it to false at any time if we don't.

onbeforeunload looks at event.returnValue for basically undefined or not. So thats the boolean action. Keep in mind, If you set event.returnValue to true OR false it will show the popup. So i just wrote a toggle around that set.

compose.js only removed the event by clearing out window.onbeforeunload… thus turning off the popup. Which can now be done by executing beforePopup = false; instead of calling a function from a linked file that is not reused on another page. Which causes a crash (settings screen) when attempting to re-use the cancel button without linking in the otherwise not needed function.


Last edited 3 years ago by Pimp Trizkit (previous) (diff)

comment:4 Changed 3 years ago by Pimp Trizkit

ok, I take back some of what I said about Edge. I'm not sure what problem I was noticing before that was different between the browsers. I'm not seeing it. It does seem to respond to on events. I was thinking.. that can't be right… I thought that seemed weird. So I re-tested.

Anyhow, my fix works for it regardless. Except that Edge doesn't suppress.


comment:5 Changed 3 years ago by zzz

  • OK, the "boolean action" is in the patch, I thought it was something else
  • OK, by "intent" you meant only the display of the translated string, not the whole popup. Obviously if it's not displayed by any browser there's no need to keep it.
  • You're now saying Edge does display the custom message? Or just that it does display a popup?
  • Isn't best practice to put js in a js file, not in the head? So the browser only has to look at it once, and perhaps enabling a more secure Content-Security-Policy?
  • If so, should we keep compose.js and move whatever fixed up code into it, rather than delete it. If we don't need the translated custom message, even better. If we still need it, perhaps best practice would be to just define a string variable in the head, but access it from the js file? I'm thinking we should keep it because we probably have more js coming for drafts support, as I said in comment 1 above.

So then the complete fix would be to replace the contents of compose.js with:

let beforePopup = true;
window.addEventListener('beforeunload', (e)=>{if (beforePopup) e.returnValue=true;} );

together with this change in WebMail?:

-			buf.append(" onclick=\"cancelPopup()\"");
+			buf.append(" onclick='beforePopup=false;'");

how does that sound?

comment:6 Changed 3 years ago by Pimp Trizkit

I do like where your mind is going and all. But I think there is still some confusion lingering around.

  • First off, Edge. I'm sorry for the confusion. Edge is behaving the same as Chrome/FF but without the suppression. Therefore, it *is* popping up a box and *without* the custom message. As I mentioned in my write-up, I can't find a browser that does use the custom string. So yes, remove the string.
  • A string variable in the head is not a bad idea; if we needed the string. However, to keep it in one place, we would put it with the rest of the code, in the link, or where ever.
  • Keep in mind. There is already more js in the head than there is in compose.js which is only a one liner that does this: window.onbeforeunload = null;. Which is linked in from the head. I just wrote new js, for the head, that is roughly the same size as the old js but the link is not required.
  • Are you suggesting to move all the js into compose.js? And, thus, removing its old contents entirely (because they are not needed in my proposal) and replacing it with your quoted content above. This should work fine. I just don't really see the reason why.
  • <Putting js in in a js file and not the head> - "So the browser only has to look at it once" → Sure, but the browser only has to look at the js once as long as all the js is together, in general, somewhere, in a linked file or in the head. Breaking up the js is what it was before. I propose to put it all together in the head because its only a couple lines and not reused code, which is part of the main reason for linking a file is to have reusable code. A linked resource is slower than code in the head. It creates another network transaction. And splices it in. You can even see the time each of these linked resources takes to process in the network tab of the web console. Therefore they use more resources. I feel the main idea of linked code is modularity. So I tend to only used linked code at all if its reused on other pages. Regardless of its size and functionality. So this goes back to the future. Ok, I could see keeping it if there are plans to use it in the future (which we dont actually know). If removed, adding it back in when we know the when and why it is required, is simple as well. The new JS from the future could definitely go here. Especially if any of it is reused. But if not reused, I would just keep it in the head of the page that uses it (Unless the next point below shows its ugly face). This will help others to follow along and do the same until a link is required. But in other words, I don't use something extra and heavy (linked resource) unless I need it (code reuse/modularity).
  • <Putting js in in a js file and not the head> - "and perhaps enabling a more secure Content-Security-Policy?" → "perhaps" being the key word here, (and I don't know how much more secure you can get than i2p, hehe)… Now, I don't know all the in's and out's to CSP, and a quick google search on this point was of no help. Maybe someone else knows more who is reading this? But I would have a hard time believing (without seeing it), that the CSP on a js file is all that different than one on html. They know there is js in html, I would assume the same precautions are taken in both places. But you are right, this *could* be a point. But it is probably browser specific, And/or not actually doing anything different between a js file and a html file to necessitate a linked resource. And with no evidence as of yet…
  • Long story short. Yes your fix above sounds good. I would just keep it all in the head tho.


comment:7 Changed 3 years ago by Pimp Trizkit

I guess I should admit. That even tho it would be a waste of a link. I see that if your planning on a lot of JS on this page in the future then for readability a link would be nicer.

And truthfully speaking, there will probably be some re-used code in the the new folder update thing anyhow.

I will concede my point about adding it to the head.


(a little TL;DR —> I actually did a full test of this in the past. And as one could guess, a fully embedded js, especially sound and pictures, was the total fastest and less weight for both server and client. And, of course as guessed, highly depending on the number of links involved… ie: the more links converted to embedded; the more the difference…. Sorry, I'm sometimes a speed junkie with coding. I will sacrifice some better practices for better performance. For instance, I love replacing one-line blocks with just the one line. Those extra {} literally add another to the stack. Which is not needed for one line. That's why the {} are optional for one liners. So I always use that option. And I heart the ,)

Last edited 3 years ago by Pimp Trizkit (previous) (diff)

comment:8 Changed 3 years ago by Meeh

I've read over now, both here and on github. We should accept this change. Not sure if CSP will help since it's easy to avoid, but for sure we need a good way to make sure it's only JS originated from the console that's allowed to run. And not something attached in an email or something like that.

comment:9 Changed 3 years ago by zzz

Milestone: undecided0.9.34
Resolution: fixed
Status: openclosed

Fixed as described at bottom of comment 5 above, in 32b8793a8ba09a4e59dbe1f50cd720b697c727db to be 0.9.33-16.
Also added to config page, and versioned the js.

Note: See TracTickets for help on using tickets.