RFC 99: Preliminary work to support auto-save functionality

[gasman]

Rendered

Abstract

A much-requested feature for Wagtail is the ability to automatically save changes in the background while editing a page or snippet. Achieving this to an acceptable standard of user experience will involve completing various milestones that are best handled as separate development efforts. This RFC outlines various items of work that are pre-requisites for implementing auto-save functionality.

[laymonage]

This makes sense to me, thanks!

I think the only thing I'm wondering is how we decide when to use Telepath and when to use Stimulus controllers instead. From what I understand Telepath seems more suited to when you want JS classes that map directly to a class in the server-side code, while the way we use Stimulus is more suited to generic reusable behaviours.

There are cases where our existing Stimulus controllers may be useful for this, in particular for replacing DOM fragments based on the server response. I've been using SwapController and TeleportController for this purpose in universal listings and the editing sessions.

That being said, I wonder I should've used Telepath instead of Stimulus for some of the stuff I've been doing. I chose Stimulus because I'm less familiar with Telepath at this point, but I'm happy to change that if Telepath suits better.

[thibaudcolas]

I like skipping the enforcement of validation rules to improve the user experience, but am concerned it’ll break developers’ expectations too much. I think this needs further exploration to check whether we can get the best of both worlds, or if not, so we have more information to decide on that kind of trade-off.

[thibaudcolas]

Suggested change

	When multiple users have the editing interface for a page or snippet open at the same time, the last user to hit save will generally overwrite any changes made by the other users. This problem is compounded when auto-saving is active, since this overwriting will happen without the user's explicit action. To address this, we will implement a notification system that warns users when another user is editing the same item, and disables auto-saving when potentially conflicting changes are detected. This feature is covered by RFC 95.
	When multiple users have the editing interface for a page or snippet open at the same time, the last user to hit save will generally overwrite any changes made by the other users. This problem is compounded when auto-saving is active, since this overwriting will happen without the user's explicit action. To address this, we implemented a notification system that warns users when another user is editing the same item. This feature is covered by RFC 95.
	This notification system will need to be extended to disable auto-saving when potentially conflicting changes are detected.

Rewording to make it clear what was covered in RFC 95. I’m concerned about disabling auto-saving, but need to think more about its alternatives.

[thibaudcolas]

I’m worried that we’ll break developers’ expectations by making the nuance between DB-level and ORM-level validation more apparent without it being something that devs knowingly opt into. Could we consider alternatives? Perhaps adding an explicit way to distinguish between "valid" and "invalid" drafts?

[thibaudcolas]

Re my above comment about developers’ expectations, I’d like to see alternatives explored, and going into more depth on the types / commonness of preview errors that this change would cause. From personal experience, it seems pretty common already that devs implement functionality without being aware of compatibility issues with previews.

[thibaudcolas]

It’d be nice to also cover the criteria when those error messages would be displayed – assuming we’d fields / Telepath to keep track of whether fields are "touched" to decide whether to display the error messages or not.

[lb-]

Would there be any way for us to instead treat autosaves as a different kind of revision?

Revisions, because they kind of act as a proxy, could be treated differently to actual model saving. Bypassing some validation for the sake of convenience.

From here, we would need to solve the UX problem of 'your work is saved but not fully saved'.

Confluence does a good job of this, it's pretty intuitive, treating saves similar to what we treat publish as. When you see the page listed it says 'unpublished changes'. You edit and then click save or publish to apply your 'work in progress '.

So our autosave is less about saving an actual version but allowing for a work in progress, per user even, that allows you to come back later and actually save your work.

Confluence won't trigger any workflows or other 'proper' validation until you've actually saved things. Confluence also won't show your 'temporary' work in the proper revision listing.

I really like it's approach, it let's me work on something but not share with my team until I'm ready.

I also think the same 'temporary' revision mechanism is used for their pseudo-concurrent editing.

Just an idea.

[lb-]

note: I put a similar question/idea on the newer RFC https://github.com/wagtail/rfcs/pull/104/files#r1865306971

[thibaudcolas]

Doesn’t have to be open questions, just things I’d want the RFC to cover.

Suggested change

	## Open Questions
	## Open Questions
	### How do revisions work in the auto-save model?
	### What’s the support goal for auto-saving snippets?
	### What’s the support goal for auto-saving arbitrary Django models?
	### What’s the support goal for fields that don’t have ready-made Telepath adapters?
	(not clear to me how common that is nor how much work it would be to make them compatible).

[balazs-endresz]

I think it would be also important to let users disable auto-save by default or for a specific page. I'd definitely do that when working on a site locally. And in production at least on certain sites if there might be PII or secret keys in text pasted in that I wouldn't want to be accidentally saved in a database. Perhaps even developers would want to completely disable it for everyone to reduce server load, or to avoid frequent external API calls.

[thibaudcolas]

Here are additional topics we discussed as likely requiring an RFC-level proposal: wagtail/wagtail#7636 (comment)

Not sure which would be worth updating this RFC for, or if it’s better to be iterative with this too.

