WordPress.org

Make WordPress Core

#22491 closed defect (bug) (fixed)

Autosave / AYS on the attachment edit screen

Reported by: helenyhou Owned by: azaozz
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (7)

22491.diff (997 bytes) - added by helenyhou 17 months ago.
22491-2.patch (1.9 KB) - added by azaozz 17 months ago.
22491.2.diff (1.1 KB) - added by nacin 17 months ago.
22491.3.diff (709 bytes) - added by SergeyBiryukov 17 months ago.
22491-4.patch (1.1 KB) - added by azaozz 17 months ago.
22491.4.diff (1.2 KB) - added by nacin 17 months ago.
22491.5.diff (781 bytes) - added by johnbillion 17 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 helenyhou17 months ago

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.

comment:2 nacin17 months ago

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

comment:3 nacin17 months ago

  • Owner set to azaozz
  • Status changed from new to assigned

helenyhou17 months ago

comment:4 follow-up: helenyhou17 months ago

  • Keywords has-patch added

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

comment:5 in reply to: ↑ 4 azaozz17 months ago

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.

comment:6 follow-up: helenyhou17 months ago

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.

comment:7 in reply to: ↑ 6 nacin17 months ago

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.

azaozz17 months ago

comment:8 azaozz17 months ago

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

comment:9 azaozz17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:10 nacin17 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

nacin17 months ago

comment:11 follow-up: SergeyBiryukov17 months ago

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

SergeyBiryukov17 months ago

comment:12 follow-up: nacin17 months ago

My patch avoids delimiters by comparing two arrays.

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

comment:13 nacin17 months ago

In 22777:

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

comment:14 in reply to: ↑ 12 helenyhou17 months ago

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. :)

comment:15 in reply to: ↑ 11 azaozz17 months ago

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 :)

azaozz17 months ago

comment:16 azaozz17 months ago

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).

comment:17 nacin17 months ago

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. :-)

nacin17 months ago

comment:18 nacin17 months ago

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?

comment:19 azaozz17 months ago

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.

comment:20 follow-up: azaozz17 months ago

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).

Last edited 17 months ago by azaozz (previous) (diff)

comment:21 in reply to: ↑ 20 nacin17 months ago

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.

comment:22 azaozz17 months ago

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 :)

comment:23 nacin17 months ago

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.

comment:24 azaozz17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 22795:

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

comment:25 johnbillion17 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

johnbillion17 months ago

comment:26 johnbillion17 months ago

  • Keywords has-patch added; needs-patch removed

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.

comment:27 nacin17 months ago

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.

comment:28 helenyhou17 months ago

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.

comment:29 nacin17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 22858:

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

Note: See TracTickets for help on using tickets.