MetaBoxes: Remove dirty-checking metaboxes state

[youknowriad]

closes #4183 and closes #3975

Due to how MetaBoxes work it's not possible to dirty-check them properly. It's not possible to know if they contain default values as well. Thus, this PR removes the meta boxes dirty checking making Gutenberg work like the classic editor when there are active meta boxes which means:

• The "save draft" and "update" button are never disabled
• If you try to leave the editor's page with changes in the meta boxes area, you're not prompted with the confirmation box.

[spencerfinnell]

This allows some of my custom meta boxes that I've been testing to continue working as before. 👍

[ghost]

you're not prompted with the confirmation box.

If that means it might happen to loose a ton of edited content by accident with one wrong click, i don't think that's an option.

If it is not possible to detect a dirty state correctly, i would instead prefer to be always prompted, even if there is no change at all.

[youknowriad]

If that means it might happen to loose a ton of edited content by accident with one wrong click, i don't think that's an option.

That's how the current editor works, and it doesn't seem to bother anyone. The confirmation box is still enabled for changes to the post content and the post settings though.

[ghost]

That's how the current editor works

If so, that is bad and should be fixed.

it doesn't seem to bother anyone

Except all the users who lost their edits.

Using ACF Pro, there is obviously an inconsistent behaviour, changes in most field types cause a prompt,
but some do not (simple textfield for example).

It should at least be documented what plugin authors have to do to cause a prompt (something like a setDirty() call?)

[youknowriad]

It should at least be documented what plugin authors have to do to cause a prompt (something like a setDirty() call?)

Sounds like a good idea to add an API like that. I think it's out of the scope of the current PR thought since this brings us to a similar behavior to the current Editor.

[youknowriad]

Can I have some thoughts here @aduth @mtias @pento

[aduth]

Due to how MetaBoxes work it's not possible to dirty-check them properly.

Is it possible to check whether there are "unsaved" changes during the beforeunload event? Unsaved meaning we serialize metabox forms when first loading, then compare serialized form values when user tries to leave?