[lb-]

In light of other discussions at the core team meeting, just flagging that this proposed approach will have larger impacts on our planned Stimulus migration path. With the underlying impacts to removal of inline scripts and jQuery.

I really think that we should ensure we've fully validated a DOM / FormData based approach before going down this path.

Having Telepath baked in, for a core concern potentially, for any other field and custom admin DOM implementation is a risk we need to think about. We'll also need to deal with JS data structures that may be at odds with the DOM/FormData.

Finally, in terms of error patching into the DOM. I really think if we POSTed a submit, changes the request header to accept JSON, our save method could return a errors structure mostly out of the box using Django clean methods. From here we have an object of named (field names associated in our DOM) errors, as strings or html. This can be patched into the DOM using setCustomValidity on the field, via its name.

Then we can use the existing system of JS form /field constraint validation for as much enhanced UI around message presentation as needed.

https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation#the_constraint_validation_api

This also means the 'ownership' of what to do when there's a field error can be split out to a different controller. Or at the PanelController level.

It's worth a POC if possible.

[lb-]

@gasman @thibaudcolas @laymonage - here's a very basic POC branch that builds our existing Stimulus approaches to bake in a simple (ignorant) auto-save behaviour.

lb-/wagtail@54a4c19

Observe the error from 'async save' based on the JSON response, with updated tab/minimap counts

POC implementation

1. UnsavedController -> when it's determined that there are changes in the client that are different, do an async POST with the form's FormData. Add accepts JSON as a header to this request.
2. In our Page save POST handler, process this roughly as a normal save, except when we get the accepts JSON, in which case we return the errors as JSON and also flush the session messages.
3. Back in UnsavedController, we map through fields and mark any matching fields as invalid, this is 100% a browser DOM API. setCustomValidity & reportValidity
4. Using our CloneController we listen for the browser invalid event which gets fired when custom validity is called.
5. Our CloneController captures the event (important, we use capture:true as invalid does not bubble by default, we also use preventDefault to NOT show the browser native popover as we have our own, better & more accessible approach). And creates a new cloned message from the data on the field, which is available at input.validationMessage.
6. We make sure that the Tabs error count gets updated by listening to any message adding/clearing - supported out of the box today with the CountController and event listeners.
7. We make sure the minimap gets updated by listening to the same events.

What we get

1. When the user makes ANY changes to the form, after the initial check delay, if the values in the form are different to the initial load, we will save the changes to the Page model.
2. If the save is successful, great. Otherwise we do not save it, but we do update fields with any messaging.
3. When errors come into the DOM, the minimap/tabs count also get updated.
4. Rather than reinventing 'code that checks for edits across all the possible universe of form changes', we just build on the existing approach in UnsavedController
5. Almost no major new JavaScript required, building on existing controllers that do smaller dedicated jobs (a. do stuff when there are unsaved changes, b. put a message element into the DOM when there's a specific event, c. update error counts when there is a specific event fired).

What's not covered

• A lot obviously, this would not be production ready, however...
• We would probably want to update the unsaved messages / sync up somehow
• We probably need to consider if we actually want the errors to show and the best practice here
• JS input elements only support a single string, no HTML or data structures for errors, we could be fine with this and it's probably a safer way to go, but we would need to do some work to get HTML error messaging working this way if we wanted. A better approach would be to clean the messages on the server and just send down simpler messages. Leave the 'HTML' errors for actual saves maybe.
• Robust code with tests.
• Nuances of FormData submission, e.g. we may NOT want to submit comments or other values like next to the server when async auto-saving, we may NOT want to send files either. This comes down to UX and expectations of the autosave functionality.
• We really should simplify how our different aspects of code 'look' for errors on the screen, but that's probably not critical for auto-save.
• Proper handling of the server-side aspect of auto-saving, a lot depends on the implementation goals but at the moment this POC will trigger all the workflow, hooks and everything else as if it's a full save.
• Nuances of radio/checkboxes with name being used across multiple inputs. In fact there's probably a bug right now in our UnsavedController for this, I will raise when I get the chance.
• Error messages that need to show 'within' inputs (e.g. mapped to parts within rich text or StreamField), due to these not really being discrete inputs. I understand this is a huge chunk of work but it's possible to build on the above to add this, even the StreamField controller could listen to the invalid event, stop propagation and do it's own parsing of the validityMessage or similar.

It's worth calling out that the more we build on our own constructs for message, DOM representation and other things, the more it forces us away from building on the browser and it's capabilities.

Sometimes it's a bit of a fight to get the most out of the browser, but we do get so much 'for free' and we also get a pretty functional out of the box behaviour from 'just a Django widget' that I think is a worthy goal when it comes to developers using and building on Wagtail.

I like to think that we should give the power of the complexity/functionality of StreamField when we/devs need it, but without having to force all our HTML/JS to follow that paradigm up the whole DOM when it's probably not required. We want our JS/DOM to 'wear the right hat' if possible.

Rant over, I hope this is helpful and is a good discussion starter.