Replace jquery.are-you-sure with @github/session-resume

[silverwind]

With @github/js-session-resumable, content in <input> and <textarea> is automatically saved and restored on page reload, eliminating the need to display a popup. This is using a allowlist-based approach where each restorable element needs to have a js-session-resumable class, which is much better than the previous ignore-dirty madness.

Currently I've only added this to the markup editor and the new issue/pr form.

@wxiaoguang there seems to be an existing mechanism somewhere that restores the markup editor content even without this resumation enabled. Can you help me find it so I can remove it?

[wxiaoguang]

• [Summary] Improve issue/PR comment editing experience #23290 (comment)

Actually session-resume doesn't work well either. Think about 2 cases:

1. Content in the textarea -> submit the form -> clean the draft -> network fails -> content lost
2. Content in the textarea -> submit the form by AJAX -> draft is still in local storage

That's why "AJAX" is before the "LocalStorage Draft" in "that" summary issue.

[silverwind]

I don't understand these points entirely but I think they are not blockers. The API used here is very flexible, I imagine you can extend it with custom AJAX-related functionality.

[wxiaoguang]

1. Content in the textarea -> submit the form -> clean the draft -> network fails -> content lost
   • It still causes content losing, doesn't really help
2. Content in the textarea -> submit the form by AJAX -> draft is still in local storage
   • Wrong content will be left in the local storage and confuse users in next page.

[silverwind]

What is "clean the draft"?

[wxiaoguang]

And one more problem: some UI heavily depends on the "input" event.

Even if the "input" element content is restored, the UI would be in a inconsistent state (eg: auto showing something when the input has some contents).

I do not think "session-resume" is the right approach.

[silverwind]

And one more problem: some UI heavily depends on the "input" event.

We can trigger a manual input event after restoration if really needed. I don't see an issue.

I do not think "session-resume" is the right approach.

I think it is absolutely the right approach and a huge improvement over previous annoying modal popup. GitHub has been using it for years. It ought to work for us as well.

[wxiaoguang]

And one more problem: some UI heavily depends on the "input" event.

We can trigger a manual input event after restoration if really needed. I don't see an issue.

It needs a complete framework design with documents and demos.

[puni9869]

I agree with silvermind here, point 2 as mentioned by wxiaoguang ajax is successful we can call the form to reset.
but for point 1 doubt if network fails then we need to think the work-around.

[silverwind]

I don't want to go overboard with this. Just a simple replacement. If there are issues later, we can solve them individually.

Generally, improvements related to network interruptions should like be upstreamed into the module.

[wxiaoguang]

I don't want to go overboard with this. Just a simple replacement. If there are issues later, we can solve them individually.

Without a clear design and explicitly defined behaviors, it might go on the wrong way, just like some old designs ........

[silverwind]

The module already dispatches a change event, which is all that is needed. A input event would be incorrect because there was no user input.

[wxiaoguang]

One more problem, I do not think GitHub's "session-resume" works for various cases. I have experienced content losing many times on GitHub. Actually, GitHub session-resume declares:

.... revisits the page again in their current browser session (excludes separate tabs).
Not designed for persisted crash recovery.

I guess if people close the tab, then the content won't be able to recover.

[silverwind]

sessionStorage only lives for the duration of the tab, but we can easily switch to localStorage which persists practically indefinitely, at the cost of likely bloating the storage over long-term with unsaved content.

[wxiaoguang]

sessionStorage only lives for the duration of the tab, but we can easily switch to localStorage which persists indefinitely, at the cost of likely bloating the storage over long-term with unsaved content.

Yup, so, there are really a lot of questions. That's why I believe that it needs a design document and demos first, it needs to have explicitly defined stable behaviors.

[silverwind]

I'm not going to write documents for this, sorry.

For me, this is good enough. All I personally need is persistence across F5 and I am happy how it works on GitHub.

A more elborate system that persists into localStorage could come later, but it will likely need some form of cleanup mechanism, e.g. store entries with timestamps and discard older than x days but imho this is a massive complexity increase that I'm not going to be willing to write or maintain. IDK why you want to blow everything out of proportions.

[silverwind]

@wxiaoguang I hope you can accept the current simple solution. It works for GitHub, so it likely will work for us.

I'm definitely not going to write a overengineered implementation for something I don't personally need.

[wxiaoguang]

IDK why you want to blow everything out of proportions.

Do you have real cases to say so? I have fixed so many unfixable bugs, a lot of them are caused by bad designs. Even for the markdown editor, the refactoring got stale for that long time, it's also partially caused by the messy code / bad design in history. I have spent so much time on making the code base clear and stable.

At least, isn't it nature to make every developer understand the newly introduced behaviors clearly?

[wxiaoguang]

I will try to summarize the are-you-sure and session-resume problems from my understanding. Correct me if I am wrong.

The background

Avoid losing user input, to improve user experience

are-you-sure package

It shows an "confirm" dialog before user navigates away from a page which has user inputted content.

It has some problems:

• Too annoying in most cases

session-resume package

It stores the user inputted content into session storage, and restore it when the page is reloaded.

The limitation:

• It doesn't work across browser tabs

The possible approaches

Could session-resume totally replace are-you-sure ?

I do not think so, because users can't restore the content when the current tab is closed by mistake.

(but if most users can accept such content losing, I won't block)

How to improve the user experience?

1. Introduce session-resume to make it work with are-you-sure
2. Limit are-you-sure only on forms with textareas and explicated declared elements (maybe it's worth to rewrite one, it's not that complex)

[silverwind]

I think we can try switchting this to use localStorage. That will persist data pretty much indefinitely at the cost of potentially accumulating some garbage over time. It may also be possible to add a timestamp to those entries to perform a "garbage collection".

[silverwind]

I guess using localStorage will be the way to go, even without a garbage collection, meaning it will persist unsaved content forever. This mechanism will be active only on very few selected pages, so with current implementation, one will have at most 4 stored values per repo (issue/pr title/body), which should be acceptable.

Another candidate for this mechanism may be the install page.

[silverwind]

@wxiaoguang do you by chance know which mechanism currently restores the issue/pr textarea? Is that a browser-native mechanism or something from our JS?

[delvh]

Not @wxiaoguang, but I think it is the local storage/browser cache

[silverwind]

As I understand it, browser should clear a textarea on page reload. Enter something and hit Run here:

https://jsfiddle.net/silverwind/48oqL6y2/

The only reason a browser might keep it is when the page is restored from bfcache, but bfcache does not apply on a reload and in fact our beforeUnload event that jquery.are-you-sure registers makes all pages with such forms ineglible for bfcache, at least in Firefox.

[wxiaoguang]

@wxiaoguang do you by chance know which mechanism currently restores the issue/pr textarea? Is that a browser-native mechanism or something from our JS?

Maybe related to:

• Fix cache-control header clearing comment text when editing issue #22604 (comment)

?

[silverwind]

Hmm could be, but I guess this behaviour may vary between browsers. Generally bfcache seems to be something that is not standardized at all, so it will be good to have our own restoration in any case.

[silverwind]

Not going to continue here, but I still feel the approach is right and it should be attempted again to migrate away from are-you-sure, especially once we are close to ripping out jQuery from the frontend it will become necessary.