Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try split content view with open metaboxes #64351

Merged
merged 18 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 120 additions & 7 deletions packages/edit-post/src/components/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ import {
} from '@wordpress/block-editor';
import { PluginArea } from '@wordpress/plugins';
import { __, sprintf } from '@wordpress/i18n';
import { useCallback, useMemo } from '@wordpress/element';
import {
useCallback,
useLayoutEffect,
useMemo,
useRef,
} from '@wordpress/element';
import { store as noticesStore } from '@wordpress/notices';
import { store as preferencesStore } from '@wordpress/preferences';
import {
Expand All @@ -36,8 +41,8 @@ import { privateApis as blockLibraryPrivateApis } from '@wordpress/block-library
import { addQueryArgs } from '@wordpress/url';
import { decodeEntities } from '@wordpress/html-entities';
import { store as coreStore } from '@wordpress/core-data';
import { SlotFillProvider } from '@wordpress/components';
import { useViewportMatch } from '@wordpress/compose';
import { ResizableBox, SlotFillProvider } from '@wordpress/components';
import { useMediaQuery, useViewportMatch } from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -151,6 +156,117 @@ function useEditorStyles() {
] );
}

/**
* @param {Object} props
* @param {boolean} props.isLegacy True when the editor canvas is not in an iframe.
*/
function MetaBoxesMain( { isLegacy } ) {
const [ isOpen, openHeight, hasAnyVisible ] = useSelect( ( select ) => {
const { get } = select( preferencesStore );
const { isMetaBoxLocationVisible } = select( editPostStore );
return [
get( 'core/edit-post', 'metaBoxesMainIsOpen' ),
get( 'core/edit-post', 'metaBoxesMainOpenHeight' ),
isMetaBoxLocationVisible( 'normal' ) ||
isMetaBoxLocationVisible( 'advanced' ),
];
}, [] );
const { set: setPreference } = useDispatch( preferencesStore );
const resizableBoxRef = useRef();
const isShort = useMediaQuery( '(max-height: 549px)' );

const isAutoHeight = openHeight === undefined;
// In case a user size is set stops the default max-height from applying.
useLayoutEffect( () => {
if ( ! isLegacy && hasAnyVisible && ! isShort && ! isAutoHeight ) {
resizableBoxRef.current.resizable.classList.add( 'has-user-size' );
}
}, [ isAutoHeight, isShort, hasAnyVisible, isLegacy ] );

if ( ! hasAnyVisible ) {
return;
}

const className = 'edit-post-meta-boxes-main';
const contents = (
<div
className={ clsx(
// The class name 'edit-post-layout__metaboxes' is retained because some plugins use it.
'edit-post-layout__metaboxes',
! isLegacy && 'edit-post-meta-boxes-main__liner'
) }
>
<MetaBoxes location="normal" />
<MetaBoxes location="advanced" />
</div>
);

if ( isLegacy ) {
return contents;
}

if ( isShort ) {
return (
<details
className={ className }
open={ isOpen }
onToggle={ ( { target } ) => {
setPreference(
'core/edit-post',
'metaBoxesMainIsOpen',
target.open
);
} }
>
<summary>{ __( 'Meta Boxes' ) }</summary>
{ contents }
</details>
);
}
return (
<ResizableBox
className={ className }
defaultSize={ { height: openHeight } }
ref={ resizableBoxRef }
enable={ {
top: true,
right: false,
bottom: false,
left: false,
topLeft: false,
topRight: false,
bottomRight: false,
bottomLeft: false,
} }
// This is overriden by an !important rule that applies until user resizes.
maxHeight="100%"
Comment on lines +242 to +243
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The same style is defined here, so is this prop unnecessary?

Copy link
Contributor Author

@stokesman stokesman Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a brief retest, neither the prop nor the rule in CSS seem necessary 😅. The branch I have that adds keyboard support uses this prop with specified pixel value (for reasons which which I’ll elaborate in its PR) and I’ll leave this be here. On the other maybe we can remember to check if the CSS rule has any purpose.

bounds="parent"
boundsByDirection
// Avoids hiccups while dragging over objects like iframes and ensures that
// the event to end the drag is captured by the target (resize handle)
// whether or not it’s under the pointer.
onPointerDown={ ( { pointerId, target } ) => {
target.setPointerCapture( pointerId );
} }
onResizeStart={ ( event, direction, elementRef ) => {
// Avoids height jumping in case it’s limited by max-height.
elementRef.style.height = `${ elementRef.offsetHeight }px`;
// Stops initial max-height from being applied.
elementRef.classList.add( 'has-user-size' );
} }
onResizeStop={ () => {
setPreference(
'core/edit-post',
'metaBoxesMainOpenHeight',
resizableBoxRef.current.state.height
);
} }
>
{ contents }
</ResizableBox>
);
}

