The Problem

If you start entering information into a Drupal form, accidentally click a link, then use the back button to go back to the form, ckeditor4 input is lost.

Proposed solution

Remaining tasks

The behaviour needs to be tested on more browsers, and on ckeditor5.

Original report

Here is an easy way to lose data, and its extremely frustrating and is reproduced frequently among my users and occasionally by myself.

Create a new page, type a paragraph, hit preview. Type several more paragraphs, now click a link or hit (in firefox) alt+home. (this is normally done on accident by the user, but can also be done by hitting the "formatting tips" link if they want help)

The obvious consequence is that the user leaves the page. They hit back, to see their work, but because they hit the preview button earlier, when they hit back, drupal has to resubmit the form (as it was before adding the additional paragraphs) causing them to lose all the work they recently did.

Note that under normal circumstances, if the preview button is not used, this data loss doesn't occur. They hit the back button and everything is fine.

Possible solutions:
* confirm(nag) - warn them that they have unsaved changes, and are they sure they want to leave the page? (blogger, gmail and many other progs do this) - still a hack, but is better than losing data.
* cookie - every 10th key press or something, save the contents of the form to a cook - yuck, but better than losing data.
* save data persistently, either through an automated save draft (gmail does this) and/or a "save and continue editing" button (wordpress does this) - ideal
* redirect after post - that way user's can still use their back button in the normal way - ideal

CommentFileSizeAuthor
#118 form_autosave_garlic-153313-118.patch33.28 KBWim Leers
#118 interdiff.txt2.77 KBWim Leers
#112 form_autosave_garlic-153313-112.patch31.21 KBWim Leers
#112 interdiff.txt16.76 KBWim Leers
#107 drupal-form-data-back-153313-107.patch29.92 KBWim Leers
#107 interdiff.txt11.57 KBWim Leers
#102 drupal-form-data-back-153313-102.patch22.56 KBjessebeach
#102 interdiff_99-to-102.txt4.09 KBjessebeach
#99 drupal-form-data-back-153313-99.patch22.05 KBWim Leers
#99 interdiff.txt9.31 KBWim Leers
#97 drupal-form-data-back-153313-97.patch21.19 KBWim Leers
#95 drupal-form-data-back-153313-95.patch20.71 KBPete B
#92 drupal-153313-form-data-back-92.patch21.19 KBWim Leers
#90 drupal-153313-form-data-back-90.patch21.78 KBWim Leers
#90 interdiff.txt3.98 KBWim Leers
#87 drupal-153313-form-data-back-87.patch21.09 KBLewisNyman
#79 drupal-153313-form-data-back-button-6740346.patch11.94 KBjohnennew
#77 drupal-153313-form-data-back-button-77.patch11.65 KBbarraponto
#69 drupal-form_value_to_localstorage-153313-6644046.patch11.59 KBjohnennew
#67 drupal-form_value_to_localstorage-153313-6604388.patch11.4 KB__cj
#60 form_autosave_screenshot.png28.43 KBjohnennew
#56 drupal-form_values_to_localstorage-153313-6547066.patch11.61 KBjohnennew
#55 drupal-form_values_to_localstorage-153313-6547066.patch11.65 KBjohnennew
#54 drupal-form_values_to_localstorage-153313-6545538.patch10.05 KBjohnennew
#52 drupal-form_values_to_localstorage-153313-6544478.patch9.61 KBjohnennew
#50 drupal-form_values_to_localstorage-153313-6526240.patch8.19 KBjohnennew
#48 drupal-form_values_to_localstorage-153313-6526022.patch5.32 KBjohnennew
#46 drupal-form_values_to_localstorage-153313-6396266.patch5.32 KBjohnennew
#30 cachecontrol.patch2.99 KBcasey
#27 cachecontrol.patch1.74 KBcasey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

I like the save and continue editing button idea. I'm forwarding this post to the user experience group.

Sutharsan’s picture

Project: » Drupal core
Version: » 7.x-dev

Moving issues from User experience project to Drupal core usability component.

David_Rothstein’s picture

Title: how to lose data easily » data loss when navigating away from the "create content" page
Component: usability » javascript
Priority: Normal » Critical
Issue tags: +Usability

I tried reproducing this in Drupal 5, 6, and 7 (using Firefox 3 on Linux). In my experience, the preview button had nothing to do with it -- I was able to experience the problem both with and without hitting the preview button. Here's what I found:

  1. In Drupal 5, it worked fine no matter what I did.
  2. In Drupal 6, any time I navigated away from the page and then hit the back button, the previous text I entered was gone. It also sometimes seems to do something funny with my input format setting (changing the radio button to something other than what I selected). However, if I do it with Javascript turned off, everything works fine.
  3. Drupal 7 seems to work about the same as Drupal 6.

This suggests that it could be a bug in the javascript that was introduced in Drupal 6. Although the behavior could very well be different in different browsers.

Carl Johan’s picture

Some browsers will save your halfway-filled-out form for you if you navigate away from it, and then fill it in when you navigate back to the form page.

I'm not sure what the circumstances are to trigger this behaviour in browsers, but I'd say it's more worthwhile to try and create a browser (and form?) independent way of doing this.

David_Rothstein’s picture

Note related issue: #655388: Many ways to lose data on form input in the overlay

(It's actually the same issue in many ways, but the overlay is a special case so it might have some problems along these lines that are unique to it. The data loss bug occurs with or without the overlay, though.)

Alex UA’s picture

Subscribing- I've definitely lost a few hours of work due to this bug.

brianV’s picture

Perhaps we need to implement a system where me make better use of the 'Unpublished' state.

1. As long as the post has a title, save it as 'Unpublished' every X seconds.
2. Before previewing, save it as well.
3. Don't publish until user hits 'Publish' button.

If the user is updating, do the above three steps, but save into a new unpublished revision, which becomes published in turn. Perhaps 'Publish' button is changed to 'Publish changes'.

tumblingmug’s picture

As David_Rothstein said: the back button functionality got lost if JS is enabled; so it should be possible to catch the data loss by an JS alert, that warns about consequences leaving the page and offers a cancel button. So both cases (with and without JS) could provide a relative sure editing then. The way brianV suggests, lets things appear very close to WordPress on one hand; on the other hand this would cost performance for sure - if you think of an active community site with many people editing at the same time. WordPress' usabilty has to pay for it and it does. But the goals are quite different, even if usability is an issue of both systems.

chx’s picture

Priority: Critical » Normal

This is not critical. Fix your browser first to support http://ejohn.org/blog/dom-storage/ this. Second, nothing ever stopped writing a contrib mdoule that throws up an alert onbeforeunload (and then Opera does not support that but then Opera has excellent back).

Alex UA’s picture

Priority: Normal » Critical

chx- your comment is not really relevant, and telling users to update to browser X, especially when we know that Drupal's JS is responsible for the problem isn't going to fly when that user loses an hour of work.

Moving back to critical, please ignore this chx if you don't like the issue, but please respect that people other than you regard this as critical.

xmacinfo’s picture

I have to agree that this is not a critical issue. :-)

However I agree that we need a fix for this. A "Dirty form" JavaScript function was added to the overlay module and removed later -> #517688: Initial D7UX admin overlay. I guess the warning fired up way to frequently to users taste.

We should have a tone down version which would fire up only for various content type creation form.

If we want to auto save data, we must also provide a way to delete these "draft" content, but that would make it only in D8.

Dries’s picture

Priority: Critical » Normal

I agree that this is very annoying, but at the end of the day, it is not critical. Critical bugs stop Drupal from working.

mikeker’s picture

In case people stumble onto this thread looking for a D6 solution...

