[Block Library - Image]: Add toolbar button to add a caption #44965
Conversation
ntsekouras
added
[Package] Block library
ntsekouras
requested review from
youknowriad,
mtias,
jameskoster,
jasmussen and
ellatrix
|
Size Change: +1.1 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Very nice! This is really close already:
Solid, and I think the icon actually works okay in this context. One issue, some extra space under the image when focused is now becoming visible:
This is not present in trunk, because the caption field masks it:
This appears to be caused by the resizable box stuff. Or more specifically, by the resizable-box component being
Can we remove that? What side effects might it have? Why was it there in the first place, do you know? |
I changed the display value to |
|
I can confirm the table display fixes the focus style:
I would echo the need for a sanity check. |
|
@ntsekouras Can you explain a little more about what this is actually doing? I do not see a button to add a caption to an image block that does not have a caption. |
@alexstine currently if an image block doesn't have a caption and you select it, it will render a RichText to add the caption. In this PR you have to select the image block and a button with If you have any issues with the button in the PR, could you share a screenshot? |
Yup, that gap is indeed caused by the Changing
Ultimately, your current approach should work well (at most, you could consider updating from One more question — have you considered removing that |
Thanks for chiming in @ciampo . |
Interesting, I gave this PR a quick spin and I couldn't replicate this behaviour: Kapture.2022-10-14.at.17.31.47.mp4Maybe I'm not testing for the right thing? |
|
Hm.. Maybe it also has to do with the image width? 🤔 Screen.Recording.2022-10-14.at.6.36.30.PM.mov |
|
Ah that makes sense, yes, block goes edge go edge, table and inline-block do not. Can you try inline-flex, just to see if that does anything? I don't think it does, but just to vet it. |
I had tried all and only |
That's exactly it! Good finding! I guess we can then leave the Side note: this behaviour could also be achieved with |
That would not help anyway. I am testing the PR for accessibility so chances are what you see visually is different than what is happening for me. After checking out and building your branch, selecting the image block places focus in the caption field automatically and I do not see a button anywhere in the block toolbar to add a caption. I need instructions that tell me how to find this with the keyboard only. If none can be provided, I guess that answers the question about accessibility and further work will be needed. 👍 Thoughts? Thanks. |
@alexstine that shouldn't happen and it didn't happen for anyone else testing this PR, as this is the old behavior.. 🤔 In this PR if there is no caption we don't even render the RichText input.. Can you try once more to checkout the branch and retest? |
|
@ntsekouras It should be a toggle button. This should work out.
My problem with this approach of simply emptying the field is this is an unknown to users. To follow the same UX, it only makes sense that you should be able to select a button to undo the action that added the field. Thanks. |
ntsekouras
force-pushed
the
add/image-caption-toolbar-item
branch
from
c5051b5
to
79caffe
Compare
@alexstine I changed back to toggle button. |
ntsekouras
force-pushed
the
add/image-caption-toolbar-item
branch
from
a525621
to
69a7db8
Compare
|
@alexstine if pressing backspace removed the caption and returned focus to the "Add caption" button, could that work? I appreciate that is still 'hidden', but it's a familiar pattern across the editor (to remove empty paragraphs etc). |
|
@jameskoster Not until we actually make this reliable for screen reader users. Still not understanding what the problem is. We want a PR to add a button to add a caption, but cannot have the same to remove a caption. We have options to remove blocks even though backspace will do it. This is for the sake of simplicity and not changing the UI based on conditionals that end-users will not understand. This is a bad problem to have for accessibility because it will not be apparent to screen reader users until they go looking back for that button which is not a problem a sighted person would have. As far as reliability goes, there is still a lot of focus loss happening in these custom interactions. It is not stable enough at this point to support it and the work to get it there will be way out of scope of this PR. Thanks. |
|
Any plans to do the same for all blocks with captions and citations? |
Yes, I'll do for Audio, and Video first(caption). I'll have to check about citation.. |
What?
Resolves: #44359
This PR adds a toggle toolbar item on Image's Block Toolbar to add/remove a caption. Another change here is that the when we insert an image, the caption is not focused, even if the inserted image has one - this is important for #44918 to add Images from the Inserter and keep the focus there.
When we land an acceptable solution for this, we should probably do the same for Video and Audio blocks.
Testing Instructions
Screenshots or screencast
Screen.Recording.2022-10-14.at.8.54.26.AM.mov