[Discussion] Refactor Save & Recover in Comment/Legacy Editor Form

[noi5e]

Just submitted a PR that refactors editor.js's save & recover methods.

Here's the old version of save and recover:

plots2/app/assets/javascripts/editor.js

Lines 112 to 117 in 2188e0a

	save() {
	localStorage.setItem('plots:lastpost', this.textAreaValue);
	}
	recover() {
	this.textAreaElement.val(localStorage.getItem('plots:lastpost'));
	}

Basically, editor.js sets an eventHandler here below. Any time the user types into a textarea, $E.save runs:

plots2/app/assets/javascripts/editor.js

Line 35 in 2188e0a

$E.textarea.bind('input propertychange', $E.save);

The main concern here is... What happens on a page with multiple comment forms?

The plots:lastpost key in localStorage.setItem('plots:lastpost', this.textAreaValue); is static. Because of that, something like this can happen:

1. User enters some text into #text-input-main, triggering save eventListener.
2. User's input is saved to plots:lastpost in localStorage.
3. User opens up a SEPARATE, unrelated Reply comment form, #text-input-reply-1.
4. User hits Recover button in the Reply form #text-input-reply-1.
5. BUG: editor.js looks up plots:lastpost in localStorage, then inputs the text value from #text-input-main into reply form #text-input-reply-1, which is weird!

---

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

[noi5e]

My PR #9132 refactors the save and recover methods like so:

plots2/app/assets/javascripts/editor.js

Lines 118 to 125 in 0811c5e

	save(thisEditor) {
	const storageKey = "plots:" + window.location.pathname + ":" + thisEditor.commentFormID;
	localStorage.setItem(storageKey, thisEditor.textAreaValue);
	}
	recover() {
	const storageKey = "plots:" + window.location.pathname + ":" + this.commentFormID;
	this.textAreaElement.val(localStorage.getItem(storageKey));
	}

The unique comment form ID string is embedded in the localStorage key as this.commentFormID, so lookup can be done based on comment form!

#text-input-main has the same ID across different pages (think, any given research note, and then a separate question), so the localStorage key also saves the pathname of the page. This is to distinguish between text written in main on one page vs another. This avoids the bug of the user hitting recover and recovering text from a different page.

[noi5e]

Discussion Point 1: When/How to Save User Input?

The old way of saving user input happened like this. As soon as a comment form was selected, this eventListener was attached:

plots2/app/assets/javascripts/editor.js

Line 35 in 6ded8c4

$E.textarea.bind('input propertychange', $E.save);

plots2/app/assets/javascripts/editor.js

Lines 112 to 114 in 6ded8c4

	save() {
	localStorage.setItem('plots:lastpost', this.textAreaValue);
	}

This means that every time a user made any kind of input (typed, deleted, pasted text, etc.) in a given text-input, their input would be automatically saved into localStorage. Basically this means two things:

1. The save button is actually useless... I think? Any kind of change to text-input's value will be saved automatically. So clicking the save button doesn't do anything.
2. Maybe the function of the save button is more symbolic? Without a save button, users wouldn't know that they could recover their text.

And where does that leave the recover button? In the past, the recover button was very buggy. For a few reasons:

1. Because editor always overwrites saved text to the plots:lastpost key in localStorage.
2. Because comment forms were cross-wired with each other and no forms had unique IDs. The editor could be listening to the wrong form for input changes. Or when the user pressed the Recover button, maybe the recovered text went into the wrong comment form.

Now that cross-wiring seems to largely be a thing of the past 🤞, the recover button DOES have one vital use that I can think of... And it DOES work for this, according to the system test that I wrote yesterday:

• If the user accidentally refreshes the page, if their browser crashes, etc... Anything that would make them load the same page again... They can scroll down to the comment form (or forms now!) where they were last typing, hit the recover button, and the text will be restored as it was at the moment of browser crash!

Otherwise the save and recover buttons don't have the functionality to:

• save a version of the comment form that WON'T be overwritten by later user input 🚫
• save multiple versions of a comment form 🚫

[noi5e]

Discussion Point 2: What Do We Signal to User?

Given all of the above... I think it's worth talking about as a community how we want save and recover to behave, and how much of that behavior we signal to the user.

If the save button is useless, maybe we can just delete it?

Maybe it could be replaced by flashing saving as a notification, either in the comment, or via Noty Notification:

This would be debounced and only kick in every 2-3 seconds. This would have the positive benefit of adding more functionality to the save & recover buttons. Think if you were a user, and you made a mistake and wanted to Ctrl-Z. recover would have that ability if save were debounced a bit.

Or do we make everything invisible?

Maybe we go the other route. Think of how a site like facebook operates. If you type something into a DM, and the page crashes. You reload the page and the text you were just typing appears like magic there.

Automatically doing this without user input does have a certain kind of appeal. We remove buttons that don't have much function at all. Or, in the case of recover, that a user might rarely think to use.

I think I want to focus my energy elsewhere for the remainder of the internship, but can revisit this if time allows. Just posing these questions for discussion if I don't get to it!

[noi5e]

Side note: I noticed that localStorage behaved inconsistently while I was making these revisions to save & recover.

Ideal: Visiting /, Then Navigating to Node

1. Visit the homepage at localhost:3000
2. Navigate to localhost:3000/questions/path/to/question
3. Scripts, when saving to localStorage save to localhost:3000 as expected
4. Everything works as expected!

Buggy: Navigating to Node Directly

1. Visit (or refresh) the URL localhost:3000/questions/path/to/question
2. Scripts, when saving to localStorage, save to about:blank instead of localhost:3000
3. Scripts aren't able to access localStorage variables, since they expect those variables to be in localhost:3000
4. Save & Recover doesn't work as expected.

From this StackOverflow question:

localStorage is stored for specific document origin - https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage. Therefore you need to visit a URL for a specific origin before you can execute JS to store anything in the local storage (otherwise you'd be attempting to set local storage associated with about:blank which doesn't make sense)

Before('@no-newsletter') do
  page.visit('/something') # visit a valid location to set localStorage
  page.execute_script "window.localStorage.setItem('subscribed','viewed');"
end

I don't know enough about Rails, there may be something about it that is causing this behavior... Or it may be a fluke and have something to do with how I was working in development with localStorage that was causing it.

Just making a note of it in case it comes up in the future!

[jywarren]

Hmm. one thing I thought of wrt the localstorage issue is that it can sometimes happen that we try to execute JS before a page is finished loading, and in test environments perhaps this is harder to notice? But it's just a guess.

This is to distinguish between text written in main on one page vs another. This avoids the bug of the user hitting recover and recovering text from a different page.

I like the solution you did with unique IDs. But noting that possibly this could prevent recovery on a page which no longer exists, or a comment which no longer exists? But still, recovery is an "as good as we can do" kind of thing so perhaps it's OK.

I guess I agree about Save being useless -- i think we started saving automatically after we added the basic function.

How about this -- the save button could become a non-clickable disabled button, and could be used primarily to signal when saving is occurring, with a fa fa-spinner fa-spin class? OR, if you do click on it, it just explains the limitations of the saving system, in a popup, rather than doing anything. Because the system's opacity is kind of a problem and kind of a feature, as you point out. We've never really wanted to explain all the caveats and "whys" of the system because it's a "just in case" kind of thing. And we're even trying to balance user privacy here, by not saving on the server side! But, maybe some basic explanation is worthwhile, especially now that it has per-form saving capabilities.

Thanks for so carefully thinking through this!!!

[noi5e]

Made a more contributor-friendly breakdown of this issue in #9362, so closing this one.