Split view with meta boxes even with legacy canvas #66706
+44
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +118 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d008922. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11906394135
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
stokesman
added
[Type] Bug
This was referenced Nov 4, 2024
This was referenced Nov 7, 2024
t-hamano
reviewed
Nov 15, 2024
| ) } | ||
| hidden={ ! isLegacy && isShort && ! isOpen } | ||
| // The class name 'edit-post-layout__metaboxes' is retained because some plugins use it. | ||
| className="edit-post-layout__metaboxes edit-post-meta-boxes-main__liner" |
There was a problem hiding this comment.
Can we combine two CSS classes into one?
There was a problem hiding this comment.
Well, I think since we have to keep edit-post-layout__metaboxes for the sake of plugins we could utilize only that one. Is that what you mean? I suppose that’d be fine.
There was a problem hiding this comment.
That's right. I thought it would be nice to remove the .edit-post-meta-boxes-main__liner selector and put these styles into the .edit-post-layout__metaboxes selector:
gutenberg/packages/edit-post/src/components/layout/style.scss
Lines 98 to 107 in d008922
Comment on lines
409
to
411
| // When there’s no iframe the canvas is the scrolling context and with the | ||
| // iframe, device previews may overflow vertically. | ||
| enableScroll={ disableIframe || deviceType !== 'Desktop' } |
There was a problem hiding this comment.
I'm wondering if there's a way to solve this at a higher level without adding a new prop to the BlockCanvas component. Here's a rough diff, but what do you think?
Details
diff --git a/packages/block-editor/src/components/block-canvas/index.js b/packages/block-editor/src/components/block-canvas/index.js
index ef70c4a26d..dd24a91970 100644
--- a/packages/block-editor/src/components/block-canvas/index.js
+++ b/packages/block-editor/src/components/block-canvas/index.js
@@ -1,8 +1,3 @@
-/**
- * External dependencies
- */
-import clsx from 'clsx';
-
/**
* WordPress dependencies
*/
@@ -38,7 +33,6 @@ export function ExperimentalBlockCanvas( {
styles,
contentRef: contentRefProp,
iframeProps,
- enableScroll,
} ) {
useBlockCommands();
const isTabletViewport = useViewportMatch( 'medium', '<' );
@@ -57,18 +51,10 @@ export function ExperimentalBlockCanvas( {
frameSize: '40px',
}
: {};
- const className = clsx(
- 'block-editor-block-canvas',
- enableScroll && 'is-scrollable'
- );
if ( ! shouldIframe ) {
return (
- <BlockTools
- __unstableContentRef={ localRef }
- className={ className }
- style={ { height } }
- >
+ <BlockTools __unstableContentRef={ localRef } style={ { height } }>
<EditorStyles
styles={ styles }
scope=":where(.editor-styles-wrapper)"
@@ -88,7 +74,6 @@ export function ExperimentalBlockCanvas( {
return (
<BlockTools
__unstableContentRef={ localRef }
- className={ className }
style={ { height, display: 'flex' } }
>
<Iframe
diff --git a/packages/block-editor/src/components/block-canvas/style.scss b/packages/block-editor/src/components/block-canva
s/style.scss
index 52c204407e..840ef3644c 100644
--- a/packages/block-editor/src/components/block-canvas/style.scss
+++ b/packages/block-editor/src/components/block-canvas/style.scss
@@ -1,4 +1,4 @@
-.block-editor-block-canvas.is-scrollable {
+.edit-post-visual-editor.is-scrollable {
overflow: auto;
// Applicable only when legacy (non-iframed).
diff --git a/packages/editor/src/components/visual-editor/index.js b/packages/editor/src/components/visual-editor/index.js
index 5a75355e08..122252ea4a 100644
--- a/packages/editor/src/components/visual-editor/index.js
+++ b/packages/editor/src/components/visual-editor/index.js
@@ -385,6 +385,7 @@ function VisualEditor( {
'has-padding': isFocusedEntity || enableResizing,
'is-resizable': enableResizing,
'is-iframed': ! disableIframe,
+ 'is-scrollable': disableIframe || deviceType !== 'Desktop',
}
) }
>
@@ -406,9 +407,6 @@ function VisualEditor( {
...deviceStyles,
},
} }
- // When there’s no iframe the canvas is the scrolling context and with the
- // iframe, device previews may overflow vertically.
- enableScroll={ disableIframe || deviceType !== 'Desktop' }
>
{ themeSupportsLayout &&
! themeHasDisabledLayoutStyles &&There was a problem hiding this comment.
Thanks for suggesting this. I’ve tried it. The only thing is actually having the visual editor’s root element as scrollable is trickier as it won’t contain the margins of the device previews. That’s because the block canvas’ root element has an inline-styled height of 100% (as specified by VisualEditor) and that’s important for fluid transitions to/from device previews. So to get the margins to be included in the overflow the height has to be auto while scrollable but then it breaks the transitions to a device preview.
Screen recording to demonstrate issues with VisualEditor as the scrolling context
visual-editor-scroll-challenges.mp4
Still, it looks like we can avoid adding a prop to ExperimentalBlockCanvas d008922. The component still needs a class added to target through CSS. It makes for what may be considered slightly worse style encapsulation because now the editor package is relying on a class applied in the block-editor package. For that reason, I think adding the prop may be slightly "cleaner" 🤷.
t-hamano
approved these changes
Nov 19, 2024
I happened to notice one more thing (that’s pretty obscure). On this branch if a device preview overflows horizontally then horizontal scrolling is possible. On trunk the device preview’s width shrinks to fit so there’s no horizontal scrolling but the canvas’s vertical scrollbar hides under the parent’s. In WP 6.6 the device preview shrinks to fit more completely—the device preview scrollbar remains visible. So there’s a related 6.7 regression. I found that the 6.6 behavior can be restored by this brach simply enough 2fdda78.
Besides that I cannot think of anything else. |
stokesman
commented
Nov 19, 2024
| @@ -60,7 +60,7 @@ export default function useResizeCanvas( deviceType ) { | |||
| marginLeft: marginHorizontal, | |||
| marginRight: marginHorizontal, | |||
| height, | |||
| overflowY: 'auto', | |||
There was a problem hiding this comment.
This dates back to #19082 which introduced device previewing and it seems at the time the canvas was not iframed. Now device previews are always iframed and its document is the scrolling context so this rule has no purpose.
|
We can punt it to a 6.7.2 that will be done after Thanksgiving, Black Friday and Cybermonday, events that are quite impactful for many websites. ping @stokesman @t-hamano |
cbravobernal
added
the
Backport to WP Minor Release
6 tasks
|
@stokesman The following issues may need to be resolved and backported to 6.7.2 before moving forward with this PR: |
|
👋🏻 Seeing that 6.7.2 is going to be delayed until mid to late January, is it possible to get this PR landed in the next GB version? Thank you! 🙇🏻 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Brings the split view introduced in #64351 to the non-iframed canvas.
Why?
The split view seems like a better UX since the meta boxes can be accessed without having to scroll the entire canvas. Also fixes the issues with device previews being obscured by the meta box pane—fixes #66629 (Though, a more minimal fix for 6.7 is in #66726).
Additionally:
How?
BlockCanvasconditionally a scrolling context withVisualEditorspecifying it as needed:Testing Instructions
These are not step-by-step but are all the things I’ve thought to test while working on this. Each one includes a screen recording to demonstrate what to test but are partial to either the iframe or non-iframe contexts. Testing should be done in both contexts and also with or without meta boxes present.
Device preview overflow
With a viewport short enough so the height of the device preview does not fit the outer scrolling area should be active and include/show the bottom margin of the canvas when fully scrolled. With meta boxes present this should still be the case both when the split view is resizable or when the viewport height is less than 550 pixels and the meta box pane expands/collapses (there’s no resizable separator).
device-preview-overflow.mp4
Focused edit mode
The canvas should shrink to fit the content without overflow and become scrollable only when the content is taller than the available height for the canvas.
focused-edit-mode.mp4
Sticky positioning
Sticky positioning should work as expected
sticky-positioning.mp4
Meta box toggle pane auto height
When the viewport height is less than 550 pixels the meta box pane should only be as tall as it needs to be to fit its content. Collapsing the pane should hide everything but the pane’s toggle button. When expanded and with overflow the toggle button should be remain visible even when the content is scrolled. With editor notices present the available height for the pane should be reduced and should not cause any extra scrollbars to appear.
toggle-meta-box-no-iframe.mp4
Meta box resizable pane
The pane should be resizable from zero height to full height. While resizing the canvas should be scrollable as needed and cause no extra scrolling areas.
resizable-meta-box-no-iframe.mp4
Overflow of block toolbar
For all combinations of iframed/non-iframed with or without meta boxes having the block toolbar below the fold should not cause extra scrolling areas.
block-toolbar-overflow.mp4
Editor notices
With notices present there should be no extra scrollbars. Now whether the canvas is iframed or not notices should push/shrink the canvas and be present above it whether or not the canvas is scrolled.
notices-v-canvas-scrolling.mp4
Testing Instructions for Keyboard
Test that no keyboarding changes have been introduced in any of the above scenarios.
Screenshots or screencast
Each testing sub-section above has a screen recording demonstration.