Avoid focusing blocks when inserting them into the canvas #28191
Conversation
youknowriad
added
[Feature] Inserter
youknowriad
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
|
Size Change: +1.29 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
You're my favorite, Riad. This is great. This is what I see:
On this one:
When you use the term "blocks" here, you're referring to blocks in the block library, correct? Because focus appears to behave just fine in-canvas:
High level, I see only upsides to this. It feels like it's closer than ever to getting the right balance of making it easy to insert blocks, not covering content, easy to close (click outside or press Escape), while still providing ample space for the blocks. I look forward to additional eyes on this, but I'm all excitement here. |
No, I meant blocks in the caves (block instances). But I did try to make the writing flow interactions work like today, so that's why you don't see major changes there. |
Gotcha. If it works as it should, then I'd not see any problem. Anything in particular to look for? |
All interactions that can insert or replace blocks. |
|
This should also be tested with screen readers to see if the new flows still make sense. |
|
This feels sooooooooooooooooooooo much better.
|
|
Here is my feedback:
This shows bad signs of not working with keyboard navigation. Tested on Windows 7, Firefox, and NV Access (NVDA) screen reader. Thanks. |
|
@alexstine Thanks for testing. I don't seem to recreate the behavior you're suggesting. When I hit "enter" on "Add block", the inserter is opened and the focus moves to its search form and when I insert a block, the focus just stays where it is in the inserter for me. That said, I'm testing on MacOS and don't have access to the same setup as you. |
|
@shaunandrews Block patterns insertion is fixed. |
|
I tried this out using VoiceOver on my Mac, and notice that once I closed the sidebar (using the
That black, floating square is the VoiceOver focus indicator; You can see how it still thinks the inserter has focus, even after I closed the sidebar. |
What do you mean "floating"? For me it should move the "plus button" and work consistently with all popovers. |
|
This Pull Request was discussed today during the weekly accessibility team's meeting. In general, the team asks for some time to test how this feature works with assistive technologies, especially screen readers, and with keyboard-only navigation. The main point of the discussion is that it should be clear to the user what happens after the inserter is closed: @alexstine reported that he couldn't figure it out, so other assistive technologies users can be confused as well. As explained by @youknowriad during the meeting, the new implementation of the inserter follows a popover model: when the inserter is closed, focus moves to the position from where the inserter was opened. Before this Pull Request, after each block is added from the inserter, focus moves to the added block. During discussion, two possible patterns emerged on how to handle focus when the inserter is closed.
As the plan is to land this feature in WordPress 5.7, there isn't a lot of time ahead, given that at least some iteration is needed to solve the two bugs identified in @alexstine's comment above. |
|
I just tried this out again and here is my experience with another round of testing.
I will say, if Escape key is used while inside the block inserter, focus does land back on Add Block as it is supposed to. Anyway, this still seems very broken for Windows keyboard navigation. Tested in Firefox with NV Access. Thanks. |
youknowriad
force-pushed
the
try/separate-block-selection-from-focus
branch
from
8719dbe
to
59ecb9f
Compare
|
@alexstine Thanks for expanding, I'm happy to fix the keyboard navigation issues. Initially, I thought you were using the global inserter, that said, when trying the steps above (quick inserter), I still have a different behavior on step 8. For me the quick inserter does close itself and the focus is back to the block wrapper though. I didn't intend to impact the quick inserter in this PR, so I'll check how different is it compared to master. |
|
The behavior of the quick inserter is restored in the last commit, how does this look? |
youknowriad
force-pushed
the
try/separate-block-selection-from-focus
branch
from
97437fd
to
9f617c3
Compare
|
Its really amazing to see the back-and-forth and continued iteration on this one. Kudos all around. -- I've been playing with this for a while this morning, and have noticed some strange interactions.
Here's a video that shows these things happening: Strange.Inserter.Interactions.mov |
| onSelect( item ); | ||
| onSelect( | ||
| item, | ||
| isAppleOS() ? event.metaKey : event.ctrlKey |
There was a problem hiding this comment.
Don't we have logic for this in the keyboard package?
There was a problem hiding this comment.
It doesn't seem exposed, but definitely something we should expose.
There was a problem hiding this comment.
mmm, actually, it seems I could use isKeyboardEvent directly. Thanks Ella
There was a problem hiding this comment.
wasn't able to do on the "click" event, we'd need to expose isApple somehow.
|
@shaunandrews the issues you raised should be fixed. Thanks for catching these. |
|
Let's get this in Thank you all. Let me know if you notice any regressions. |
closes #26223
Despite what the title of the PR might suggest, this is a very impactful PR that changes a lot of things on how subtle things in the editor work.
So, it's very important to actually test the PR properly to ensure it's working as intended and that the new behavior makes sense for all kind of users.