WIP: Try new api for in toolbar editing (and fix image block toolbar focus loss issue). #25890
Conversation
talldan
added
[a11y] Keyboard & Focus
[Block] Image
This reverts commit 5078c43.
…ed to the crop button
talldan
force-pushed
the
try/new-api-for-in-toolbar-editing
branch
from
9306e11 to
5a1e62a
Compare
|
Size Change: -5.3 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
|
@enriquesanchez Top toolbar mode for the in-toolbar editing idea is a bit problematic. The trouble is that all of this is considered one toolbar: So we'd be proposing to hide everything shown in the screenshot above when the user enables crop tools. A possible interim solution might be to use a normal popover for the crop tools when the top toolbar is active, it'd look a bit similar to the Heading block's options: Though I think this would mean you end up with multiple popovers if you Crop and then change the Aspect Ratio or Zoom. |
There was a problem hiding this comment.
This is looking good, @talldan! 🎉 Thank you!
I found a couple of issues during my testing.
- The 'Apply'/'Cancel' buttons are not aligned with the rest of the image editing popover. A few pixels up should fix it.
- I found it a bit disorienting that, coming from the block toolbar that uses the roving tabindex pattern, I can't move focus through the image editing options with my Left and Right keys. I assume this is happening because I'm technically no longer inside a toolbar and instead this is a popover, but I wonder if something can be done to match the behavior?
Other than that, this feels pretty good to my focus was preserved and predictable at all times 👍
Ah, this is a tricky one. If it's not too much trouble, I'd love to give your suggestion a try and test to see how it feels and behaves. On the other hand, I wonder how much of an issue it'll really be if we hide the whole toolbar area. After all, the user is in the middle of an image editing operation and contextually they only need the image editing tools in that specific moment. I'm trying to think of a scenario where doing so will have a negative repercussion and I can't come up with anything. What do you think? |
|
What's the status of this? It would be good to get a fix for #24676 into 5.6, but this PR is looking a bit feature-heavy for a backport, especially this close to RC1 😅 Is there any simpler fix we could try? |
|
@tellthemachines Sorry, I thought I wrote a response to this, but just re-checking and there's nothing here. My feeling was/is that we should bump this to 5.7 because of the complexity (I don't think there's a simple fix). It is unfortunate that the bug will have been in both 5.5 and 5.6. I started working on a refactor of the image editing tools that should make any solution easier (#27089), but even that is a fairly sizeable code change. There are quite a lot of pieces that need to come together for this 'in-toolbar' editing thing to work. You're right, it's a big change and affects some important pieces in the editor:
|
|
Definitely let's push this to 5.7; we're in RC stage now so we should only be fixing regressions in 5.6. |
|
Can we get a status update? |
|
I don't really have time to work on this at the moment. I've unassigned myself from the associated issues. The PR can be kept open as a draft in case anyone wants to reference it or take it over. |
|
@Mamaduka Yep, I think that's resolved now. The top toolbar could now potentially behave the same way as the normal block toolbar. I don't plan on picking this up again any time soon, so happy for someone else to take over or start fresh, and can assist with reviews or advice. |
|
Thanks for the reply, @talldan. I can take over/start fresh in a week or two. Reviews/advice would be much appreciated. |
Note: this PR currently includes the reversion changes from #25854. Ideally that will be merged first before this is code reviewed.
Description
Fixes #24676, Fixes #23721, related #23375
Using the image block's crop tool from the keyboard resulted in a focus loss.
This PR fixes the issue, introducing a new in-toolbar editing system.
The system works like this:
__experimentalBlockToolbarInlineEdit.BlockPopoveras a sibling to theBlockContextualToolbarPopoverWrapper.The advantage of this system compared to #23613 is that it doesn't require as many changes to the toolbar, the only change is an additional classname that hides the toolbar.
It also follows the advice provided by the @enriquesanchez and @afercia in using a popover for the content that 'replaces' the toolbar.
TODO:
<Dropdown>How has this been tested?
Prerequisite: Ensure top toolbar is not active (this doesn't work yet for the top toolbar).
Other things to test
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: