WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#22266 closed defect (bug) (fixed)

onbeforeunload alert always triggers with attachments

Reported by: duck_ Owned by: koopersmith
Milestone: 3.5 Priority: low
Severity: blocker Version: 3.5
Component: Media Keywords:
Focuses: Cc:

Description

Steps

  1. Create a post with attachments in it
  2. Visit the post's edit screen
  3. Navigate away without modification and see an alert about "The changes you made will be lost..."

Trying to press "Update" and then leaving gives the same result.

Cause

tinymce.activeEditor.isDirty() is always returning true because attachments are lazy loaded with individual XHRs and so the editor's startContent property doesn't contain the full HTML for an attachment. Once an attachment model has been loaded then it's re-rendered and the editor content is different even though the user hasn't made any modifications.

Related: #21390, #21813, [22012]

Attachments (1)

22266.diff (1.8 KB) - added by koopersmith 17 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 duck_18 months ago

A couple of options:

  1. Some kind of bootstrapping for images instead of loading them in a separate XHR
  2. Overwriting the editor's startContent

Option 2. has the potential issue of a slow loading image updating the startContent after a user has made some actual modifications and stopping the navigation warning from appearing.

comment:2 SergeyBiryukov18 months ago

Related/duplicate: #22048

comment:3 SergeyBiryukov18 months ago

#22048 was marked as a duplicate.

comment:4 nacin18 months ago

  • Priority changed from normal to high
  • Severity changed from normal to blocker

comment:5 follow-up: azaozz18 months ago

Discussed this with @koopersmith in IRC. Only happens when the user opens an existing post for editing but doesn't edit anything, caused by the replacement of the shortcodes with "views" when the editor loads the initial content.

There is a way to tell TinyMCE the content wasn't modified which will cancel the beforeunload event. We need a bit more logic to make sure the user hasn't started typing before all shortcodes were replaced with views. Patch coming soon.

comment:6 in reply to: ↑ 5 ; follow-up: nacin18 months ago

Replying to azaozz:

There is a way to tell TinyMCE the content wasn't modified which will cancel the beforeunload event. We need a bit more logic to make sure the user hasn't started typing before all shortcodes were replaced with views. Patch coming soon.

We're not going to block the user, right? But rather, once all views are loaded, we should only set dirty to false if the user hasn't started typing by then.

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

Replying to nacin:

Right. Tested attaching an one-time-only callback to the keydown and paste events so we know when the user has started typing. If that occurs before all views are loaded, we leave the editor state as "dirty" (i.e. onbeforeunload will trigger). If the user hasn't started typing when all views are loaded, we set the editor state to "clean".

comment:8 nacin18 months ago

The other thing to think about is what happens when they try to leave a post before all of the views are loaded. If the one-time-only callbacks have yet to fire, we'll want to consider it to be clean.

comment:9 azaozz18 months ago

Yeah, the logic is pretty simple: there's a var isClean = true; that is changed to false onpaste and onkeydown. Both callbacks are removed if any of them runs or when all views are loaded. After all views are loaded we check that var and set ed.isNotDirty = true if isClean is true.

It's a bit tricky to sync this with ed.isNotDirty while the views are loading, trying to figure out the best way (the onbeforeunload event in the browsers is a "special" one and doesn't behave as a standard event).

comment:10 nacin17 months ago

  • Priority changed from high to low

Removing views, see #21813 (comment 30), should resolve this for 3.5.

comment:11 koopersmith17 months ago

  • Owner set to koopersmith
  • Resolution set to fixed
  • Status changed from new to closed

In 22567:

Media: Restore 3.4 editor behavior and remove TinyMCE views.

  • Reactivates the wpgallery and wpeditimage TinyMCE plugins. Deactivates the wpviews TinyMCE plugin.
  • Moves still-relevant logic from mce-views.js to media-upload.js and shortcode.js.
  • No longer include wp-includes/js/mce-views.js. This code will not be used in 3.5, and should be considered unstable.
  • Currently, this is the real 3.4 experience; as such, editing triggers the old modals. Changing this is the next major step.

When reassessing views, we should look over all of these tickets and anticipate these bugs accordingly.

fixes #21813, #22123, #22155, #22161, #22257, #22266, #22318, #22407, see #21390.

comment:12 koopersmith17 months ago

Another method we could use here is to avoid editor.isDirty() entirely, and compare processed versions of the content. Attaching a patch as an example.

koopersmith17 months ago

Note: See TracTickets for help on using tickets.