Thanks for raising this.

This is because when live preview isn't in use the whole page reloads, and so the 'initial' state of the form includes the unsaved modifications, so when we restore back (to what is in most cases the unchanged state) it is wrong.

One fix could be to force the use of live preview for Edit Recovery's 'Show changes' button, and ignore the user's preference. That would make the preview quicker and fix this bug, but perhaps isn't what experienced users would expect.

An other fix could be to reload the whole page when discarding. This might be the most robust because there are gadgets etc. that will do things to forms' content on first load, but not at subsequent points (although Edit Recovery is aiming to provide sufficient JS hooks to make it possible for those gadgets and script to react appropriately, it may be easier to not have to).

And a third could be to make an API call when discarding to get the unmodified state of the form. This would be possible for the main wikitext, but could be more complicated for other fields (e.g. how do we know what to reset the edit comment to?).

Maybe I'm doing something wrong, but the "Discard changes" doesn't appear to work at all in my testing, both on my local and on Beta. No errors in the JS console either.

Repro steps:

• Open a new page for editing (I just did this to ensure there wasn't a conflict with old Edit Recovery data)
• Write something and save
• Edit again, adding more content but don't save
• Close browser tab
• Re-open, click "Discard changes"
• The content remains unchanged, and I can click "Show changes" and observe that the recovered changes are still present

This was tested both with Live Preview on and off.

Disregard my issue, it was T353616: "Discard changes" does not discard changes for CodeEditor or CodeMirror. Apologies for the noise.

Change 988747 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] Edit Recovery: Force live diff view

https://gerrit.wikimedia.org/r/988747

Change 988747 abandoned by Samwilson:

[mediawiki/core@master] Edit Recovery: Force live diff view

Reason:

This is not a good way to do this.

https://gerrit.wikimedia.org/r/988747

Is this issue still around? I'm not able to replicate it :)

In T354494#9552224, @HMonroy wrote:

Is this issue still around? I'm not able to replicate it :)

Yesterday, Edit Recovery seemed to be out of service on Beta Simple Wiki, but I’m just reproduced the issue with provided steps.
You should also ensure “Show preview without reloading the page” is not checked in your preferences (Editing tab).

Yeah this is only when non-live preview is used.

I think the approach that I mentioned above is probably the easiest and most robust: when clicking Discard changes, to delete the stored data (as is currently done) and then reload the whole page (currently it just leaves you were you are). This would mean that the preview disappears too which is probably a good thing as it'd no longer be correct. This could be done for both live and non-live preview, and although it means a sort-of unnecessary page load in the case of live preview I think it might still actually be better because it'd return the form to the initial state.

What do you think?

This could be done for both live and non-live preview, and although it means a sort-of unnecessary page load in the case of live preview I think it might still actually be better because it'd return the form to the initial state.

If we can’t force Live preview, I think we should try to keep such “live discarding” when Live preview is enabled: the UI is much more user-friendly than a page reload. I don’t know how complex it is to guess whether Live preview is enabled, though.

a sort-of unnecessary page load in the case of live preview

I would find that disorienting. Now that live preview has an API (require( 'mediawiki.page.preview' )), why not let "Show changes" in the notification do an ad hoc live diff, without touching "Show changes" in the edit form?

Able to replicate it :) This issue is also there when the user makes an edit followed by "Show Preview" or "Show Changes", and then "Discard Changes".

Depending on how you try to "Discard Changes" there might be different elements in the UI that we need to remove after discarding changes.

I think we should follow a similar approach as when "Cancel" button is clicked. The page is loaded to the article page, but in our case, I'm thinking that we should reload to the edit page so that the user can go back to the initial form.

Thoughts?

Change 1006095 had a related patch set uploaded (by HMonroy; author: HMonroy):

[mediawiki/core@master] Reload page when discarding changes after showing/preview changes

https://gerrit.wikimedia.org/r/1006095

"Discard changes" would bring you back to the original wikitext, so it would seem safe to just hide the preview areas entirely, no?

Alternatively, T354494#9566557 sounds like a good approach. It should be easy enough to leverage the preview module, essentially "forcing" use of live preview. This is no different than using Realtime Preview, in the sense it still uses live preview regardless of the user preference. "Show changes" doesn't update things like indicators and other parts of the UI, so it should be safe to call doPreview() with the showDiff option set, and not have to worry about what UI changes are needed when clicking "Discard changes".

I think it should be possible to determine when Live Preview is in use or not, and act accordingly. It would be pretty annoying to get a page reload every time you want to discard (although maybe it would encourage people to switch to live preview!).

The change I abandoned above forces live preview, if that's the way we want to go. It should probably force live preview for all previews though, otherwise it's still possible to end up doing a non-live preview with the normal preview button and for edit recovery to then not be able to discard changes.

"Discard changes" would bring you back to the original wikitext, so it would seem safe to just hide the preview areas entirely, no?

Not for non-live preview, because the 'original text' in that case doesn't match the edit's parent revision.

I think if @HMonroy's patch above also checks for live preview (as well as wgAction=='submit') then all cases should be covered.

