Image Block: Enable image block to be selected correctly when clicked #56043
Conversation
t-hamano
added
[Type] Bug
|
Size Change: +13 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
requested review from
artemiomorales,
michalczaplinski,
ramonjd and
alexstine
|
@alexstine I would appreciate it if you could let me know if applying |
t-hamano
added
the
Backport to WP Minor Release
t-hamano
changed the title
|
@t-hamano I am not sure about this but I would think so. CC: @joedolson @afercia @jeryj. |
|
The Reproduction
I haven't dug into the code, but my guess is that it has to do with the Could we use |
|
The markup in the editor and the font should match as close as possible, otherwise it becomes more difficult to style. |
|
@jeryj In that case, I think writing-flow/use-arrow-nav hook needs to be explored. I think it matches |
|
The problem with focusing on the I was able to solve this problem by adding The video below shows pressing the up and down arrow keys repeatedly on an editor containing an Image, Site Logo, and Paragraph block. You can see that the focus no longer moves to the 1cd1b0e17b62389da42a5825bf970f12.mp4 |
There was a problem hiding this comment.
Thank you @t-hamano for this PR!
Using role="link" would probably be okay but maybe not for visuals.
Unfortunately, in my testing, changing the tag and adding the role attribute doesn't trigger the styles as the markup is different as @carolinan notes.
The pointer-events: none; may be part of the solution, but it may not be necessary.
In my testing, I couldn't find an alternative to this. Removing the pointer events ensure that the click even is triggered on the parent element.
We only need to cater for Safari (surprise, surprise) by adding display: inline-block.
I've tested in FF, Chrome and Edge as well and it's working with the above change.
Here's what I'm seeing:
✅ Click bug is fixed (with display: inline-block for Safari)
✅ Arrow up and down selects previous and subsequent blocks immediately
Would it be helpful to add role="presentation" to the link to remove the <a /> semantics or am I completely overthinking it? 🤣
I'm happy to approve this if other folks are fine with the changes.
| onClick: ( event ) => event.preventDefault(), | ||
| 'aria-disabled': true, | ||
| tabIndex: -1, |
There was a problem hiding this comment.
@t-hamano No, you can't add tabindex="-1" like this. Doing that prevents the link from gaining focus with arrow key navigation. You need to be able to focus if you are going to hear things like the URL, ALT text, filename, etc.
There was a problem hiding this comment.
In the context of the editor, the link is a very recent addition whose only function is to ensure the editor styles match the frontend because some global styles target A-tags. The A-tag doesn't actually need an href value to achieve this.
Would it be acceptable to remove it from the accessibility tree or mark it as presentation only or something?
Were the ALT text and filename not available from the IMG element previously?
If getting these attributes is preferable and, bear with me as this might be wild 😄, could we add a data-href to the surrounding figure, which is the selectable "block" element?
There was a problem hiding this comment.
@ramonjd You can't add role="presentation" to the link or else you will also destroy the semantics of the <img> tag wrapped inside. I never actually knew you could link an image block so not sure how it worked before. Pretty sure if there is no link, you could not access that information because <img> by default cannot take focus. That is a bug that needs to be fixed somehow.
There was a problem hiding this comment.
It's a very difficult problem, but for this PR, it might be better to focus on the problem of not being able to click the linked Image block correctly. That is, don't add code like tabIndex=-1 that prevents the a element in the block from being focused.
It is not only the Image block that the keyboard focuses on the a element within the block. For example, a similar problem occurs with the Site Logo block with the home link set.
I think we can discuss more broadly in another issue, such as what kind of keyboard interaction is expected in the editor and what should be communicated to screen readers when the block contains an a element or an img element. .
There was a problem hiding this comment.
Agreed. If you use your arrow keys to select the block and the <a> becomes focussed, you can always focus the block wrapper with left arrow. This is how all blocks with wrappers work with tabindex="0" set on the wrapper.
There was a problem hiding this comment.
Ah okay, I appreciate the explanation @alexstine Thank you! 👍🏻
I think we can discuss more broadly in another issue, such as what kind of keyboard interaction is expected in the editor and what should be communicated to screen readers when the block contains an a element or an img element. .
Sounds like a good approach.
| @@ -790,7 +791,7 @@ export default function Image( { | |||
| { ! temporaryURL && controls } | |||
| { /* If the image has a href, wrap in an <a /> tag to trigger any inherited link element styles */ } | |||
| { !! href ? ( | |||
| <a href={ href } { ...disabledClickProps }> | |||
| <a href={ href } { ...disabledProps }> | |||
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
|
I noticed that if I applied 4802ad85b1bcf757c12f9f7808e17c1c.mp4 |
I tried to solve this problem by wrapping the Before: <a style={{pointerEvents: 'none'}}>
<ResisableBox>
<img />
</ResisableBox>
</a>After: <ResisableBox>
<a style={{pointerEvents: 'none'}}>
<img />
</a>
</ResisableBox>Other changes are as follows:
|
|
It seems to work fine in my testing. In all scenarios, the Image block is clickable, and the border color also seems to inherit the link color. In the screenshot below, I'm testing various scenarios with the Twenty Twenty-Three theme, which has a green link color. 8495a705d7cd1274095f3c6b42cc0750.mp4 |
| // Restore cursor style so it doesn't appear 'clickable' | ||
| // and remove pointer events. Safari needs the display property. | ||
| pointerEvents: 'none', | ||
| cursor: 'default', |
There was a problem hiding this comment.
@t-hamano Do you think this could be confusing for low vision users to get a normal mouse pointer when the link is not clickable?
There was a problem hiding this comment.
On the face of it, default seems appropriate to me given that the link is disabled. (?)
cursor: 'pointer' is the "hand", or default browser cursor, for linked elements.
There was a problem hiding this comment.
I think that since this element cannot actually be clicked, it is okay to leave the cursor as the default.
By the way, #56123 has been submitted to add a pressed state to the link button in the block toolbar to make it easier to tell if the image is linked.
|
I can confirm that there's no regression to the fix for #55336 as well. Just to confirm I can still reproduce the arrow selection scenario described above, but that's for a follow up, right? Thanks @t-hamano and @alexstine for sticking with this 🍺 |
That's right. The behavior of being able to select the |
|
I checked again after merging the latest trunk and discovered another problem with this PR. When I apply a link to the image, the height of the image changes and it extends beyond the 8bda9092985b55c6df010084abf68115.mp4I expected that @ramonjd In this comment of yours, you say that the Could you please confirm whether |
|
Another approach might be to focus the block when the |
I think you mean Safari? No problem, I'll test today and get back to you! |
It looks good to me Here's what I'm seeing in Safari: With 2023-11-17.07.07.22.mp4With |
|
Thanks for the testing, @ramonjd!
Yes, that's right 😅
I was relieved that it seemed to work well. I believe that all problems have been resolved, so I would like to merge this PR. Thank you for your cooperation! |
|
Note: As stated in this comment, this PR fixes a bug that first appeared in WP6.4. I'm giving it a backport label because I think this should be fixed in a minor release. |
|
@mikachan Is it possible to backport this PR to WP6.4.2? |
Yes that should be fine, as 6.4.2 is still being worked on. I'll cherry-pick this PR for inclusion in a package update, ready for 6.4.2. |
|
I just cherry-picked this PR to the 6-4-2-cherry-picks branch to get it included in the next release: d5de4af |
…#56043) * Image Block: Enable image blocks to be selected correctly when clicked * Add tabIndex * Update packages/block-library/src/image/editor.scss Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Wrap img element directly to make the image resizable * Use `display:inline` instead of `display:inline-block` --------- Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
mikachan
removed
the
Backport to WP Minor Release
…#56043) * Image Block: Enable image blocks to be selected correctly when clicked * Add tabIndex * Update packages/block-library/src/image/editor.scss Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Wrap img element directly to make the image resizable * Use `display:inline` instead of `display:inline-block` --------- Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Package Update includes fixes for: - Image Block: Enable image block to be selected correctly when clicked. - Reduce specificity of default Cover text color styles. - Image Block: Fix deprecation when width/height attribute is number. - Text selection: show CSS hack to Safari only. - SlotFill: Allow contextual SlotFillProviders. See: #5696 See: WordPress/gutenberg@bd6767b See: WordPress/gutenberg#56043 See: WordPress/gutenberg#56411 See: WordPress/gutenberg#57063 See: WordPress/gutenberg#57300 See: WordPress/gutenberg#56779 Props mikachan, wildworks, alexstine, poena, isabel_brison, andrewserong, czapla, andraganescu, joen, ellatrix, youknowriad, ntsekouras. Fixes #59943, #59943. git-svn-id: https://develop.svn.wordpress.org/trunk@57258 602fd350-edb4-49c9-b593-d223f7449a82
Package Update includes fixes for: - Image Block: Enable image block to be selected correctly when clicked. - Reduce specificity of default Cover text color styles. - Image Block: Fix deprecation when width/height attribute is number. - Text selection: show CSS hack to Safari only. - SlotFill: Allow contextual SlotFillProviders. See: WordPress/wordpress-develop#5696 See: WordPress/gutenberg@bd6767b See: WordPress/gutenberg#56043 See: WordPress/gutenberg#56411 See: WordPress/gutenberg#57063 See: WordPress/gutenberg#57300 See: WordPress/gutenberg#56779 Props mikachan, wildworks, alexstine, poena, isabel_brison, andrewserong, czapla, andraganescu, joen, ellatrix, youknowriad, ntsekouras. Fixes #59943, #59943. Built from https://develop.svn.wordpress.org/trunk@57258 git-svn-id: http://core.svn.wordpress.org/trunk@56764 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Package Update includes fixes for: - Image Block: Enable image block to be selected correctly when clicked. - Reduce specificity of default Cover text color styles. - Image Block: Fix deprecation when width/height attribute is number. - Text selection: show CSS hack to Safari only. - SlotFill: Allow contextual SlotFillProviders. See: WordPress/wordpress-develop#5696 See: WordPress/gutenberg@bd6767b See: WordPress/gutenberg#56043 See: WordPress/gutenberg#56411 See: WordPress/gutenberg#57063 See: WordPress/gutenberg#57300 See: WordPress/gutenberg#56779 Props mikachan, wildworks, alexstine, poena, isabel_brison, andrewserong, czapla, andraganescu, joen, ellatrix, youknowriad, ntsekouras. Fixes #59943, #59943. Built from https://develop.svn.wordpress.org/trunk@57258 git-svn-id: https://core.svn.wordpress.org/trunk@56764 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Follow-up on #55470
Fixes: #56017
What?
This PR allows you to select image blocks "correctly".
This will display a blue outline when a block is selected, and inserting a block with the Enter key and deleting a block with the Delete key will work correctly.
Why?
With #55470, the image block is now wrapped in an A tag when it's linked. As a result, even if you click on the block, it no longer appears to be selected correctly.
How?
To find a way to solve this, I looked into blocks where the block itself is an A element, or where there is an A element inside the block. As a result, I noticed that there are many approaches that add
pointer-events:noneto the A element.I can't explain it exactly, but I found that block selection works correctly with this CSS.
Update: I found that if I simply added
pointer-events: noneto theaelement, the image could no longer be resized. This is because theResizableBoxcomponent is wrapped in anaelement. To solve this I moved theaelement inside theResisableBox. Check this comment for details.Testing Instructions
Screenshots or screencast
6eecccd992589fb13988da7ce03d9b7b.mp4