(I should perhaps note I don't have the clearest understanding of the meta boxes behaviors)

That said, if it's prohibitively difficult, my preference would be toward resolving the bugs of #4183 and #3975 even if it means losing the dirtiness prompts.

[youknowriad]

Is it possible to check whether there are "unsaved" changes during the beforeunload event? Unsaved meaning we serialize metabox forms when first loading, then compare serialized form values when user tries to leave?

Good proposal, I might try it, it may be difficult to achieve though since the "state" of the metaboxes is dispatched through several areas and if the areas are not visible, the DOM is in a hidden node.

But it has a downside. Some metaboxes come with default values, even if they do not have any change, they are still "dirty" (needs to be saved)

[aduth]

Some metaboxes come with default values, even if they do not have any change, they are still "dirty" (needs to be saved)

I'd agree these are "dirty" only in that they need to be included in the save payload, but wouldn't agree that they'd need to stop the user from navigating away. If we're not already, couldn't we just send the entire metabox form of fields in the save request, whether or not any changes have been made?

[youknowriad]

If we're not already, couldn't we just send the entire metabox form of fields in the save request, whether or not any changes have been made?

Yes, that's what I'm doing in this PR alongside the the removal of the dirty checking to prevent leaving the editor (which matchers the old editor's behavior).

I'll try to update the PR to make it work similarily to what you propose. But the only way to make it behave properly is to avoid relying on the "onChange" event and check the state when the user wants to leave the page because we can't catch updates done programmatically with onChange events.

[youknowriad]

@aduth I added the check in 35276f0 I'm not totally satisfied with the code but can't think of a good alternative right now

[aduth]

I don't love the DOM querying where we expect to know the class structure of meta box elements. The only idea I can think of as an alternative is a pattern similar to slots where dirtyable areas (components) can register themselves with UnsavedChangesWarning, and the initial HTML is tracked when the (dirtyable) component first mounts. At this point there'd be no need for the Redux property tracking.

Can you explain what it is about the changes here that resolves #4183?

[aduth]

Typo: I'd expect singular form of metaBoxes to be metaBox

[aduth]

Minor: This can be simplified using Lodash property matchers:

return some( getMetaBoxes( state ), 'isActive' );

[aduth]

We should clear the cache of isSavingMetaBoxes before each test.

[aduth]

Might recommend splitting this long line:

return (
	isEditedPostDirty( state ) ||
	hasMetaBoxes( state ) ||
	[ 'publish', 'private', 'future' ].indexOf( post.status ) === -1
);

Minor: I like semantic readability of Lodash's _.includes:

return (
	isEditedPostDirty( state ) ||
	hasMetaBoxes( state ) ||
	! includes( [ 'publish', 'private', 'future' ], post.status )
);

[aduth]

Minor: Typo: Needs space between "not" and "currently"

[aduth]

Same note as previous.

[aduth]

When would we expect this line to be reached? (Can we add a comment explaining this?)

[aduth]

And if we're expecting the first query selector might return an empty result, is there any reason to think this one might as well?

[youknowriad]

The metaboxes are rendered to an initial hidden dom node (the later here) but when their panel is visible (it's not always the case for the sidebar), they are moved to their new location and when closed moved back to the original location.

[aduth]

Rather than calling into DOM from the reducer:

• Could the MetaBoxesArea component dispatch a "set HTML" action when it mounts?
• Should this at least be modeled as an effect? (Minding that with Framework: Bump refx to 3.x #3983 we should respect that effects occur after the original action)

[aduth]

Seems this.originalFormData is defunct now and can be removed, along with getFormData.

[youknowriad]

@aduth this closes #4183 because we never check for dirtyness before saving the metaboxes, each time we save a post, we save them. So by default the save button is visible.

[aduth]

@aduth this closes #4183 because we never check for dirtyness before saving the metaboxes, each time we save a post, we save them. So by default the save button is visible.

My reading of #4183 is not related to the Save button being unavailable, but rather that the default values aren't included in the payload of the save request.

[youknowriad]

@aduth I think the request for saving the metaboxes is not triggered at all because they are considered "non-dirty"

[youknowriad]

Ok, I think the PR is better now. I updated it. It still uses the DOM direct access. It's not great but since the metaboxes are kind of "weird chunk fo HTML" we have to deal with, it's acceptable IMO.
I added a comment to clarify its behavior.

Also,

• I moved the save behavior of the metaboxes to an affect
• If we have multiple areas, they're all saved together now fixing the bug where hidden metaboxes were not saved.
• Some usage of jQuery.serialize to merge the different forms (I prefer this over having a custom code).

What do y'all think?

[aduth]

I'm generally fine with the direction here.

[aduth]

Unrelated: When I see bindNode, I assume it's assigning the ref of the root return value, not a child of the element. A minor thing, but I think it can be nice to clarify in the function name, e.g. bindContainerNode.

[aduth]

Not sure if we care to be guarded here, but if I pass an invalid location prop, this will error.

[aduth]

I think there was a faulty rebase here, I wouldn't expect these doc changes (i.e. we don't want the extra whitespace here, or in the @returns below)

[aduth]

Unrelated: I prefer when the action creators are prefixed with the verb of what's happening, i.e. "set", setMetaBoxSavedData

[aduth]

Could be DRY'd up a bit with the INITIALIZE_META_BOX_STATE effect logic.

[youknowriad]

Not so simple because the effect is triggering before the action. Didn't you make an update to refx to update this?

[aduth]

Not so simple because the effect is triggering before the action. Didn't you make an update to refx to update this?

Yes: #3983

[aduth]

Extra space before the } is unexpected (here and in the DocBlock below)

[aduth]

This line is unnecessary.

[aduth]

Personally I don't find this comment to be particularly helpful beyond pointing out that it's a reducer, and would encourage elaborating on the purpose for why we track a meta box saving state.

[aduth]

I don't see a rule for it, but I'd conventionally have expected if and endif statements for conditions / loops within a PHP template:

Note that requiring the use of braces just means that single-statement inline control structures are prohibited. You are free to use the alternative syntax for control structures (e.g. if/endif, while/endwhile)—especially in your templates where PHP code is embedded within HTML, for instance:

<?php if ( have_posts() ) : ?>
   <div class="hfeed">
       <?php while ( have_posts() ) : the_post(); ?>
           <article id="post-<?php the_ID() ?>" class="<?php post_class() ?>">
               <!-- ... -->
           </article>
       <?php endwhile; ?>
   </div>
<?php endif; ?>

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

[youknowriad]

Any other concern here?