Block editor: placeholders: try admin shadow #33494
Conversation
|
Size Change: +590 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
I tested this on Quadrat and it works like a charm: Before: After: What is blocking this from being merged? |
|
That's amazing! Thanks for testing. Just need to polish it up a bit more and fix tests. |
| { preview && ( | ||
| <div className="components-placeholder__preview"> | ||
| { preview } | ||
| <AdminShadow> |
There was a problem hiding this comment.
Any concern about a generic UI component (@wordpress/components) having knowledge about the existence of an "admin".
The other question I have is why only apply this to placeholder component. From the @wordpress/components packages, it's just a component like any other component, how do you decide to apply this or not.
There was a problem hiding this comment.
Yeah, this is a work in progress, but I do think that this component is misplaced and belongs in the block editor package instead.
There was a problem hiding this comment.
(It's something to be used in block only.)
There was a problem hiding this comment.
The other question I have is why only apply this to placeholder component. From the
@wordpress/componentspackages, it's just a component like any other component, how do you decide to apply this or not.
It depends on whether the component is part of the editable content or not. Normally only placeholders are pieces of editor UI embedded in a block (instead of being rendered in the block toolbar or inspector).
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
a2fd8f3 to
c1c9164
Compare
|
Sorry, what is the use-case/goal of this component? I can't glean it from reading the code or the before/after example. What does "shadow" mean in this case? |
|
This is still a draft, I have yet to write a PR description. |
|
@sarayourfriend one thing it will help with is this problem: #30205 |
annezazu
added
[Type] Enhancement
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
83b6f33 to
f0e8004
Compare
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
f0e8004 to
51286cc
Compare
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
2 times, most recently
from
d86414a to
7593ef5
Compare
packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
Outdated
Show resolved
Hide resolved
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
81f470e to
c3f9be9
Compare
ellatrix
requested review from
ajitbohra,
nerrad,
noisysocks and
ntwb
as code owners
ellatrix
added
[Type] Bug
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
1276ce3 to
77c93de
Compare
packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
Outdated
Show resolved
Hide resolved
| const root = element.attachShadow( { mode: 'open' } ); | ||
| const style = document.createElement( 'style' ); | ||
| Array.from( document.styleSheets ).forEach( ( styleSheet ) => { | ||
| if ( styleSheet.ownerNode.getAttribute( 'data-emotion' ) ) { |
There was a problem hiding this comment.
Feels weird to target data-emotion. That's sounds like an implementation detail that shouldn't be relied upon.
There was a problem hiding this comment.
Yeah, ideally we should only include the styles that are needed for the placeholder, but that's very hard to do. Not sure what else we can do here, aside from just including everything. It's not really a problem to include the styles, it's just bad for performance.
|
This is sweet, shadow DOM feels like it was created for this very purpose, to protect block UI. However I did encounter two challenges. For example in the case of the Site Logo, it's basically a styled placeholder. It's supposed to look like this when resting: Like so when selected:
It also inherits the style and size that it is defined to: This is all CSS, that exists in the I'm also seeing a subtle difference in how tabbing and focus works. When I insert an Image block with the slash command, focus is trapped inside as if it were a modal: I can escape out of it, but when I then click the image block to focus it, I can no longer tab inside it. |
This is by intention. It's not possible to tab in the editor content, but you can navigate to the placeholder with the arrow keys and enter the placeholder context by pressing Space or Enter (it works the same way as a button opening a dialog). |
|
Ah, cool, just wanted to be sure it was checked! |
|
@jasmussen I fixed the site logo issue by using the old placeholder component. |
|
Looks well fixed, yes! I still have to get used to focus being inside the placeholder rather than on the placeholder, when I insert a block using the slash command, but I'm sure I'll get the hang of that. |
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
9910b2f to
261e232
Compare
| root.addEventListener( 'focusin', onFocusIn ); | ||
| root.addEventListener( 'focusout', onFocusOut ); | ||
| root.addEventListener( 'keydown', onRootKeyDown ); | ||
| element.addEventListener( 'keydown', onKeyDown ); | ||
| element.addEventListener( 'mousedown', onMouseDown ); |
There was a problem hiding this comment.
This feels very awkward to use. I had to look at this patch to figure out that I could press Enter or Space at the right moment and then Tab to reach the placeholder controls.
And there is no indication that Enter or Space did anything related to focus (Space even scrolls the page), and on Safari 15.1 I sometimes need to press ↓ not once, but twice, when moving into the Table block from above.
So there is no membrane visible to the eye, but there is one to the fingers. This should be made consistent: either there is always a clear membrane, either there is never one. I'd prefer the latter.
There was a problem hiding this comment.
And there is no indication that Enter or Space did anything related to focus
Doesn't focus move to the first element in the placeholder?
This should be made consistent: either there is always a clear membrane, either there is never one. I'd prefer the latter.
There has always been a membrane of some sort. Tab works in placeholders but doesn't work in the content (to tab within the content). What do you suggest by removing the membrane?
There was a problem hiding this comment.
I shared my screen with Ella in private, and I think she'll put up some notes soon. In the meantime, for other readers:
Doesn't focus move to the first element in the placeholder?
The focus is never apparent to the visual keyboard user. Only once one of the placeholder's controls is focused do I see visual feedback of that focus.
There has always been a membrane of some sort. Tab works in placeholders but doesn't work in the content (to tab within the content). What do you suggest by removing the membrane?
I was coming from the position that the placeholder, while not really content, doesn't communicate that it should behave differently from content when it comes to keyboard navigation and focus. And, visually, there's not enough indication that the focus is on a different type of UI when the placeholder has the focus. That's what I meant by "a visual membrane not existing". From that interpretation it followed that there should also not be a membrane or hindrance when navigating with the keyboard.
ellatrix
force-pushed
the
try/placeholder-admin-shadow
branch
from
eeb5677 to
1b15d4c
Compare
priethor
added
[Focus] Accessibility (a11y)
|
I will soon revisit this PR. |
|
Hey @ellatrix 👋 Is there any way one can assist your efforts here? What were the blocksers that prevented you from shiping this last time around? :) |
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @laurelfulford, @designsimply, @MichaelArestad, @spencerfinnell. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNCore Committers: Use this line as a base for the props when committing in SVN: GitHub Merge commitsIf 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. |
|
The blockers were time and a11y testing :) |
Description
Fixes #48256.
Fixes #59414.
Problem: placeholders in blocks should be styled as admin UI, not as editor content. Currently we're loading admin styles in the iframe to work around this, but it's not ideal. Theme CSS might still target e.g. form elements and we'd need to reset more CSS.
Solution: use shadow DOM to isolate admin UI styling from editor content styling. This would also allow us to load less styling in the iframe and make things simpler.
An additional change here is that the shadow DOM is isolated from the rest of the DOM, and this impacts keyboard navigation. The new experience is closer to that of a modal: you can navigate to the placeholder, then press Space or Enter, and focus moves into the placeholder and is constrained there. To exit press Escape.
This new behaviour is not only needed because of shadow DOM, but also because of the way we handle navigation in the editor content. The editor content is a single tab stop and you navigate through content with the arrow keys, at least in edit mode. This is how editors traditionally work. In placeholders we don't really want this behaviour because it's (1) a form manipulating editor content, not editor content, and (2) admin UI in every way. So tabbing should work normally in placeholders. Currently we have a hack for this in WritingFlow, but with shadow DOM, we can now remove this hack because of the isolated DOM and the modal-like behaviour. So it's a win-win.
Another way to think about this is comparing it with other editors. Not many editors feature placeholders. When inserting images, tables etc., there's usually a dialog type of use, or a popover, to pick some things before inserting the content. So in Gutenberg, we can think of this placeholder as a dialog as well that can be entered with the keyboard from content and escaped from back to the content.
How has this been tested?
Insert blocks with placeholders such as the image block.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).