By using mediawiki.page.preview but NOT mediawiki.action.edit.preview you can do a live diff just for once. That's what I was suggesting, and it's different from the patch you abandoned.

That's true, I was glossing over that because the main thing I think is that we don't want to force a live diff on people unless they've opted in to doing so. Live diff people will get it already, and non-live people should get a page reload (back to action=edit).

I wouldn't characterize it as "forcing" a live diff because the popup is already a JS component and "Show changes" in the edit form would remain unaffected. Rather, it would be more of a surprise if a button on an interactive popup sent you to a different page. A one-time live diff is the less astonishing approach.

But regardless of what "Show changes" does, it seems to me we need to figure out how to make "Discard changes" always work. The pending patch strikes me as inadequate as it'll remove parameters like section and preload. And just because the URL has action=submit doesn't mean the page was opened in POST (it might have been via a bookmark, browser history, or a shared link), and it'll likely be moot after whatever you do to address T354186: Don't show the edit recovery popup on non-live preview, edit summary prompt, etc..

Live diff people will get it already

Note just because the user has live preview enabled in preferences doesn't mean they never see a diff in POST. They might have clicked "Show differences" too early (before the handler is attached), or they might have enabled it between the time they started editing and the time they previewed the diff. So whether to reload the page shouldn't depend on whether live preview is on.

Retrieving the source asynchronously seems to be the only surefire way. And probably the best. Nothing in the popup should reload the page or send you to another IMHO.

By using mediawiki.page.preview but NOT mediawiki.action.edit.preview you can do a live diff just for once. That's what I was suggesting, and it's different from the patch you abandoned.

I wouldn't characterize it as "forcing" a live diff because the popup is already a JS component and "Show changes" in the edit form would remain unaffected. Rather, it would be more of a surprise if a button on an interactive popup sent you to a different page. A one-time live diff is the less astonishing approach.

This was my thinking as well. We abstracted out the preview logic into its own module for Realtime Preview (where we also intentionally ignore the live preview preference), but it was always the intention that it could be used for other purposes. This seems like a fitting use-case. If we wanted to do a check for if live preview is enabled and conditionally do a live diff or full page refresh based on that, I think that's fine, but adds unnecessary complexity. It also sounds like the full page refresh is causing problems. If "forcing" live preview avoids those problems, I say go for it. The blockers to making live preview on by default (T41272) aren't mostly to do with diffs, and those that are don't seem to be exasperated by Edit Recovery.

Change 1006095 abandoned by HMonroy:

[mediawiki/core@master] Reload page when discarding changes after showing/preview changes

Reason:

A different approach needs to be implemented as described on T354494

https://gerrit.wikimedia.org/r/1006095

Change #988747 restored by Samwilson:

[mediawiki/core@master] Edit Recovery: Force live diff view

https://gerrit.wikimedia.org/r/988747

Okay, so it sounds like we can enable live diff for the show-changes button in the ER notification, and that does solve the bug. My point above was about also enabling it for the normal preview button, regardless of whether the user has enabled live preview; that's the unexpected bit, perhaps. If we enable it only for the notification button, then we still have the problem of what to do for the normal button when non-live preview is enabled.

It's correct that there are other cases of the page being reloaded, but they're far less common; let's fix for the majority of cases first, which seem to be related to previewing & diffs.

If we do want live preview to happen all the time while Edit Recovery is enabled, then could it perhaps make sense to just tie the two preferences together? i.e. if ER is enabled, live preview is enabled too? This would be simpler than adding the same functionality to the ER code I think, and would be more obvious to the user (we'd add something to the preference description that explains things). Or am I just being silly?

If we enable it only for the notification button, then we still have the problem of what to do for the normal button when non-live preview is enabled.

I don't understand how this is a problem at all. We can just leave the normal preview/diff buttons alone (i.e. no handler attached).

Change #988747 merged by jenkins-bot:

[mediawiki/core@master] Edit Recovery: use live diff view

https://gerrit.wikimedia.org/r/988747

@Samwilson If you click 'Show Changes' in the notification and then discard changes, as you see in the 1st video, the changes are now discarded. In the 2nd video, you can see that the live diff is now automatically on when edit recovery is on, even though it's supposed to be off. This will be now moved to Done. Thanks for all your work!

Regarding https://phabricator.wikimedia.org/T354494#9573630, I confirmed with Sam that 'Show Preview' or 'Show Changes' at the bottom is not an issue but as designed. The idea is that when you're previewing or showing changes, you know what you're doing, and the contents are all what you're currently working with.

Status: ✅PASS
Environment: Beta: 1.43.0-alpha (cb7daae)
OS: macOS Sonoma 14.4.1
Browser: Chrome 123, Firefox 123, Safari 17.3
Skins. Vector 2022, Vector 2010, Minerva, Monobook, Timeless
Device: MBA M2
Emulated Device:: n/a
Test Links:
https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Lightsaber&action=edit

✅AC1: https://phabricator.wikimedia.org/T354494

Vector 2022	Vector 2010	Minerva	Monobook	Timeless

Chrome	Firefox	Safari