Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image Block: Enable image block to be selected correctly when clicked #56043

Merged
merged 9 commits into from
Nov 17, 2023
Merged

Image Block: Enable image block to be selected correctly when clicked #56043

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8c0dd62
Image Block: Enable image blocks to be selected correctly when clicked
t-hamano Nov 10, 2023
63cc630
Merge branch 'trunk' into fix/image-click-event
t-hamano Nov 11, 2023
348e110
Add tabIndex
t-hamano Nov 11, 2023
f2a8b6a
Update packages/block-library/src/image/editor.scss
t-hamano Nov 14, 2023
6818f46
Merge branch 'trunk' into fix/image-click-event
t-hamano Nov 14, 2023
e9ea459
Wrap img element directly to make the image resizable
t-hamano Nov 14, 2023
746099d
Merge branch 'trunk' into fix/image-click-event
t-hamano Nov 16, 2023
8a37216
Use `display:inline` instead of `display:inline-block`
t-hamano Nov 16, 2023
5936361
Merge branch 'trunk' into fix/image-click-event
t-hamano Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/block-library/src/image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ figure.wp-block-image:not(.wp-block) {
transform: translate(-50%, -50%);
}

// When the Image block is linked,
// it's wrapped with a disabled <a /> tag.
// Restore cursor style so it doesn't appear 'clickable'.
> a {
pointer-events: none;
// When the Image block is linked,
// it's wrapped with a disabled <a /> tag.
// Restore cursor style so it doesn't appear 'clickable'.
cursor: default;
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down
5 changes: 3 additions & 2 deletions packages/block-library/src/image/image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ const scaleOptions = [
},
];

const disabledClickProps = {
const disabledProps = {
onClick: ( event ) => event.preventDefault(),
'aria-disabled': true,
tabIndex: -1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was able to solve this problem by adding tabIndex="-1" to the a element of the image block, but are there any concerns about this?

This fixes the issue that @jeryj reported for me.

Seems to be acceptable according to the spec, but I'll defer to other folks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

export default function Image( {
Expand Down Expand Up @@ -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 }>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

{ img }
</a>
) : (
Expand Down
Loading