Remove empty column blocks from mobile in editor, to match front-end #63962
Conversation
richtabor
added
[Type] Bug
|
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. |
|
Size Change: +64 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
richtabor
changed the title
richtabor
requested review from
t-hamano and
a team
and removed request for
ajitbohra and
ntsekouras
|
This makes sense to me. There is a quirk though; if all columns are empty then nothing appears on the canvas, and you have to use list view to select the Columns block. Should we use the Group approach there (show a dotted outline when empty to aid on-canvas selection)? |
Yes, that's like it is in trunk. I'm not confident that empty columns should be rendered with outlines in the editor, unless the parent is selected. Perhaps it could be any parent? Otherwise, you'd see empty columns in areas that are intended to be empty, messing with the 1:1 with the front-end expectation. But empty columns should have a hover/focus effect so there is some affordance it exists, when you're intending to engage with it. The Group block should perform this same way imo. |
There was a problem hiding this comment.
While I agree with matching the editor and the frontend, this approach seems to cause some issues.
- In the mobile layout, I can't move blocks into an empty Column block via dragging.
- When the Columns block is completely empty, I can't click on it with the mouse because its height is zero.
If you want to achieve a layout like the screenshot, how about giving explicit widths to the children of the Row/Columns block?
c0ac1c925f4a403a8d5e90dcdd8802a3.mp4
In any case, this needs to receive accessibility feedback.
t-hamano
added
the
[Focus] Accessibility (a11y)
I'd consider both of these edge cases, and worth the case that the mobile preview looks like it should. |
I'm not following this? |
It's unclear to me why this needs a11y feedback. The edge case of dragging into columns on mobile seems a bit out there (and not a11y related). If the block is selected, you can add blocks to it all the same as in desktop. Currently, you view a mobile view of your site but see a lot of spacing within the editor (if you have empty columns, which is not uncommon). So you try to fix it, but it's not actually a problem, making things quite confusing. Also, makes the columns block seem not properly responsive. I want to fix this, which is significant to building confidence in the editing experience. |
To me, these are not edge cases that can be ignored, but rather lead to a decrease in usability and accessibility. Also, I think it's better to have PRs that affect screen reader reading, keyboard operation, and mouse operation undergo accessibility checks as much as possible. cc @joedolson @alexstine (@afercia might be AFK right now) |
It 100% is an edge case to drag a block into a column while viewing the mobile view in the editor. It's actually not readily possible to drag blocks when editing mobile—there are no drag handles and the toolbar is pinned to the top.
You can actually, it's just not ideal. This is also an edge case (maintaining a columns block with no items in it on a page). The empty blocks are also still accessible from the List View, selecting either the parent, or the individual columns blocks. CleanShot.2024-07-31.at.08.50.15.mp4 |
richtabor
force-pushed
the
try/columns-appender-on-parent
branch
from
65cff82
to
282ed6a
Compare
Yes, I added
My explanation for this was incorrect. I was referring to the narrow editor canvas width, not mobile devices. If the editor canvas width is less than 781px, columns are stacked vertically. In this case, you can't drag blocks into columns, as shown in the video below. 1a3faaa9dd3cb814826accb7d7cc68d7.mp4 |
How can we pass that through to the column blocks? |
I still don't think it is normal/expected to drag items underneath each other and expect them to be in a column on desktop. It's a much worse—and much more prevalent— issue to have an incorrect mobile view when editing. |
|
There's a larger question at play here, which looks a bit to the future when the block editor is more refined. Should you be able to design your desktop site, from your phone? The answer is nuanced, and will likely be assisted by locked-down patterns, but also ways to preview the desktop breakpoint from smaller physical devices. However given that, I'd tend to agree with Rich's assessment, that in absence of the ability to preview a desktop view from a physical mobile device, we should at least treat WYSIWYG as a goal in itself, an increasingly important one at that, since block themes are getting more powerful. In that vein, I'd think 1:1 parity with the frontend to be the most important aspect to optimize for. |
We can also do similar checks in child blocks. I've updated the selector in ab74c04. I've not touched the appender logic because I'm not 100% sure what we want to achieve here. |
|
@Mamaduka Thanks for the update! @richtabor I made the columns block retain its height if all columns are empty. Is this what you intended? bd7f5c10985d2bc1ccfa80bb275ad98c.mp4 |
Yes! This works quite well now.
Now, you can still select/edit appropriately, and an empty columns block does not distort the mobile view. |
|
This E2E test is failing and needs to be addressed. This test ensures that we can drag and drop blocks into empty columns. However, presumably because the browser width emulated in the E2E test is narrow, the columns are stacked vertically. The empty column block has no height, so we cannot drag and drop blocks into it: 17a33dad6e666b4fc49ab99a83f25b4a.mp4This is my concern in this comment, but is this not a problem? For example, some monitors may have a low resolution of around 1280. On such a monitor, if you open both the List View and the Block Sidebar, the iframee editor canvas width will be less than 781px, so the column blocks will be stacked vertically. |
I don't think this is a problem. I still consider this an edge case that is accounted for by the work we added to check if columns are empty (then render them entirely), and also render columns one selection. The only caveat being that you need to have a columns block selected to add something to it, on mobile. |
|
I tried to fix the E2E tests. Personally, I would hesitate to ship this PR, but if enough people feel this approach makes sense, it may be worth a try. However, if this behavior is reported as a bug after we ship this PR, we might want to consider reverting it. |
|
@Mamaduka could you take a look at these e2e tests?
I'm confident this is a better solution than what we have in trunk. I'm down to give it a go. |
|
Flaky tests detected in 602fd05. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10543277873
|
|
I think I've resolved the E2E test failure. The root cause is that the block appender is not rendered when the Column block has a non-empty column block. I know this is an intentional change in this PR. This E2E test tests whether an Image block can be dragged into the second of three columns:
However, no block appender is rendered for the second column, so the test fails as it cannot find the appender here. To solve this, I inserted a Columns block where all three columns are empty. This ensures that the column block has a block appender, so the test should pass:
One other thing I want to mention is that another purpose of this E2E test is to ensure a blue visual cue appears when we drag a block:
However, no visible visual cues are present in the current Column block. The test does pass, but internally it only checks whether the CSS for visual cues has been applied. I know that the current behavior of empty blocks and block appenders is intentional, but it feels like the current editor experience is straying too far from the editor experience proposed in #39064. What do you think? |
What?
Fixes #63933 by not always rendering an empty column block when viewing the mobile preview in the editor.
Why?
The front-end and editor instances of the Columns block should be exactly the same. That way, I can design in the editor and it renders on the front-end exactly like I intended.
Testing Instructions
Screenshots or screencast
CleanShot.2024-07-25.at.17.32.44.mp4