The advantage of the "nag" option is that no data will be lost (eg: Draft only auto-saves every 30 seconds by default and there are bandwidth issues if you make it something like 3 seconds). But Draft gives you a "Save as Draft" button which is easier to explain to clients than "first click on Publishing options and then uncheck Published."

No reason you can't have both "nag" and auto-save options running at the same time, I suppose...

- Mike

ksenzee’s picture

Subscribing. If I get the time, I'll investigate how exactly the node form Javascript could be breaking the back button. Something odd is going on there.

casey’s picture

Anonymous’s picture

Subscribe

David_Rothstein’s picture

Version: 7.x-dev » 6.x-dev

I can't reproduce this anymore in Drupal 7 (except when the Overlay is enabled, but that has its own issue at #655388: Many ways to lose data on form input in the overlay), so I'm moving it back to Drupal 6.

I think this narrows it down to one very likely culprit: the "split summary at cursor" button on the Body field, which currently exists in Drupal 6 but no longer in any other version of Drupal.

David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev

Actually, it looks like it's still reproducible in Drupal 7, but only with Internet Explorer - all other browsers seem to work fine. Aargh...

casey’s picture

Bojhan’s picture

Status: Active » Closed (duplicate)
casey’s picture

Status: Closed (duplicate) » Active
Bojhan’s picture

@casey please, add some information when you do stuff :P jeez

casey’s picture

Sorry :)

The other one is just about the overlay and only contains a fix for forms inside the overlay.

The real issue is much broader (and causes issues like #358961: checkboxes lose state if action requires a confirmation screen and user cancels or uses back button). According to the link I posted it may be caused by cache headers. I'd like to see if we can fix that.

Bojhan’s picture

But its not really only related to the create content page then?

casey’s picture

Title: data loss when navigating away from the "create content" page » Losing formdata when using the browser's back button

Not indeed.

David_Rothstein’s picture

According to the link I posted it may be caused by cache headers. I'd like to see if we can fix that.

When the cache headers were previously incorrect it caused much wider problems with forms being emptied all over the place: #109941: Store site view in HTTP headers

I guess it could still be the case that some minor adjustment is needed to what we have now to make IE behave correctly, though.

casey’s picture

Title: data loss when navigating away from the "create content" page » Losing formdata when using the browser's back button
Status: Active » Needs review
FileSize
1.74 KB

This should do.

Note this isn't working inside overlay cause of the way overlay is working. I might be able to fix that however. If so I'll post a patch in the overlay-specific issue.

catch’s picture

Component: javascript » base system
Issue tags: +Performance

Looks interesting, will try out later.

Status: Needs review » Needs work

The last submitted patch, cachecontrol.patch, failed testing.

casey’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Fixing test.

locomo’s picture

subscribe

xmacinfo’s picture

Title: Losing formdata when using the browser's back button » Losing form data when using the browser's back button
Alex UA’s picture

Priority: Normal » Major

I'm bumping this to "major"- we're getting a bunch of complaints about this behavior in 6.x from our clients, which I know is a self-selecting/unscientific polling method, but I have to believe this is causing a lot of pain to Drupal site owners and is really a pretty nasty little bug (borking the browser = bad).

mgifford’s picture

Version: 7.x-dev » 8.x-dev

I'm moving this to D8 so it can be looked at and hopefully backported.

I'm also interested in if there are suggestions to Seven where this can be applied in the interim.

It's a really frustrating problem and hopefully it will be fixed in browsers, but this one bit me today in Chrome. Wasted a bunch of time editing a blog.

casey’s picture

#30: cachecontrol.patch queued for re-testing.

sun’s picture

Priority: Major » Normal
Issue tags: +Needs backport to D7

Indeed interesting. Love the comments. However, this is not major.

mgifford’s picture

My team upgraded this module to D7 and it works great - http://drupal.org/project/node_edit_protection

It would be great if there was something like this as part of core in D8 mind you.

cosmicdreams’s picture

mgifford’s picture

HTML5's local storage? I suppose that might work. Should be better supported by the time D8 is released. I think this is likely much bigger than this particular issue queue item mind you.

geerlingguy’s picture

@mgifford - I'll be testing Node Edit Protection to see how it works for one of my sites (looks to be very similar to Dirty Forms and onBeforeUnload, also mentioned in this issue).

I've been getting these complaints from site users here and there... it's mostly an annoyance, but enough of one that I will be looking for better ways to solve it. local storage would be one nice way (HTML5), but that still won't work well for most people for another 2-4 years, I think.

Subscribe.

mgifford’s picture

Hopefully Node Edit Protection works for you. It's a simpler implementation I think than having to enable Dirty Forms & onBeforeUnload which seems to be the proposed approach.

If folks like the approach used in Node Edit Protection we can look at producing a patch for core.

nod_’s picture

Issue tags: +JavaScript

tag

nod_’s picture

Status: Needs review » Needs work

Probably needs a reroll, just saying :)

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Performance, -JavaScript, -Usability, -Needs backport to D7

#30: cachecontrol.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +JavaScript, +Usability, +Needs backport to D7

The last submitted patch, cachecontrol.patch, failed testing.

johnennew’s picture

Status: Needs work » Needs review
FileSize
5.32 KB

Hi all,

PeteB, ChrisH and I supply the attached patch for review. It's certainly a work in progress but needs some initial comments now. We're using local storage to store form values as they are entered. When the form is revisited the previous values are recalled from localStorage. If the user deliberately closes the overlay with its close button or saves the form then the local store is removed.

This implementation fixes the specific use case described previously, the steps to reproduce are:

1. Click add new article
2. Fill in the form
3. Press Preview
4. Click "Read More" or otherwise leave the overlay without clicking save or close
5. Press back

Previous outcome: the form would be empty

Outcome with this patch applied: Your previous form states are restored.

Bojhan’s picture

@ceng Thanks, awesome that you guys have been working on this.

johnennew’s picture

Heh, already found it doesn't work in Chrome, attached is a better version.

nod_’s picture

Status: Needs review » Needs work

That's a pretty good start :)

It'd be good to read up on http://drupal.org/node/172169

  • we use lowerCamelCase variable names, no "_"
  • Always put a ";" at the end of line/expression
  • Strict equals

As for the patch itself, a new file like form.autosave.js would be better than stuffing it in form.js.
you can use document.querySelectorAll instead of getElementsById, getElementsByClassName
Drupal form elements will always have an ID (or they should at least) you can based you assumptions on it, the serialized object can be a simple {id: value, id2: value2} kind of object (we want to keep the data as light as possible for storing in localstorage).

Just use jQuery for updateForm. No need to spend time to work around at this point :)

Don't hesitate to create new events that will be listen for to clear the localStorage. It's much better than hardcoding the click thing on relevant DOM elements (what about keyboards right now?)

So yeah, it's a good start! thanks!

johnennew’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

Hi all,

Still some work in progress but would be good to get a review at this stage. I've followed nod_ suggestions in #49

* Applied coding standards
* Moved the form save javascript to form.autosave.js
* Added code to the system module to load this js when forms are used
* Added code to the node module to place a class on a form which want to save form state immediately when loaded, for example when previewing or after a form validation error (still to do) as in these cases the users form state has not been saved on the server.
* Added an event called drupalOverlayCloseByLink which is fired when the overlay is deliberately closed. We should remove the localStorage version at this point
* Removed the timed saving of the form to localStorage and replaced with saving on a formUpdate event.
* Replaced some of the items with jQuery instead as suggested.

Do all instances of getElementById need replacing with querySelectorAll?

mgifford’s picture

I got this on a fresh install:

$ git apply drupal-form_values_to_localstorage-153313-6526240.patch
drupal-form_values_to_localstorage-153313-6526240.patch:216: trailing whitespace.
      $form['#attributes']['class'][] = 'form-save-to-localstorage-on-load'; 
error: core/misc/form.autosave.js: already exists in working directory

