Fix metabox reordering #30617

ribaricplusplus · 2021-04-08T14:21:09Z

Description

Important: This PR also includes changes to WordPress core. I will submit those here: https://core.trac.wordpress.org/ticket/52818

Cause of issue:

postboxes.js wp-admin script (this one), which is responsible for saving meta box order, is initialized by setAvailableMetaBoxesPerLocation here:

gutenberg/packages/edit-post/src/store/actions.js

Lines 279 to 281 in 8aa446b

    
           if ( window.postboxes.page !== postType ) { 
        
           	window.postboxes.add_postbox_toggles( postType ); 
        
           }

However, that function is called from post.php, here:

https://github.com/WordPress/WordPress/blob/57da3e781320dd62e3016624b310c23447d9e6d1/wp-admin/includes/post.php#L2379-L2381

But at that point the editor is not yet fully initialized because setupEditor action didn't run yet. So postType is still null, postboxes becomes initialized incorrectly (with null instead of proper post type), and then when reordering happens the reordered metaboxes are saved incorrectly.

How has this been tested?

Manually. Try changing metabox order. Steps to reproduce are given in the linked issue.

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.

I'm not sure if anything needs to be changed for React Native:

I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

The text was updated successfully, but these errors were encountered:

peterwilsoncc

Confirmed this fixes the bug with some manual testing. Also checked the postType is passed as a string on trunk with some well placed console.logs and that is the case too.

peterwilsoncc · 2021-04-30T05:02:35Z

packages/edit-post/src/editor.js