function Layout( {
postId: initialPostId,
postType: initialPostType,
Expand Down Expand Up @@ -355,10 +471,7 @@ function Layout( {
extraContent={
! isDistractionFree &&
showMetaBoxes && (
<div className="edit-post-layout__metaboxes">
<MetaBoxes location="normal" />
<MetaBoxes location="advanced" />
</div>
<MetaBoxesMain isLegacy={ ! shouldIframe } />
)
}
>
Expand Down
70 changes: 67 additions & 3 deletions packages/edit-post/src/components/layout/style.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,70 @@
.edit-post-layout__metaboxes {
flex-shrink: 0;
clear: both;
.edit-post-meta-boxes-main {
filter: drop-shadow(0 -1px rgba($color: #000, $alpha: 0.133)); // 0.133 = $gray-200 but with alpha.
background-color: $white;
clear: both; // This is seemingly only needed in case the canvas is not iframe’d.

&:not(details) {
padding-top: 23px;
max-height: 100%;

&:not(.has-user-size) {
max-height: 50% !important;
}
}

// The component renders as a details element in short viewports.
&:is(details) {
& > summary {
cursor: pointer;
color: $gray-900;
background-color: $white;
height: $button-size-compact;
line-height: $button-size-compact;
font-size: 13px;
padding-left: $grid-unit-30;
box-shadow: 0 $border-width $gray-300;
}

&[open] > summary {
position: sticky;
top: 0;
z-index: 1;
}
}

& .components-resizable-box__handle-top {
top: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than overwriting the existing handle with CSS, as implemented here, how about defining a custom handle?

If we use a button as the handle and add a keyDown event, we should be able to focus the resize handle with the keyboard, making it operable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d thought this too and even think it’d be necessary but hoped to spare some effort while uncertain if this PR will garner favor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the PR is approved, would it be a good time to apply @t-hamano 's advice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve got a branch that does it. It’s looking like about 160 insertions(+), 40 deletions(-) minimum from this branch so not too trivial. There are also design/UX considerations so a clean thread would be nice for any discussion. I’m thinking a separate PR. We can either merge to this one or to trunk if we land this before then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good — feel free to ping on that separate PR

box-shadow: 0 $border-width $gray-300;
}
& .components-resizable-box__side-handle::before {
border-radius: 0;
top: 0;
height: $border-width;
}
& .components-resizable-box__handle::after {
background-color: $gray-300;
box-shadow: none;
border-radius: 4px;
height: $grid-unit-05;
top: calc(50% - #{$grid-unit-05} / 2);
width: 100px;
right: calc(50% - 50px);
}
}

.edit-post-meta-boxes-main__liner {
overflow: auto;
max-height: 100%;
// Keep the contents behind the resize handle or details summary.
isolation: isolate;
}

.has-metaboxes .editor-visual-editor {
flex: 1;

&.is-iframed {
isolation: isolate;
}
}

// Adjust the position of the notices
Expand Down
11 changes: 2 additions & 9 deletions packages/edit-post/src/components/layout/use-should-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,13 @@ import { useSelect } from '@wordpress/data';
import { store as blocksStore } from '@wordpress/blocks';
import { store as blockEditorStore } from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import { store as editPostStore } from '../../store';

const isGutenbergPlugin = globalThis.IS_GUTENBERG_PLUGIN ? true : false;

export function useShouldIframe() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case the iframe is not used now? Can't we just remove this hook entirely

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replying to myself: I'm guessing if there are non v3 blocks. I think we should probably reconsider this decision as well. People had time to migrate and we can address this differently now. (Error boundaries for this kind of blocks, or a behavior similar to the classic block). But we should explore this in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also WP.com users have been using iframe for all blocks (regardless of version) for block themes for a while. I think that represents a big number of users already, so I'm confident that there are very few blocks that actually break because of the iframe these days.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, when the post editor became an iframe in block themes (#46212), plugin developers reported issues (e.g. #47924). So the block API version 3 was introduced and the post editor was changed to not become an iframe, at least when v1/v2 blocks were registered (#48286).

And this PR doesn't just affect blocks: in fact, as mentioned later in this comment, it seems there are still some well-known plugins that don't support the iframe editor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this PR doesn't just affect blocks: in fact, as mentioned later in #64351 (comment), it seems there are still some well-known plugins that don't support the iframe editor.

Do you know if these plugins actually don't support the iframe or just register blocks < 3. My suspicion is that most of the plugins that have blocks < v3 support the iframe but have trouble upgrading for other reasons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if these plugins actually don't support the iframe or just register blocks < 3.

It seems that these plugins actually don't support the iframe. Please see the video at the end of this comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this about the "link" format?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it's not really about blocksV3, it seems like a separate discussion and these plugins seems like they're already broken for most new WP installs / site editor...

Copy link
Contributor

@t-hamano t-hamano Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. From what I can see, these plugins don't enable their own link formatting outside the post editor.

So the place these plugins don't actually work is in the mobile/tablet views which always run as an iframe editor. With this PR the desktop view will also run as an iframe editor, making the issue more apparent.

const {
isBlockBasedTheme,
hasV3BlocksOnly,
isEditingTemplate,
hasMetaBoxes,
isZoomOutMode,
} = useSelect( ( select ) => {
const { getEditorSettings, getCurrentPostType } = select( editorStore );
Expand All @@ -31,14 +25,13 @@ export function useShouldIframe() {
return type.apiVersion >= 3;
} ),
isEditingTemplate: getCurrentPostType() === 'wp_template',
hasMetaBoxes: select( editPostStore ).hasMetaBoxes(),
isZoomOutMode: __unstableGetEditorMode() === 'zoom-out',
};
}, [] );

return (
( ( hasV3BlocksOnly || ( isGutenbergPlugin && isBlockBasedTheme ) ) &&
! hasMetaBoxes ) ||
hasV3BlocksOnly ||
( isGutenbergPlugin && isBlockBasedTheme ) ||
isEditingTemplate ||
isZoomOutMode
);
Expand Down
1 change: 0 additions & 1 deletion packages/edit-post/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,6 @@ export const isMetaBoxLocationVisible = createRegistrySelector(
isMetaBoxLocationActive( state, location ) &&
getMetaBoxesPerLocation( state, location )?.some( ( { id } ) => {
return select( editorStore ).isEditorPanelEnabled(
state,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was making isMetaBoxLocationVisible unusable. Seemed a simple enough fix to just include it here instead of making a separate PR.

`meta-box-${ id }`
);
} )
Expand Down
18 changes: 11 additions & 7 deletions test/e2e/specs/editor/plugins/meta-boxes.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test.describe( 'Meta boxes', () => {
await expect( saveDraft ).toBeDisabled();

// Add title to enable valid non-empty post save.
await page
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'Hello Meta' );

Expand All @@ -44,7 +44,7 @@ test.describe( 'Meta boxes', () => {
page,
} ) => {
// Publish a post so there's something for the latest posts dynamic block to render.
await page
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'A published post' );
await page.keyboard.press( 'Enter' );
Expand All @@ -53,7 +53,7 @@ test.describe( 'Meta boxes', () => {

// Publish a post with the latest posts dynamic block.
await admin.createNewPost();
await page
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'Dynamic block test' );
await editor.insertBlock( { name: 'core/latest-posts' } );
Expand All @@ -70,10 +70,12 @@ test.describe( 'Meta boxes', () => {
editor,
page,
} ) => {
await page
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'A published post' );
await page.getByRole( 'button', { name: 'Add default block' } ).click();
await editor.canvas
.getByRole( 'button', { name: 'Add default block' } )
.click();
await page.keyboard.type( 'Excerpt from content.' );

const postId = await editor.publishPost();
Expand All @@ -89,9 +91,11 @@ test.describe( 'Meta boxes', () => {
page,
} ) => {
await editor.openDocumentSettingsSidebar();
await page.getByRole( 'button', { name: 'Add default block' } ).click();
await editor.canvas
.getByRole( 'button', { name: 'Add default block' } )
.click();
await page.keyboard.type( 'Excerpt from content.' );
await page
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'A published post' );

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/editor/plugins/wp-editor-meta-box.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ test.describe( 'WP Editor Meta Boxes', () => {
await admin.createNewPost();

// Add title to enable valid non-empty post save.
await page
await editor.canvas
.locator( 'role=textbox[name="Add title"i]' )
.type( 'Hello Meta' );

Expand Down
7 changes: 3 additions & 4 deletions test/e2e/specs/editor/various/publish-button.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,12 @@ test.describe( 'Post publish button', () => {
admin,
page,
requestUtils,
editor,
} ) => {
await requestUtils.activatePlugin( 'gutenberg-test-plugin-meta-box' );
await admin.createNewPost();
await page
.getByRole( 'textbox', {
name: 'Add title',
} )
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'Test post' );

const topBar = page.getByRole( 'region', { name: 'Editor top bar' } );
Expand Down
Loading