I think that this form.autosave.js is not stopping me from loosing my data if I navigate away from the page in FF on Linux. Haven't done more testing.

johnennew’s picture

Thanks for the review mgifford!

I've not got a local linux install but I'll setup a Virtual Box with CentOS to give it a go.

I fixed the whitespace issue in the patch but I don't understand the error 'error: core/misc/form.autosave.js: already exists in working directory' - it is this patch that creates this file so there is no way it would be there already. I tested the attached patch on a fresh d8 install and I don't get either error now.

mgifford - when you say you don't think this patch solves the issue, could you describe the steps you take, what you see and what you expect to see (or reference comment above you think is not being achieved); as far as the workflow in #46 goes, I believe this is solved with this patch. Are there any JS errors in your console?

This new patch adds some additional logic to not load the localstorage version of the form if the form's changed timestamp has changed since the local storage copy was made. This means that if someone else updates the form to the server, your local changes will be lost next time to arrive and you'll see the current latest version of the form. I think this makes sense.

mgifford’s picture

Don't worry about the form.autosave.js... I'm thinking that may have just been there from a previous install. After deleting the file, then resetting the repo it applied without difficulty.

johnennew’s picture

Another patch update, added support for select elements. Also removed a floating console.log in the last patch (doh!)

Still todo:

  • Save and restore radio button values
  • Trigger a form save to local storage after a validation error
  • Add some functionality where forms declare they want to make use of localStorage autosave
johnennew’s picture

johnennew’s picture

Hi all,

I think this patch now does everything required of it but needs a close review of coding standards and approach. I added a Array.indexOf prototype (see below) which might well be done in a more elegant way:

if (!Array.prototype.indexOf) {
  Array.prototype.indexOf = function(obj, start) {
    for (var i = (start || 0), j = this.length; i < j; i++) {
      if (this[i] === obj) { return i; }
    }
    return -1;
  };
}

Also, there's some suggestion of enabling the autosave on a form in a better way than setting a class on the form, perhaps through some formapi method.

Kind regards,

John

Angry Dan’s picture

Hi ceng,

