Show edit page confirmation dialog on tree view file change

[bytedream]

Currently, when editing or deleting a file and the edit/commit form has changes, navigating the file tree will discard all changes without any warning. This PR prevents partial reloading when the edit form has unsaved changes, which will trigger a browser native warning dialog.

[silverwind]

So I understand that the JS would navigate to another entry, ignoring the "dirty form" state, and by adding this condition, it makes it skip the JS navigation via navigateTreeView and instead trigger a full-page load with the dirty checking done inside beforeunload by jquery.are-you-sure.

A better solution would be to make navigateTreeView perform the dirty checking and warning itself. That way, it will work without a disruptive full-page load when the user wants to discard their changes.

[wxiaoguang]

A better solution would be to make navigateTreeView perform the dirty checking and warning itself. That way, it will work without a disruptive full-page load when the user wants to discard their changes.

I don't think it is a must at the moment. are-you-sure.js module has many internal logic, since it is there, we should reuse that module, but not re-invent the wheel and write a new "are-you-sure" from scratch, unless you would replace the legacy "are-you-sure.js" completely

---

The "dirty check" (document.querySelector('.repo-view-content .form.dirty')) can be a helper function exported from the are-you-sure.js to make the code more stable and maintainable.

The real logic is here:

[silverwind]

unless you would replace the legacy "are-you-sure.js" completely

Yes I think it's best to completely replace it with custom logic eventually. The plugin only works for full page navigation, while here we have a case of JS-based navigation (pushState and replaceState). It needs to support both forms of navigation.

The "dirty check" (document.querySelector('.repo-view-content .form.dirty')) can be a helper function exported from the are-you-sure.js to make the code more stable and maintainable.

Yes, this would be a good way to expose the dirty checking as a function used for JS-based navigation.

[silverwind]

Oh and I still think the "unsaved warning" should eventually be replaced by a mechanism that automatically saves and restores unsaved content (based on URL) if the user decides to navigate back. E.g. like https://github.com/github/session-resume.

[bytedream]

The "dirty check" (document.querySelector('.repo-view-content .form.dirty')) can be a helper function exported from the are-you-sure.js to make the code more stable and maintainable.

Sorry for the delay, I've relocated it to a helper function now

[silverwind]

Tested it, works. I notice it does a full-page load after the popup is triggered, because it let's the onClick process normally and then the beforeunload event fires and is caught and the popup is displayed.

I think it could be optimized to do it without the full-page reload but that requires more changes in are-you-sure because it works by returning a string to beforeunload which triggers the message, while manual popup would need to be done with window.confirm.

So this is LGTM, and more improvements can be done later.

[silverwind]

I checked further: This specific browser popup can only be triggered by returning a string from beforeunload. The string is not actually displayed in at least Firefox and Chrome. Using window.confirm during beforeunload does not work and navigates away. So given the constraint the the popup is unchanged, this implementation with the full-page load is likely the only solution.