@@ -102,6 +106,7 @@ function Editor( {
 					? getEditedPostTemplate()
 					: null,
 			post: postObject,
+			isEditorReady: __unstableIsEditorReady(),


Reading the coding standards and wondering if this should be returned with the __unstable prefix too.

What am I missing? This is mainly for my own education rather than an opinion either way :)

I was following the style established elsewhere in the code where __unstable and __experimental is often removed, for example here:

gutenberg/packages/edit-post/src/components/layout/index.js

Lines 17 to 25 in 04c7c76

import {

BlockBreadcrumb,

__experimentalLibrary as Library,

} from '@wordpress/block-editor';

import { Button, ScrollLock, Popover } from '@wordpress/components';

import {

useViewportMatch,

__experimentalUseDialog as useDialog,

} from '@wordpress/compose';

Also, if I'm not mistaken, it seems that the __unstable and __experimental prefix is there for people using Gutenberg packages in their projects, so it's important not to document these functions and tell people to use them and so forth. Here it's just used locally and not exported anywhere.

That's just my reasoning for renaming it, but I'll put the __unstable prefix back if you think that would be better.

paaljoachim · 2021-04-30T19:02:52Z

I believe this PR could use some additional eyes...
@mcsf @ockham @youknowriad

youknowriad · 2021-05-03T08:52:17Z

Hi there! thanks for the fix here, it seems to me that the current way this call is handled could be improved, and this is highlighted by the issue being fixed here.

In other words and to avoid any timing issues in the future, what about a solution like this:

1- Remove the promise from here https://github.com/WordPress/WordPress/blob/57da3e781320dd62e3016624b310c23447d9e6d1/wp-admin/includes/post.php#L2379-L2381 and just call setAvailableMetaBoxesPerLocation action synchronously.

2- Split the setAvailableMetaBoxesPerLocation action creator to two separate action steps:
a- one that sets the state in the store (saves the active meta boxes)
b- one that triggers the post box script and also enables the meta boxes saving: all this code

gutenberg/packages/edit-post/src/store/actions.js

Lines 275 to 322 in 8aa446b

    
           const postType = yield controls.select( 
        
           	'core/editor', 
        
           	'getCurrentPostType' 
        
           ); 
        
           if ( window.postboxes.page !== postType ) { 
        
           	window.postboxes.add_postbox_toggles( postType ); 
        
           } 
        
           let wasSavingPost = yield controls.select( 'core/editor', 'isSavingPost' ); 
        
           let wasAutosavingPost = yield controls.select( 
        
           	'core/editor', 
        
           	'isAutosavingPost' 
        
           ); 
        
           // Meta boxes are initialized once at page load. It is not necessary to 
        
           // account for updates on each state change. 
        
           // 
        
           // See: https://github.com/WordPress/WordPress/blob/5.1.1/wp-admin/includes/post.php#L2307-L2309 
        
           const hasActiveMetaBoxes = yield controls.select( 
        
           	editPostStore.name, 
        
           	'hasMetaBoxes' 
        
           ); 
        
           // First remove any existing subscription in order to prevent multiple saves 
        
           if ( !! saveMetaboxUnsubscribe ) { 
        
           	saveMetaboxUnsubscribe(); 
        
           } 
        
           // Save metaboxes when performing a full save on the post. 
        
           saveMetaboxUnsubscribe = subscribe( () => { 
        
           	const isSavingPost = select( 'core/editor' ).isSavingPost(); 
        
           	const isAutosavingPost = select( 'core/editor' ).isAutosavingPost(); 
        
           	// Save metaboxes on save completion, except for autosaves that are not a post preview. 
        
           	const shouldTriggerMetaboxesSave = 
        
           		hasActiveMetaBoxes && 
        
           		wasSavingPost && 
        
           		! isSavingPost && 
        
           		! wasAutosavingPost; 
        
           	// Save current state for next inspection. 
        
           	wasSavingPost = isSavingPost; 
        
           	wasAutosavingPost = isAutosavingPost; 
        
           	if ( shouldTriggerMetaboxesSave ) { 
        
           		dispatch( editPostStore.name ).requestMetaBoxUpdates(); 
        
           	} 
        
           } );

c- when setting the metaboxes, only call the second part if the editor is ready

3- Add an effect somewhere in the MetaBoxes component that monitors __unstableIsEditorReady and calls the action defined in b above when the flag is set to true.

The main reason I'm suggesting this is to make the code less dependent on the timing of the actions being called. Does that sound like a better solution?

paaljoachim · 2021-05-04T06:40:28Z

Associated PR:
Allow moving metaboxes to previously empty area
#25187

ribaricplusplus · 2021-05-11T05:58:40Z

Thank you for the review @youknowriad.

Your suggestions sound good. However, I realized that I will not be able to invest time into this PR for at least a month. Just wanted to let you know. If it's still relevant when I can invest time in it, I'll do it.

desrosj · 2021-07-28T12:33:25Z

Hey @ribaricplusplus, just wanted to see if you had some time that freed up to revise this PR. I'd love to wrap this one up and get it shipped in a 5.8.x release of WordPress.

rodrigo-arias · 2021-07-28T19:57:08Z

Hi, I'm having a similar issue with ACF that I think has the same origin. After inserting a taxonomy I cannot open the meta boxes. The error doesn't happen in WP 5.6.x

I've seen a similar report here: https://core.trac.wordpress.org/ticket/52818

It would be great if you could have this fix for the next minor version as right now it is hard to use the block editor since WP 5.7 with this bug.

Thank you

ribaricplusplus · 2021-07-28T20:49:04Z

I haven't forgotten about this, but finding time is a struggle!

I'll update you in the next few days about when I'll be able to work on this.

ribaricplusplus · 2021-08-01T11:51:42Z

I'll try to do it mid September.

ribaricplusplus · 2021-08-27T00:05:01Z

@youknowriad Hey. I've made the changes as you proposed, the patch to WordPress core is here: https://core.trac.wordpress.org/ticket/52818

Please review this again and let me know if something still needs to be changed.

packages/edit-post/src/components/meta-boxes/index.js

youknowriad

This is looking very good to me, I'd appreciate some testing from folks relying on meta boxes a bit.

I assume this works even without the Core patch right (as we're supposed to support existing and previous WP versions in the Gutenberg plugin)

ribaricplusplus · 2021-08-29T13:10:03Z

I assume this works even without the Core patch right

@youknowriad Yes.

I'd appreciate some testing from folks relying on meta boxes a bit.

@desrosj Can you mention anyone who could potentially do some testing?

ribaricplusplus · 2021-08-29T17:23:57Z

@youknowriad I just realized that my previous solution was actually not good because the MetaBoxes component is created multiple times for different values of the location parameter. So the effect that initializes meta boxes would also run multiple times.

Now I extracted the meta box initializing logic into a separate action on the edit-post store.

youknowriad · 2021-08-30T08:15:01Z

packages/edit-post/src/store/selectors.js

+ *
+ * @return {boolean} Whether meta boxes are initialized.
+ */
+export function metaBoxesInitialized( state ) {


Can we maybe rename this one areMetaBoxesInitialized, hasInitializedMetaboxes or something like that to match the naming scheme of the other selectors?

youknowriad

Still looking good. Good point on the extraction to an action + isInitialized check.

youknowriad · 2021-08-30T08:58:35Z

Thanks for your work and persistence here :)

dariaknl · 2021-09-01T11:34:11Z

Tested on a last checkout from trunk: the re-ordering bug from the original issue is not reproduced. Re-ordering is kept after updating the post.

* Fix API docs generation (#33384) * Docs: use markdown headings instead of links for API declarations (#33381) * Docs: Run Prettier after updating API in documentation (#33498) (cherry picked from commit 626f233) * Use tabs instead of spaces in block transform doc example (#33549) (cherry picked from commit 8afca1e) * Fix metabox reordering (#30617). * Block editor: don't render layout and duotone styles in between blocks (#32083) * Widgets: Allow HTML tags in description (#33814) * Widgets: Allow HTML tags in description * Use `dangerouslySetInnerHTML` Avoid `<div />` inside the `<p />` tag * Describe by dangerouslySetInnerHTML is used * Use safeHTML * Update comment * Editor: Set 'hide_empty' for the most used terms query (#33457) Don't include terms that aren't assigned to any posts as "most used" terms. * Update widget editor help links to point to the new support article (#33482) * If select-all fires in .editor-post-title__input, end the process.. (#33621) * Writing flow: select all: remove early return for post title (#33699) * Call onChangeSectionExpanded conditionally (#33618) * FontSizePicker: Use number values when the initial value is a number (#33679) * FontSizePicker: Don't use units if the value is a number * Add unit tests * Disable units when we have number values * Fix justification for button block when selected (#33739) * Remove margin setting, auto right conflict with justify buttons * Per review, add little margin back * Add error boundaries to widget screens (#33771) * Add error boundary to edit widgets screen * Add error boundary to customize widgets * Refactor sidebar controls provider to application level so that its state is not lost when re-initializing * Revert "Refactor sidebar controls provider to application level so that its state is not lost when re-initializing" This reverts commit 7d607ff. * Remove rebootability from customize widgets * Remove debug code * Fix insertion point in Widgets editors (#33802) * Default batch processor: Respect the batch endpoint's maxItems (#34280) This updates the default batch processor to make multiple batch requests if the number of requests to process exceeds the number of requests that the batch endpoint can handle. We determine the number of requests that the batch endpoint can handle by making a preflight OPTIONS request to /batch/v1. By default it is 25 requests. See https://make.wordpress.org/core/2020/11/20/rest-api-batch-framework-in-wordpress-5-6/. * Fix button block focus trap after a URL has been added (#34314) * Rework button block link UI to match RichText format implementation * Refine some more, determine visibility by selection and url state * Add e2e test * Also focus rich text when unlinking using a keyboard shortcut * Text for dropdown fields within legacy widgets in the Customizer is off centered (#34076) * Fix ESLint errors reported * Regenerate autogenerated docs * Update the `INSERTER_SEARCH_SELECTOR` path. This is a partial cherry pick of 2356b2d in order to fix the performance tests. Co-authored-by: André <[email protected]> Co-authored-by: JuanMa <[email protected]> Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: Jeff Bowen <[email protected]> Co-authored-by: Bruno Ribarić <[email protected]> Co-authored-by: Ella van Durpe <[email protected]> Co-authored-by: Petter Walbø Johnsgård <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Hiroshi Urabe <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Marcus Kazmierczak <[email protected]> Co-authored-by: Robert Anderson <[email protected]> Co-authored-by: Anton Vlasenko <[email protected]>

ribaricplusplus added 7 commits Apr 8, 2021

Fix metabox reordering

c432edc

Cleanup

91ca744

Merge branch 'trunk' into fix/metabox-order

fe02d17

Cleanup

a30bd3c

Formatting

68e70f4

Fix typo

d36f3c2

Update docs

eb73aff

ribaricplusplus marked this pull request as ready for review Apr 8, 2021

ribaricplusplus added [Feature] Meta Boxes [Type] Bug labels Apr 8, 2021

peterwilsoncc reviewed Apr 30, 2021

View changes

peterwilsoncc mentioned this pull request Apr 30, 2021

Meta box reordering does not save in user preferences #30071

Closed

paaljoachim mentioned this pull request May 4, 2021

Allow moving metaboxes to previously empty area - fixes #7960 #25187

Open

6 tasks

ribaricplusplus added 3 commits Jul 30, 2021

Merge branch 'trunk' into fix/metabox-order

0ac660d

Formatting

3a263c6

Merge branch 'trunk' into fix/metabox-order

97c3a01

ribaricplusplus added 2 commits Aug 26, 2021

Merge branch 'trunk' into fix/metabox-order

bfa0820

Improvements

a24dcbc

ribaricplusplus added 2 commits Aug 27, 2021

Don't unsubscribe

47ffc13

Refactoring

f86617e

youknowriad reviewed Aug 27, 2021

View changes

packages/edit-post/src/components/meta-boxes/index.js Outdated Show resolved Hide resolved

youknowriad reviewed Aug 27, 2021

View changes

packages/edit-post/src/components/meta-boxes/index.js Outdated Show resolved Hide resolved

youknowriad reviewed Aug 27, 2021

View changes

packages/edit-post/src/components/meta-boxes/index.js Show resolved Hide resolved

youknowriad reviewed Aug 27, 2021

View changes

ribaricplusplus added 3 commits Aug 29, 2021

Use useRef instead of useState for initialized variable

4f67de1

Use useRegistry

521d072

Add location dependency

9f19aec

Meta boxes must be initialized only once

361c2af

ribaricplusplus requested review from ajitbohra and chrisvanpatten as code owners Aug 29, 2021

ribaricplusplus added 3 commits Aug 29, 2021

Better dispatch check

342361c

Add documentation for new action

0d362b8

Better dispatch check

1d64a78

paaljoachim added the Backport to WP Minor Release label Aug 29, 2021

youknowriad reviewed Aug 30, 2021

View changes

Rename selector

c96590a

youknowriad approved these changes Aug 30, 2021

View changes

youknowriad merged commit 893b2b8 into WordPress:trunk Aug 30, 2021
19 checks passed

github-actions bot added this to the Gutenberg 11.5 milestone Aug 30, 2021

ribaricplusplus deleted the fix/metabox-order branch Aug 30, 2021

desrosj added a commit that referenced this issue Aug 30, 2021

Fix metabox reordering (#30617).

c985f56

desrosj mentioned this pull request Aug 30, 2021

WordPress 5.8.1 backports #34393

Merged

7 tasks

WordPress / gutenberg Public

Fix metabox reordering #30617

Fix metabox reordering #30617

ribaricplusplus commented Apr 8, 2021 •

edited

peterwilsoncc left a comment

peterwilsoncc Apr 30, 2021

ribaricplusplus Apr 30, 2021

paaljoachim commented Apr 30, 2021

youknowriad commented May 3, 2021

paaljoachim commented May 4, 2021

ribaricplusplus commented May 11, 2021 •

edited

desrosj commented Jul 28, 2021

rodrigo-arias commented Jul 28, 2021

ribaricplusplus commented Jul 28, 2021

ribaricplusplus commented Aug 1, 2021

ribaricplusplus commented Aug 27, 2021 •

edited

youknowriad left a comment

ribaricplusplus commented Aug 29, 2021 •

edited

ribaricplusplus commented Aug 29, 2021 •

edited

youknowriad Aug 30, 2021

ribaricplusplus Aug 30, 2021

youknowriad left a comment

youknowriad commented Aug 30, 2021

dariaknl commented Sep 1, 2021

	import {
	BlockBreadcrumb,
	__experimentalLibrary as Library,
	} from '@wordpress/block-editor';
	import { Button, ScrollLock, Popover } from '@wordpress/components';
	import {
	useViewportMatch,
	__experimentalUseDialog as useDialog,
	} from '@wordpress/compose';

WordPress / gutenberg Public

Fix metabox reordering #30617

Fix metabox reordering #30617

Conversation

ribaricplusplus commented Apr 8, 2021 • edited

Description

How has this been tested?

Types of changes

Checklist:

peterwilsoncc left a comment

peterwilsoncc Apr 30, 2021

ribaricplusplus Apr 30, 2021

paaljoachim commented Apr 30, 2021

youknowriad commented May 3, 2021

paaljoachim commented May 4, 2021

ribaricplusplus commented May 11, 2021 • edited

desrosj commented Jul 28, 2021

rodrigo-arias commented Jul 28, 2021

ribaricplusplus commented Jul 28, 2021

ribaricplusplus commented Aug 1, 2021

ribaricplusplus commented Aug 27, 2021 • edited

youknowriad left a comment

ribaricplusplus commented Aug 29, 2021 • edited

ribaricplusplus commented Aug 29, 2021 • edited

youknowriad Aug 30, 2021

ribaricplusplus Aug 30, 2021

youknowriad left a comment

youknowriad commented Aug 30, 2021

dariaknl commented Sep 1, 2021

ribaricplusplus commented Apr 8, 2021 •

edited

ribaricplusplus commented May 11, 2021 •

edited

ribaricplusplus commented Aug 27, 2021 •

edited

ribaricplusplus commented Aug 29, 2021 •

edited

ribaricplusplus commented Aug 29, 2021 •

edited