Rather than adding stuff to the prototypes of JavaScript objects (John Resig once said this was bad, can't find the post now but he definitely said it somewhere) it would probably be a better practice to define a function for indexOf(). But, since jQuery already has one, you might just want to replace your calls to Array.indexOf with $.inArray(). See http://api.jquery.com/jQuery.inArray/ for documentation on that.

Also and I don't fully understand this yet, so @_nod might be able to help out here, but would this be better implemented using the Drupal.settings object? I'm guessing the ideal would be to do something like this to your $form array:

$form['#autosave'] = TRUE;

(Here's where I get shaky) I'm guessing that then you would use hook_form_alter() to catch all forms with the autosave property set to true. Then you could drupal_add_js("autosave.js", $settings) where $settings contained an array of form ID's to be autosaved. Then in your drupal.behaviours I believe the form ID's would be passed directly into your behaviour code.

Any thoughts?

nod_’s picture

Great work, that's awesome :)

I do have some things to point out.

  1. We have in jquery.form.js a $('form').formToArray() method, using it would turn Drupal.form.localSave in a 2 line function. Some data structure changes but nothing too crazy.
  2. I'd use data- attributes instead of classes to tell JS if the form is auto save or not.
  3. The formUpdataed event needs some love too.

Sorry it's just a short review, it's very much on the right tracks, keep it up :)

nod_’s picture

Status: Needs review » Needs work

There are still issues with fields in vertical tabs, doesn't save everything.

There should be a way to discard localstorage values and go back to the default values. I guess it's the revival of the reset button :p Also we might want to use sessionStorage instead. This is the backbutton issue after all, not the 'everything crashes' one.

johnennew’s picture

FileSize
28.43 KB

Hi nod_

Not sure I understand exactly what you mean in #59.

Vertical tabs: I'm on chrome and ff on a mac and can't replicate this issue, what field values are not saved or restored from a vertical tab? I tried all the basics on a node edit form - sticky at top of lists, URL path, comments settings, create new revision, log entry etc. Clicking off the form then pressing back appears to restore the values I entered here fine.

Reset button: You should see a "Clear local edits" button in the message that appears at the top of the form when local edits have been applied, this button disappears onces clicked and so I think is preferable to the form reset button. (see screen shot attached)

Back button: Using localStorage rather than sessionStorage solves both the crash issue and back button issue. sessionStorage is just back issue. Isn't this approach therefore preferable? I notice that basecamp opts for the localStorage approach for saving comments in a thread so you can go away and come back and your last edit is still there waiting for you.

Will look at implementing the suggestions in #58 next though I am going to spend some time on dan-sprog's suggestion in #57, I wonder of this approach is more in keeping with the rest of formapi, so adding a simple parameter like this includes the JS file?

Bojhan’s picture

What is this local business, no user knows what that means.

johnennew’s picture

Hi Bojhan, thanks for your message. Wording is easy to change, could you suggest something better?

Bojhan’s picture

@ceng I am not sure what exactly it does, so I might be wrong here - but what about just removing local: "You have edited this form before, so it has changes which have not been saved yet"

I don't think the word "edits" is a very clear label, I'd go with changes. For the button a couple options:
[ Reset previous changes]
[ Reset ]
[ Remove earlier changes ]
[ Clear previous changes ]

seutje’s picture

should have some time later today to take this out for a spin. Looks pretty solid already though.

seutje’s picture

+++ b/core/includes/form.incundefined
@@ -874,6 +874,9 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+    else {
+      $form['#attributes']['class'][] = drupal_html_class('form-localstorage-save-on-load');

why are we adding this class and shouldn't it reflect the name of the behavior at least a bit?

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+  attach: function (context, settings) {

we don't seem to be using these parameters, how do we handle ajaxy forms?

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+      var changed = parseInt($(this).find("input[name='changed']").val(), 10);
...
+      if ($(this).hasClass('form-localstorage-save-on-load')) {
...
+      $(this).on('formUpdated', function(e) {
...
+      $(this).on('submit', function() {

could we cache these queries?

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+      $(this).on('formUpdated', function(e) {
...
+      $(document).bind('drupalOverlayCloseByLink', function (event) {

we don't seem to be using these parameters, can we leave them out, or at least use the same naming?

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+  for ( var key in formValues ) {

should probably guard against broken prototypes with hasOwnProperty. And I would be more comfortable not touching the Array prototype and use $.inArray instead, or at least have the polyfill live in its own file.

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+      if (el.checked != formValues[key]) {
...
+  if (typeof(serializedForm) != 'string') {

please don't use soft comparison with type coercion, and is there any need for the inner parens on typeof?

+++ b/core/misc/form.autosave.jsundefined
@@ -0,0 +1,265 @@
+        // We want to physically click this as it might
+        // invoke other JS events such as displaying
+        // previously hidden page furniture.
+        el.click();

this doesn't work, does it? the summaries on vertical tabs don't seem to get updated in my chrome. You probably need to trigger formUpdated on the field or summaryUpdated on the fieldset.

seutje’s picture

hmz, when I click the "clear local edits" button and refresh, the local changes are still reset, but the message also still appears and I can "clear local edits" again. (chrome 22 on OSX, but I don't think it matters)

__cj’s picture

This patch removes the indexOf prototype, and replaces it with $.inArray() (as suggested in #57), caches some queries (as suggested in #65), and replaces 'edits' with 'changes' (as suggested in #63).

mgifford’s picture

Status: Needs work » Needs review

go bot.

johnennew’s picture

This patch builds on __cj's patch. We've removed the use of classes to define that a form should be autosaved and replaced by a new formapi attribute:

$form['#autosave'] = array(

// Set Enabled to true to tell JS to save the form state in localStorage.
'#enabled' => TRUE,

// Set presave to TRUE to save the form values to localStorage as soon
// as the form is loaded. This is useful after a validation error or on presave
// when the form was not saved on the server.
'#presave' => TRUE
);

The message informing the user there are local edits is now:
"You have previous changes which have not been saved yet." and the button text to clear local edits is "Discard previous changes"

Thanks for all the comments, we're still working through them all,

John

yurtboy’s picture

Patch applied fine and the only field that did not save values for me was the Tag field. I typed in a tag that did not exist, click a link on the menu, pressed back and it was gone but all other fields where fine.
I then made a term on the Taxonomy page. Made a new node and referenced this new term. But when I clicked a link in the menu then the back button this was the only field still not saved.

Firefox 16.01

xmacinfo’s picture

@yurtboy: Looks like you can switch back this issue to Needs work. :-)

seutje’s picture

Status: Needs review » Needs work

When adding an article, I also seem to be getting an error on line 172 of form.autosave.js (var type = el.tagName === 'INPUT' ? $(el).attr('type') : el.tagName;) because that el is null when it tries to var el = document.getElementById(key); where key is 'edit-field-image-und-0-alt'

nod_’s picture

I would very much like to have this for D8, meaning we need to be rtbc in a matter a weeks.

@ceng, would you have some time to follow-up on this? I'm happy to give reviews or unblock whatever is needed to make it before feature freeze.

barraponto’s picture

Could not reproduce #70 running Firefox 16.0.2 nor Chrome 23.
Using overlay, btw.

barraponto’s picture

Succesfully reproduced #72, Firefox. Keep in mind you have to preview in order to see the error.

I found another issue: once you come back and the warning message is displayed, uploading a file in a file field widget makes the warning show up again. twice!

johnennew’s picture

Thanks for the reviews, I should have time to look at this on 23/11

barraponto’s picture

So, #72 seems to happen only with filefield widgets, maybe with any field that is not displayed by default. I got around by checking whether el is defined, since I definitely don't think we can save file input values anyway. Gotta see what happens with multiple valued fields though.

johnennew’s picture

Status: Needs work » Needs review

Hi barraponto - tried out your patch and seems to be working as far as I can tell. I think you are right to say you cannot set a file input element with javascript.

I've added a new '#changed' option to the autosave form element. This lets form builders specify the last changed state of the form or entity the form is altering. If someone else edits and saves a node back to the server we want all local copies to be discarded - they have all been over written, the same as would happen if two people edited a node at the same time, only one would end up saving it.

$form['#autosave'] = array(

// Set Enabled to true to tell JS to save the form state in localStorage.
'#enabled' => TRUE,

// Set presave to TRUE to save the form values to localStorage as soon
// as the form is loaded. This is useful after a validation error or on presave
// when the form was not saved on the server.
'#presave' => TRUE,

// Set the last changed time for this form.  If set, this value is saved
// on local systems.  Should this value change on the server, all local
// edits will be discarded since they have been overwritten by a submission
// back to the server.
'#changed' => 1353714364,

);

Ready for further review.

johnennew’s picture

And here is the patch.

Status: Needs review » Needs work
Issue tags: -Performance, -JavaScript, -Usability, -Needs backport to D7

The last submitted patch, drupal-153313-form-data-back-button-6740346.patch, failed testing.

Angry Dan’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +JavaScript, +Usability, +Needs backport to D7
Angry Dan’s picture

Not sure why the last patch failed testing, but it's passed now.

Patch all looks good to me!

barraponto’s picture

Status: Needs review » Needs work

We still need to address the issue i mentioned in #75:

once you come back and the warning message is displayed, uploading a file in a file field widget makes the warning show up again. twice!

LewisNyman’s picture

After a quick chat on IRC with Wim and nod_ I think it's worth suggesting we leverage Garlic.js to persist the local data. It seems well tested, full featured, and with a decent amount of community support. It would give Garlic.js a nice boost is test cases if we threw our weight behind it.

What does everyone else think?

barraponto’s picture

The less I have to mantain, the better.

mgifford’s picture

I hadn't heard of it before, but http://garlicjs.org looks good to me. Wow - "don't lose any precious data if they accidentally close their tab or browser" - that's a big feature enhancement over simply protecting against just hitting the back button.

LewisNyman’s picture

Ok, I've got a proof of concept patch up and running with Garlic.js. Apply and try out on any form!

I've kept form.Autosave.js that requires Garlic.js. At the moment it just calls Garlic against all forms on the page. I couldn't seem to get the Drupal.settings output and that's really not my strong suit, I'm hoping someone can help merge the two patches into a more complete solution.

moshe weitzman’s picture

Title: Losing form data when using the browser's back button » Preserve in-progress form data in localStorage

More ambitious title :)

drupal_add_js($form_autosave_settings, 'setting');. should use #attached['js'] instead.

Lets think about what tests we could add. Might be none.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
3.98 KB
21.78 KB

This reroll is about cleaning up and conforming to latest coding standards (including #88). It now also properly uses drupalSettings (which was not yet done as per #87).

1. It supports expiration (imagine the megabytes of text core contributors have entered over the course of months on Drupal.org, our localStorage would grow quite large :)), yay! The patch at #79 did not yet do that.

2. As always with these things, it's in the conflict management aspect where these things either shine or suck. Garlic is in between. It does not support a server-side generated ID and depend on that unique ID to automatically discard (or at least ignore) localStorage data. We could do trickery with comparing the "changed" timestamp with the auto expiration date stored in localStorage, but that's never going to be reliable (timezones etc), we just need it to accept whatever ID we feed it.

3. If I look at the data in localStorage, it is stored with a key such as garlic:localhost/d8/node/1/edit>form:eq(0)>input.field_image[und][0][alt], despite me passing using the form ID as a selector. Clearly, Garlic.js fails to differentiate between multiple forms on the same URL with similar form structures that can change order. But then again, which forms would swap order *and* have the same form structure? Still, I'd rather see this use the form ID, if it's available.

Taking the above into account, both remaining problems (points 2 and 3, point 1 is no problem) are solvable by a simple change: improving garlic by allowing implements to implement a callback that will generate the "root node" part of the DOM structure of the localStorage key. I.e. instead of form:eq(0), we could then generate form#article-node-form[data-localStorage-changed="<whatever value we have for 'changed'>"]. That would still require us to set the data-localStorage-changed attribute (in JS, before calling .garlic(), which would not invalidate Drupal's render cache) though, but it would require only minimal changes to Garlic.js.

I don't think the #presave stuff is necessary with Garlic, though it's also simply not entirely clear to me why it's necessary. AFAICT it's only used for the message to alert the user and to give them the option to get rid of their previous changes. Is this really necessary? I can definitely see its use, but I think it might be annoying to be gr

I also don't think the enabled flag makes any sense to pass to the JS side; if it's not enabled, then just don't pass it via drupalSettings. Similarly, ['#autosave']['#enabled'] doesn't make any sense: to not enable autosaving, simply don't set (or unset) #autosave.

Status: Needs review » Needs work

The last submitted patch, drupal-153313-form-data-back-90.patch, failed testing.

Wim Leers’s picture

A tiny remnant of my CKEditor core work snuck into #90. Sorry.

Sutharsan’s picture

Status: Needs work » Needs review

Changing state to 'needs review'. I guess that was intended ;)

Status: Needs review » Needs work

The last submitted patch, drupal-153313-form-data-back-92.patch, failed testing.

Pete B’s picture

Status: Needs work » Needs review
FileSize
20.71 KB

Reroll of #92 to make it pass testing...

Wim Leers’s picture

I'm pretty sure that the problems I pointed out in #90 have been solved upstream in Garlic.js 1.2.0, see https://github.com/guillaumepotier/Garlic.js/commit/aa6af62254fc2a3818ca....

Anybody who wants to push this to the finish line? Feature freeze is next Monday!

Wim Leers’s picture

Straight reroll of #95 to apply to HEAD again.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll take a quick stab at this, because it shouldn't be too much work.

Wim Leers’s picture

This should be close to committable.

Fully working. All concerns I expressed in #90 are addressed, and loose ends have been cleaned up.

netsensei’s picture

Okay. I skimmed through the documentation and the patch, but I didn't have the time to test it. I do have one minor question based on the patch in #99:

--- a/core/modules/node/lib/Drupal/node/NodeFormController.php
+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -57,6 +57,12 @@ protected function prepareEntity(EntityInterface $node) {
    */
   public function form(array $form, array &$form_state, EntityInterface $node) {
     $user_config = config('user.settings');
+
+    // Let the node form use localStorage to prevent data loss.
+    $form['#autosave'] = array(
+      '#changed' => isset($node->changed) ? $node->changed : NULL,
+    );
+
     // Some special stuff when previewing a node.
     if (isset($form_state['node_preview'])) {
       $form['#prefix'] = $form_state['node_preview'];

Would this enable autosave only on the node edit form? My first reaction was: what about other entities? The taxonomy term could have multiple fields. There are use cases where i.e. the description field might contain more text (i.e. Detail page of a taxonomy term containing a title, description and a list of referenced entities)

Moreover, I think it would be inconsistent if the node edit form has this safety net while other entities created by content managers,... don't.

So, I was wondering: if we'd move this to EntityFormController instead, wouldn't all entities profit from this?

Wim Leers’s picture

#100: That's probably the only remaining code from before garlic.js (before I started working on this) :)

You are of course right, but I figured we might want to slowly introduce this. For nodes, it's obvious that it's useful. I think we should discuss the consequences of enabling it for all entities (including comments) in a follow-up issue.

jessebeach’s picture

I pulled generateGarlicLocalStorageKey off of Drupal.behaviors.formAutoSave and moved it to the attach method's closure so it can't be futzed with.

I tested nodes and it works as promised. Even across tabs which is really sweet!

I'm not in love with Garlic, but I also wouldn't want to rewrite it. So Garlic it is.

Ranko’s picture

If you don't like Garlick.js how about Sisyphus.js http://simsalabim.github.com/sisyphus/ ? In other words, what's the issue with garlic?

jessebeach’s picture

I just found the code in the Garlick source incredibly hard to read because of its formatting. Who puts a comma at the beginning of a new line?

Ranko, that's an interesting comparison! I brought it up with Wim Leers and he thinks it's worth a comparison. So I'm evaluating it now. Thanks for the suggestion!

jessebeach’s picture

So, here's my take after looking at the two code bases and the two projects.

Let's stick with Garlic for now

Why?

  • It has better test coverage.
  • It has a tagged release, not just a master branch.
  • It has fewer issues logged against it.
  • It has more recent activity.

We can revisit these thoughts later, but for now, Garlic seems the better choice.

netsensei’s picture

Just tested the patch with the default Article content type.

1.

I noticed that the field_image upload is not restored. Neither before or after the upload button has been pressed. Its value does get stored as garlic:node/add/article~0>form#article-node-form input#edit-field-image-und-0-upload in Local Storage however.

As far as I understand garlic.js and what it does, Garlic doesn't really discern between types of <input> elements. It treats the file type as any other text field. So, I believe this more a problem on the side of Garlic then it is with Drupal.

My question: Should we keep the reference to a file on the users' machine in localStorage if it isn't going to be used anyway?

2.

On a related note: when I uploaded a file, I filled out the "Description" text field. The field is triggered through AJAX but I noticed that it doesn't get stored. Given my previous point, the upshot is that any stored "description" field data can not mismatch if the user would add a different file instead. On the flipside, form altering field widgets like multivalued or dependent fields (i.e addressfield, field collection,...) might prove hard to restore to their original state. (I set the body field to multivalued, add a few values, closed and reopened the page: only the first item gets restored. Adding a second item to the field does not restore the original values)

Then again, I take Garlic for what it is: a tool for restoring form data, not form state. Unless it's our intention to broaden the scope and restore state too, these objections are minor.

3.

Do we need specific tests for this?

Apart from that: the patch looks really solid, cleanly applies and works for me.

Wim Leers’s picture

  1. It shouldn't be saving it. As per http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input...

    On getting, it must return the string "C:\fakepath\" followed by the filename of the first file in the list of selected files, if any, or the empty string if the list is empty. On setting, if the new value is the empty string, it must empty the list of selected files; otherwise, it must throw an INVALID_STATE_ERR exception.

    See also:

    (Source for all of the above: http://my.opera.com/community/forums/topic.dml?id=685752&t=1361097892&pa... — transcribed here for posterity.)

    Solved by excluding input[type=file][type=hidden] from the inputs that Garlic.js should handle (I figured I'd explicitly exclude the hidden <input>s at the same time).

  2. Great point! Indeed, it does seem Garlic is currently incapable of restoring value of form items that do not exist on the initial page load, i.e. that are loaded post-page load via AJAX. I think that could be achievable, but that's of course pretty advanced, and not the minimum viable product. OTOH, forms that have "dynamic" form items in the sense that they use Drupal's #states system, those should work just fine, since the form item already exists.
  3. Garlic.js is already unit tested. Drupal core does not yet have JS tests (unfortunately), so we can't write extensive tests. What we can do, though, is ensuring that the necessary JS is loaded when appropriate. Added tests for that.

Assuming this patch gets RTBC'd, follow-up issues for these aspects should be created:

  1. Where else to enable #autosave, besides on the node form?
  2. Somehow get this in http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc..., I can't find how to get it in there via the Drupal core code.
  3. … what else?
nod_’s picture

Status: Needs review » Needs work

Broken on FF

nod_’s picture

garlic.js is a massive cpu hog on FF at least, makes FF take 10-20% of CPU when idle

Wim Leers’s picture

I can't reproduce because FF is using 7–15% when idle, without a single page loaded. This also makes it effectively impossible for me to debug this. :(

sun’s picture

I can't comment on the browser-specific performance issues today/right now, but here's my review of the Drupal integration code (which most likely won't change, regardless of which library is used):

+++ b/core/includes/form.inc
@@ -969,6 +969,19 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+  // Attach localStorage-powered auto form saving behavior.
+  if (!empty($form['#autosave'])) {

drupal_process_form() isn't really the right spot to for #attached - the info is only needed when a form is actually rendered.

Technically, the most appropriate place is a new #pre_render callback that's declared for #type 'form'.

+++ b/core/includes/form.inc
@@ -969,6 +969,19 @@ function drupal_process_form($form_id, &$form, &$form_state) {
+          $form['#id'] => ctype_alnum($form['#autosave']) ? $form['#autosave'] : 1,

We're not using ctype functions in Drupal.

Thus far, the only used #autosave identifiers are timestamps and numbers? So any reason to not just simply cast to (int)?

+++ b/core/misc/form.autosave.js
@@ -0,0 +1,74 @@
+          .attr('data-localStorage-changed', changed)

Must be namespaced correctly; i.e., data-formautosave-changed

+++ b/core/misc/form.autosave.js
@@ -0,0 +1,74 @@
+ * We don't need to store the entire DOM node path, because each form item (as
+ * well as the form itself) is uniquely identified by its id attribute.

Hmm. We can make a lot of assumptions about Drupal forms, but HTML IDs on the form or on form elements do not actually belong to them. In fact, the most important form input elements in a Drupal form do not have a HTML ID (i.e., the hidden form_id, form_build_id, token, etc elements).

+++ b/core/misc/form.autosave.js
@@ -0,0 +1,74 @@
+ * Finally, it takes a "last modification" identifier into account, which allows
+ * us to ignore localStorage data that doesn't apply anymore to the latest
+ * version of the object that the form allows the user to edit.

Does that mean this facility will suddenly forget my last input if you happen to comment on a d.o issue before I managed to press submit?

Why would the server-side state of an entity play any role in this? Isn't the whole point here to auto-save form data?

Note: Garlic seems to have some limited support for handling conflicts.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -56,8 +56,11 @@ protected function prepareEntity(EntityInterface $node) {
+    $form['#autosave'] = isset($node->changed) ? $node->changed : TRUE;

What if $node->changed is 0?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.php
@@ -43,6 +43,16 @@ function setUp() {
   function testNodeCreation() {
+    // Check that the autosaving functionality is present.

+++ b/core/modules/node/lib/Drupal/node/Tests/PageEditTest.php
@@ -54,6 +54,15 @@ function testPageEdit() {
+    // Check that the autosaving functionality is present.

I'd prefer a dedicated test class for this functionality.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/AutoSaveTest.php
@@ -0,0 +1,80 @@
+  function testAutoSave() {
+    // Simple #autosave === FALSE/NULL.
+    $this->drupalGet('form_test/autosave/disabled');
+    $this->checkJS(FALSE);

This does not actually test #autosave = FALSE/NULL. It only tests a non-existing property.

Wim Leers’s picture

Title: Preserve in-progress form data in localStorage » Preserve in-progress form data in localStorage using Garlic.js
Status: Needs work » Needs review
FileSize
16.76 KB
31.21 KB

#111:
Thanks for your stellar feedback! That's the sort of feedback I'd been looking for for a while :)

Most concerns addressed; where relevant or where not addressed, explained below:

  1. RE: "#pre_render for #type 'form'"
    I tried precisely this, but … it doesn't work for entity forms, seemingly because entity_get_form() calls drupal_build_form(), which doesn't call drupal_render(), and none of the code in form.inc has handling for #pre_render; that is a drupal_render() thing.
    I used #process instead.
  2. RE: ctype vs. (int) casting.
    I wanted to allow for alphanumerical identifiers. But you're right, just integers should do.

    Note that your claim is not entirely true. I did check whether Drupal was using ctype, and it actually is: in \Drupal\Core\Utility and in the EasyRdf, Assetic, PHPUnit, Symfony, and Twig libraries.

  3. Hmm. We can make a lot of assumptions about Drupal forms, but HTML IDs on the form or on form elements do not actually belong to them. In fact, the most important form input elements in a Drupal form do not have a HTML ID (i.e., the hidden form_id, form_build_id, token, etc elements).

    Well, we don't need to store input[type=hidden]' values anyway; they're explicitly excluded in #107.
    I think that for the remaining form items, this assumption is true? I.e., form_builder() does this:

      if (!isset($element['#id'])) {
        $element['#id'] = drupal_html_id('edit-' . implode('-', $element['#parents']));
      }
    

    AFAICT, that would cause every non-hidden form item to have an ID?

  4. Does that mean this facility will suddenly forget my last input if you happen to comment on a d.o issue before I managed to press submit?

    Why would the server-side state of an entity play any role in this? Isn't the whole point here to auto-save form data?

    Great point. I've also pondered about this.

    It seems like you're conflating two things here though:

    1. Node forms having $form['#autosave'] = $node->changed
    2. Comment forms having $form['#autosave'] = $comment->changed, not $form['#autosave'] = $node->changed. (I've added autosaving to comment forms in the attached reroll.)

    I agree it would make no sense at all to have your locally saved comment being deleted (actually, ignored, it's currently left to garlic's expiration mechanism to do the clean-up) if the node had changed. But it does make sense (in most cases) to have your locally stored node/comment form data be ignored if the node/comment has been updated since you wrote it. After all, your modifications may no longer be relevant.
    OTOH, for things like issue summaries, where multiple people might be making modifications, it is definitely reasonable to want to have a way to regardless of changes made by others, still being able to retrieve your changes. But that implies introducing yet another UI concept, with a most likely confusing UI — see Bojhan's comment in #61.
    So, personally, I'd rather defer deciding how to deal with that to a follow-up issue. In that follow-up issue, we could then decide whether core should handle this, or whether it should be left to contrib.

    In any case, this is an improvement and a big step in the right direction.


#108/#109:
If this really were a Garlic.js problem, then:

  1. http://garlicjs.org should be similarly CPU taxing, but it's not — when doing nothing, Firefox idles to 0.0–0.1% with Firefox in the foreground.
  2. Not loading form.autosave.js nor garlic.js should remove that "10-20%" of CPU usage, but it does not. This means that this Firefox-only JS performance issue (assuming it is a JS performance issue) was *not* caused by this patch, but is already a (Firefox-only) problem in D8 HEAD.

Nevertheless, I tightened Garlic.js settings:

-            inputs: 'input[type!=file][type!=hidden], textarea, select',
+            inputs: 'input[type!=file][type!=hidden][type!=submit][type!=button], textarea, select',
+            events: [ 'textInput', 'input', 'change', 'keypress', 'paste', 'focus' ],

Garlic.js works fine for me in Firefox (tested by going to node/add/article, typing stuff everywhere, closing Firefox, opening it again, navigating to node/add/article again, and it's all there).

(Maybe the tightened settings helped.)

If you're still having problems, please disable EVERY Firefox add-on, manually reset all caches and restart it. Possibly create a temporary "clean slate" new user profile. Firefox (18) exhibits a lot of weird performance characteristics, so it's possible that that is also influencing what you're seeing.


Besides the above:

  • Changed the scope of the two JS files to "footer", for faster page loading.
msonnabaum’s picture

Performance impact looked rather minimal to me doing a quick profile in chrome on a node add form, but I didn't try on firefox.

The patch looks rather good to me. The only thing I noticed was that generateGarlicLocalStorageKey is defined globally and I'd expect it to be somewhere on the Drupal object.

Wim Leers’s picture

#113: actually, it's not at all a globally defined function, it's scoped in a closure (meaning you have no way to modify it at all). :) Jesse did this in #102. (Originally, I had it at Drupal.behaviors.formAutoSave.generateGarlicLocalStorageKey().)

Dave Reid’s picture

Does this work with WYSIWYG-enabled fields now that CKeditor is included in core? Has anyone tested that? I don't see any issues in the Garlic.js tracker or documentation related to WYSIWYG.

jessebeach’s picture

#113: actually, it's not at all a globally defined function, it's scoped in a closure (meaning you have no way to modify it at all). :) Jesse did this in #102. (Originally, I had it at Drupal.behaviors.formAutoSave.generateGarlicLocalStorageKey().)

It seemed to me that we don't want to make the function public to alter. If a contrib module comes along and changes the function and subsequently how the local storage key is generated, it could strand a lot of saved data in localStorage.

So as the patch stands, the function is tucked away in a closure where outside scripts can futz with it.

msonnabaum’s picture

Yup, that seems the right thing to do. Carry on :)

Wim Leers’s picture

#115: Wow. Great point. I think we've all been so focused on getting this to work well, that we forgot about WYSIWYG :)

The short answer is: yes, this is possible.
The better answer is: here it is :)

At first, I was hesitant to do it in this patch — I wanted to make it a follow-up. But both Dave Reid and msonnabaum preferred to get that committed as part of this patch, so here we go.

The interdiff will show you how little has changed: A) editor.module's JS API is extended slightly to also have an onChange(element, callback) method (was already planned and reviewed at #1886566: Make WYSIWYG editors available for in-place editing + #1873500: CKEditor + Edit anyway), B) editor.module will now serialize changes every 0.5 seconds back to the corresponding <textarea> and will trigger an event, C) garlic.js will now listen to that event as well.

EDIT: and to clarify: I checked with msonnabaum on the "sync changes every 0.5 seconds" aspect, and that definitely does not introduce performance problems; nor would it lose to much user changes — after all, how much can one type in 0.5 seconds? :)

Test it at http://simplytest.me/project/drupal/612c7e2c4b79927b919175c0eaeb115ab6172215?patch[]=http://drupal.org/files/form_autosave_garlic-153313-118_0.patch.

seutje’s picture

ran trough the code and don't see anything I'd have a problem with, also tested it manually on a variety of setups and could manage to break it or experience any noticeable performance issues.

I wasn't able to test it on IE10, and considering https://github.com/guillaumepotier/Garlic.js/issues/25, I was wondering if anyone had an IE10 environment they could manually test this with?

Wim Leers’s picture

I do not; but considering IE10's much improved standards support, I expect it to run just fine. In any case we can solve browser-specific problems if they arise, and thanks to Garlic.js' test suite, it should be pretty easy to prevent regressions.

What's left to do to get this to RTBC? :) We've got a few more hours left to achieve that…

webchick’s picture

I don't think this is a commit-blocker, per se, but UX-wise, it would be nice to get a dsm() or some other kind of alert when things are being loaded from the local store. A real, actual use case that just happened:

I applied this patch, edited an article and started typing crap on the end of my tag to see what would happen. Closed the browser. Then I got into an IRC PM session for a half hour. Then I ate lunch. Then I came back to test something related to WYSIWYG, completely forgetting about the earlier edits I'd made and the patch I was testing, and I had random crap on the end of my tags, which was not there on the front end. What?

It would be nice if, upon loading the node edit form, I got a little message that was like "Loaded your changes from an earlier draft" or something. Except not draft, since it's not a DRAFT draft. Ugh. Is there other CMS's text out there we could copy/paste?

Damien Tournoud’s picture

Status: Needs review » Needs work

This is a really bad idea, as there are many ways it can and will be wrong.

Two examples off the top of my head (I haven't checked if it is true, but seems logical conclusions from reviewing the code):

  • The node form locking mechanism is going to get in the way: User A opens the node form and prepares some modification, User B prepares and save the modification, User A is going to end up with a "The content on this page has either been modified by another user, or you have already submitted modifications using this form.", possibly forever?
  • All dynamic form modifications are unsupported. Which means that 99% of the node forms are actually unsupported, because the "Add another item" from the Field API is using an AJAX modification.

Given the current state of the Form API, in-progress form data will have to be saved on the server side, we cannot reliably save anything on the client side.

Wim Leers’s picture

Status: Needs work » Needs review

#122:

  • First: when a node is changed, then the form data in localStorage is ignored, so "endless lock" is impossible. Second: we don't restore <input type="hidden" name="changed" value="<$node->changed value here>">, because that's a input[type=hidden], which we specifically do not deal with. So even if we weren't going to ignore form data when the node was changed, then it would still be impossible to have an endless lock.
    Your specific use case would work like this: user B has saved the changes, user A tries to save but gets the "modified by other user" message. Upon reloading the page, the form will actually no longer contain any of the changes.
    Please see #112's reply to #111, point 3 (the reply to the blockquote) for full detail.
  • "99% of the node forms are actually unsupported" — well, not 100% of form items would be supported, that's true, but 90% of form items are supported.
    This was originally raised in #106.2, and I answered in #107.2:

    Great point! Indeed, it does seem Garlic is currently incapable of restoring value of form items that do not exist on the initial page load, i.e. that are loaded post-page load via AJAX. I think that could be achievable, but that's of course pretty advanced, and not the minimum viable product. OTOH, forms that have "dynamic" form items in the sense that they use Drupal's #states system, those should work just fine, since the form item already exists.

    Restoring of AJAX form items is nigh impossible, because it'd require replaying user actions in certain cases. So yes, from that POV you're right that server-side storage is a requirement. But that comes with increased network activity (and thus worse battery life), scalability/storage requirements, poor reliability on mobile/spotty network connections and so on. So that's actually a really bad idea as well.

I'm a perfectionist, so I totally understand where #122 is coming from. This patch isn't going to be able to cover every possible advanced form use case, that's true. But it's going to make a significant difference for big parts of much used forms.

Given that the first point was wrong, and the second one is also "a really bad idea", reverting to needs review.

swentel’s picture

Status: Needs review » Needs work

All dynamic form modifications are unsupported. Which means that 99% of the node forms are actually unsupported, because the "Add another item" from the Field API is using an AJAX modification.

Doesn't work indeed, even with a single image upload for instance either. I can predict dozens of support/bug tickets in the field queue to ask where 'OMG, where did our uploaded files go to, or my second textfield, or whatever". The only answer to that will be "won't fix" or "works as designed" from me (and I think yched would agree). I know that sounds negative, but I can only give you my point of view as well on this.

swentel’s picture

Status: Needs work » Needs review

Crosspost - none the less, I don't think this is a good idea.

jessebeach’s picture

Status: Needs review » Needs work

I don't understand why we're unwilling to make a minor improvement that will save some data because we haven't yet made the major improvement that will save all data.

An uploaded image can be re-uploaded. A textarea of text that gets lost is gone. Gone like a thesis lost to a failed hard drive. That's harsh.

This issue had been active for more than 5 and a half years. That's 2007 days of people losing the body text of a node they just spent perhaps hours writing because when switching tabs they accidentally closed the tab instead of selecting it.

An incremental improvement here is better than no improvement.

jessebeach’s picture

Status: Needs work » Needs review

I think I cross-posted to needs work. Setting back to needs review.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

It's a shame this won't work with ajax forms, but I think this is most useful on simpler forms with textareas anyhow, so no sense in vetoing the whole idea based on that.

I'd be fine if this didn't have the DSM, but it might be nice to clarify that Drupal restored your content and not your browser. That could easily be handled in a followup.

I tested this again with a more complex content type and it worked quite well. This seems ready to me.

Crell’s picture

For fancy ajaxy forms, there's still the autosave module. (Much to my dismay. :-) ) We could probably borrow some UI ideas from there for the "hey, you've been restored!" message, too.

jessebeach’s picture

I'm happy to code the followup that produces a message to a user that form data has been restored. Bojhan and I can collaborate on what that would look like.

sun’s picture

Status: Reviewed & tested by the community » Needs review

We still need to use #pre_render instead of a process callback.

That said, I also agree with @Damien Tournoud in #122. If you fully think through the consequences, then we essentially arrive in the same problem space and conclusion as #1510544-131: Allow to preview content in an actual live environment — user data should be saved on the backend instead.

I don't really see the point of adding a facility to core that claims to do something and thus introduces user expectations, while the facility is known and guaranteed to break.

As long as that rather big limitation exists, this is contrib material.

webchick’s picture

Issue tags: +RTBC Feb 18

Tagging as something that was RTBC in time for feature freeze.

Damien Tournoud’s picture

I agree with @sun that something that works in some very limited cases (in my experience, 99% of the node forms have an AJAX "Add another item") is contrib material.

Gábor Hojtsy’s picture

We still need to use #pre_render instead of a process callback.

As said above, entity forms don't use any of the code that would currently make it possible to use a #pre_render. We can go and refactor entity form handling, that might not be wise to do in this issue.

netsensei’s picture

I tend to follow Sun et al per #106. Given the extensive efforts put into UX through other initiatives, I think this boils down to: are we willing to compromise for the sake of a moderate functional improvement?

Come to think of it: let's not forget that this is problem is not limited to Drupal. Any filled out data on any form on the web is prone to getting lost. Other frameworks, like WordPress, solve this with similar basic autosave functionality. Our ingrained AJAX system in the Form API really raises the bar to get this in in a sensible way on short notice imho.

Crell’s picture

Autosave gets around the dynamic form problem by just regenerating and replacing the entire form server-side rather than using localStorage. It's horribly ugly, but works. If this does get punted from core this cycle I'm happy to take the work here into autosave to replace what's there. (I'm happy to turn it over to someone from this issue entirely, in fact. :-) )

What about allowing the client side code to essentially re-script the the process of modifying the form? Probably something more elegant than a macro, but along those lines: FAPI demands that we trigger "change the form structure" commands. So, let's trigger those commands, then fill in the saved data accordingly.

Damien Tournoud’s picture

@Crell: the whole state of the form is on the server. There is no way to reconstruct it completely from the client at this point. We should should refactor the Form API so that (1) the state is the king, and the form is built from the state [which is kind of the case right now, but only kind of], (2) interaction to the state is done via actions (as in MVVM models). This would allow us to build the form both from the client and the server, to perform some of the form modification / validation workflows directly on the client without hitting the server, and eventually to save the full state of the form in the client, as described in this issue, while keeping a unified framework between the client and the server and strong validation guarantees.

But that's a complete other story :)

Crell’s picture

Damien: Yes, that's very Drupal 9 material, wherein we may be looking at the Symfony Form API anyway. For Drupal 8, is what I described possible? Conceptually I don't see why it couldn't be, even if it's ugly.

moshe weitzman’s picture

I think this is a significant usability win, and am fine with reuploading images, not supporting multiple values, etc.

nod_’s picture

Version: 8.x-dev » 9.x-dev

We're way past feature freeze and well into API freeze.

kattekrab’s picture

But this was RTBC before Feature Freeze... see #132

and agree with @jessebeach #126

"I don't understand why we're unwilling to make a minor improvement that will save some data because we haven't yet made the major improvement that will save all data."

Wim Leers’s picture

Version: 9.x-dev » 8.x-dev

Moving back to 8.x because it is not uniformly agreed upon that this should be a 9.x feature.

Furthermore: this is a small *addition*, not a change, that could potentially happen in e.g. the Drupal 8.2 release.

Damien Tournoud’s picture

Version: 8.x-dev » 9.x-dev
Category: bug » feature
Issue tags: -RTBC Feb 18

Randomly marking a feature RTBC before feature freeze doesn't make it actually RTBC (granted, that would make the review process way easier if it was true).

We have three people (sun, swentel, myself) agreeing that the feature is at most half-backed in it's current state, and will require significant refactoring to get right. So, yes, this is Drupal 9 material.

Also, this is really not a bug for any definition of a bug.

moshe weitzman’s picture

Version: 9.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +RTBC Feb 18

Agree on this being a feature.

Please don't remove the tag that Angie set in February. We should let the committers decide about eligibility for D8. If they decide that now is too late, then the supporters will work on this for D9.

We have key people who support the current patch as well (jessebeach, msonnabaum, wimleers, moshe weitzman,..)

kattekrab’s picture

Would a new issue summary help here?

kattekrab’s picture

Oh look! here's that garlic.js issue again.

Is anyone still interested in pursuing this? If yes, I'll happily refresh the issue summary to get everyone on the same page.

:)

cosmicdreams’s picture

This totally sounds like Drupal 8.3 material. If we can agree to develop and continue to improve Drupal past launch we can build support for this in later.

LewisNyman’s picture

Issue tags: +frontend
kattekrab’s picture

This is a major pain point for content people. It's so so sooooo sad to see this can perpetually kicked down the road.

Someone said this could be 8.2 - and then someone says 8.3

And some say 9? Gah.

Bojhan’s picture

Assigned: Wim Leers » Unassigned
Issue tags: -RTBC Feb 18

@Kattekrab No worries, I don't think anyone can predict that far away. If anything it signals, that people are aware of the amount of work involved. It could easily be picked up whenever.

Bojhan’s picture

@Kattekrab No worries, I don't think anyone can predict that far away. If anything it signals, that people are aware of the amount of work involved. It could easily be picked up whenever.

Bojhan’s picture

@Kattekrab No worries, I don't think anyone can predict that far away. If anything it signals, that people are aware of the amount of work involved. It could easily be picked up whenever.

David_Rothstein’s picture

Title: Preserve in-progress form data in localStorage using Garlic.js » Form data is lost when using the browser's back button
Category: Feature request » Bug report
Issue tags: +Needs manual testing

The original issue here is definitely a bug. (Garlic.js was just one possible solution.)

I'd be interested to see testing with the latest Drupal codebase and with modern browsers to see where things currently stand (for examples from very long ago see #3 and #18 although more recently than that people have said it's Chrome too).

I think @casey's approach from #30 still looks interesting and could use some testing as well.

Preserving form data in other situations (e.g. after closing the browser) should really be a separate issue (and that sounds like a feature request) since it gets quite a bit more complicated.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

laVera’s picture

I came to this issue after loosing many hours of redaction work, and I have to say: WTF! Shame on all us!

I totally agree with @kattekrab and @Crell this is a big bug, and if is possible to fix it (even in a nasty way) it should be done ASAP.

But reading here how this issue was treated, I feel like last @dries keynote was an empty promise on content creators (Donald Trump level empty promise). Content people needs' are not only underrepresented, but also pushed down right here on issue tracker.

I never before felt so disappointed with our community; I don't mean to offend personally to anybody. But I do mean to call you all attention here, this issue need it.

So, my question to the seniors here:

How should a Drupaler next door —like me— should deal with this?

  • Should I go Rocky Balboa in my code skills to became a core contrib? (it may take me some years)
  • Perhaps I should open a online petition to it?
  • Or maybe it's a sign that I should give a serious try to Backdrop?
droplet’s picture

Saving data into localStorage lead to another security issue. We could close/hang the browser at any time. Sensitive data we filled will be exposed.

I believed a simple popup warning message could save most of those posting here. And then seek for the perfect deal. Gmail-like app also popup a message before they actual saving draft into the remote server. It's almost a must element.

@nahuel,
This post has pretty a lot discussion. To help other issues, you can post your thoughts here: #2706483: [policy, no patch] Policy to help less interested Patch move forward.. Don't kick developers with passions out! Actually, I mentioned `What Is the Real Critical Issue` there: https://www.drupal.org/node/2706483#comment-11250037

Benia’s picture

I suggest adding autosave system as they did a few years ago in Wordpress... It could improve much of the edit experience in Drupal and will align with the major improvements in Drupal 8.

Benia’s picture

Opened a discussion about an autosave system here:

https://www.drupal.org/node/2801839#comment-11633647

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

margyly’s picture

I love the idea of autosave and the users and my organization would love it, too. Sorry I can't help make it happen!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

lauriii’s picture

catch’s picture

Title: Form data is lost when using the browser's back button » ckeditor input is lost when using the browser's back button
Issue summary: View changes
Issue tags: +Needs manual testing, +Bug Smash Initiative

I just briefly tested this on chromium, on Drupal 7 (Drupal.org) and Drupal 9.

1. Entering content into a form, clicking off (in my case to admin/structure), then hitting the back button, doesn't lose data from regular form fields.

2. Doing the same, but with ckeditor4 in a textarea does.

This needs testing safari, edge, firefox to see if the behaviour is the same. We could also test the same thing with ckeditor5. If it's fixed in ckeditor 5 and consistent across browsers, I think we could close this issue (I don't see us successfully fixing it for ckeditor 4, and the work would be obsolete for Drupal 10).

pameeela’s picture

Status: Needs work » Closed (outdated)
Related issues: +#3029488: Implement Autosave

Discussed with @catch. Since this only applies to CKEditor fields now, and the proposed fix was an autosave feature, I'm going to close this in favour of #3029488: Implement Autosave.