Autosave / AYS on the attachment edit screen

[helenyhou]

Autosave/AYS you want to leave this page behavior is based on an input with the ID of content; specifically, the JS is looking for #post #content. The attachment edit screen uses attachment_content for the textarea's ID instead (#content is also used for some JS dealing with the editor size). Thus, if there is a description/attachment page content for the attachment, you'll get an AYS when trying to leave the page, even if you haven't changed anything, and if autosave has fired (which you can trigger by running autosave(); in the console instead of waiting), there will always be a red warning at the top saying that there is a newer autosave, which has empty content.

Here are options I see:

1. Turn off autosave for attachments. Since it isn't displayed like a regular post content editor, and in most use cases doesn't see a lot of action, it doesn't seem as crucial or disruptive to not support it.
2. Change autosave/AYS comparison JS to look for an input with the name content rather than ID. Not sure what side effects this might cause, including with nonces and such. This seems like a more correct solution for the admin as a whole, though.
3. Use a WP_Editor instance that supports TinyMCE, which would then need to be out of the metabox. This has been discussed and tried out - I think it disrupts the editing hierarchy and though one might think that consistency with other edit screens would be ideal, it isn't in this situation, as things like media buttons would be turned off (attachments to attachments, oy).

The first option is probably the least disruptive, and is where both azaozz and I am leaning.

1 seems good at this stage. Attachments didn't have autosave in 3.4, so this is not a regression.

22491.diff​ checks the post type before enqueueing the script on both post-new.php and post.php.

Replying to helenyhou:

Is there a case where attachments are loaded as new posts (with post-new.php)? As far as I can see attachments are always edited, they cannot be created separately from the uploaded file.

I don't think there's a link to it anywhere, no. Technically you can navigate to /wp-admin/post-new.php?post_type=attachment though. Seems like we should probably prevent/redirect that as well.

Replying to helenyhou:

I don't think there's a link to it anywhere, no. Technically you can navigate to /wp-admin/post-new.php?post_type=attachment though. Seems like we should probably prevent/redirect that as well.

Yeah, getting to that today.

22491-2.patch includes 22491.diff​ and adds AYS on page unload for title, caption, alt, content and slug fields.

In 22725:

Don't load autosave.js on the attachment editing screen, add simple AYS on unload if there are changes, props helenyhou, fixes #22491

I don't mean to be a pain, but because getFieldsContent() doesn't use a delimiter, moving an entire field's content from title to caption, or caption to alt, etc., would result in no AYS. I'm fine with simple, but moving content from field to field isn't exactly an uncommon use case.

22491.3.diff​ is an attempt to use .map().

My patch avoids delimiters by comparing two arrays.

Should 'autosave' simply not be printed when the post type doesn't support 'editor'?

In 22777:

Redirect post-new.php?post_type=attachment to media-new.php. see #22491, #22083, #21391.

Replying to nacin:

Should 'autosave' simply not be printed when the post type doesn't support 'editor'?

Yeah, actually. That would be the sensible way. :)

Replying to nacin:

Sure, comparing an array of strings would make it more precise.

Replying to SergeyBiryukov:

We could use few different jQuery functions but thinking building the array by hand is the fastest and clearest way :)

22491-4.patch is essentially the same as 22491.2.diff​ but uses jQuery.each() to loop through the array and compare the values (makes it a little shorter).

I count 6 lines of code versus 8 lines of code. If by mean shorter you mean 25% longer and using a function instead of a language construct, then sure. :-)

The issue here simply appears to be that when you do title + content, and one of them is undefined, autosaveLast sometimes gets 'undefined' stuffed into the string — but only in some situations. It is set properly during autosave, and improperly on pageload. So when autosave fires, it screws things up.

Can someone confirm that, after reverting [22725], 22491.4.diff​ solves the reported issues?

It does, although I'm a bit concerned changing autosave right before RC :)

On the other hand the change looks good and fixes a bug there, so lets go with it.

Also: fixing and loading autosave here would run it and catch changes to the title only. If the title is edited, it will create a revision of the attachment post and show AYS on unload.

We can avoid creating a revision by changing the id of the title field (not the name, so $_POST won't change).

Replying to azaozz:

Also: fixing and loading autosave here would run it and catch changes to the title only. If the title is edited, it will create a revision of the attachment post and show AYS on unload.

We can avoid creating a revision/showing AYS by changing the id of the title field (not the name, so $_POST won't change).

The chance of the title field changing but not being saved is fairly slim. We can improve autosave in a future release.

I'm also okay with keeping [22725] as long as we also apply 22491-4.patch​ or 22491.2.diff​ for 3.5. But we should also apply 22491.4.diff​ for the sake of plugins.

Okay, lets do that. Having autosaves/revisions only for the titles would be strange.

22491.2.diff looks good, native JS would always be faster than jQuery :)

In 22794:

Autosave: Properly convert undefined fields to empty strings. This bug could cause issues if a post type didn't support the title and/or editor. see #22491.

In 22795:

Improve AYS comparison on the Edit Attachment screen, props nacin, fixes #22491

I'm getting an AYS when I do update the content on this screen now.

To reproduce: edit any of the fields and hit the Update button.

22491.5.diff​ removes the onbeforeunload handler when the form is submitted or the delete link is clicked. It also disables the submit button and displays the spinner so it's consistent with the main post editing screen.

We have a bad case of D.R.Y. on this screen. This code is mostly copied from autosave.js.

I'd be inclined to just restore the original autosave.js here. It'd only autosave the title, but I think that is fine.

If changing the ID from attachment_content to content provides autosaving for the attachment page content without breaking anything, I'm fine with that oo.

Can we just go back to the original plan of not having autosave on the attachment edit screen (or if the post type doesn't support editor)? The original autosave.js has problems because it's specifically looking for #post #content to match the post content, which in this case, it won't if there's anything in the post content (#post #content being empty here). It may autosave the title, but you'll still get the AYS as well as an autosave with empty content that will give you the newer autosave admin warning when the content wasn't originally empty.

Using the ID of content creates issues with styling, since it's not a TinyMCE instance but there's a bunch of JS that looks for #content and does special things to it that are specific to matching the textarea to TinyMCE and the editor size. Seems like that should be dealt with in a better way (like only doing that special stuff if #content is associated with a TinyMCE instance), but I don't think that's a wise thing to pursue in the RC tage.

In 22858:

No AYS or autosave for attachments on post.php. Reverts part of [22725]. fixes #22491.