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 all 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: 0 additions & 7 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 @@ -62,13 +62,6 @@ figure.wp-block-image:not(.wp-block) {
left: 50%;
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 {
cursor: default;
}
}

// This is necessary for the editor resize handles to accurately work on a non-floated, non-resized, small image.
Expand Down
77 changes: 48 additions & 29 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,29 @@ const scaleOptions = [
},
];

const disabledClickProps = {
onClick: ( event ) => event.preventDefault(),
'aria-disabled': true,
// If the image has a href, wrap in an <a /> tag to trigger any inherited link element styles.
const ImageWrapper = ( { href, children } ) => {
if ( ! href ) {
return children;
}
return (
<a
href={ href }
onClick={ ( event ) => event.preventDefault() }
aria-disabled={ true }
style={ {
// When the Image block is linked,
// it's wrapped with a disabled <a /> tag.
// Restore cursor style so it doesn't appear 'clickable'
// and remove pointer events. Safari needs the display property.
pointerEvents: 'none',
cursor: 'default',
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 Do you think this could be confusing for low vision users to get a normal mouse pointer when the link is not clickable?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

display: 'inline',
} }
>
{ children }
</a>
);
};

export default function Image( {
Expand Down Expand Up @@ -653,25 +673,31 @@ export default function Image( {

if ( canEditImage && isEditingImage ) {
img = (
<ImageEditor
id={ id }
url={ url }
width={ numericWidth }
height={ numericHeight }
clientWidth={ fallbackClientWidth }
naturalHeight={ naturalHeight }
naturalWidth={ naturalWidth }
onSaveImage={ ( imageAttributes ) =>
setAttributes( imageAttributes )
}
onFinishEditing={ () => {
setIsEditingImage( false );
} }
borderProps={ isRounded ? undefined : borderProps }
/>
<ImageWrapper href={ href }>
<ImageEditor
id={ id }
url={ url }
width={ numericWidth }
height={ numericHeight }
clientWidth={ fallbackClientWidth }
naturalHeight={ naturalHeight }
naturalWidth={ naturalWidth }
onSaveImage={ ( imageAttributes ) =>
setAttributes( imageAttributes )
}
onFinishEditing={ () => {
setIsEditingImage( false );
} }
borderProps={ isRounded ? undefined : borderProps }
/>
</ImageWrapper>
);
} else if ( ! isResizable ) {
img = <div style={ { width, height, aspectRatio } }>{ img }</div>;
img = (
<div style={ { width, height, aspectRatio } }>
<ImageWrapper href={ href }>{ img }</ImageWrapper>
</div>
);
} else {
const numericRatio = aspectRatio && evalAspectRatio( aspectRatio );
const customRatio = numericWidth / numericHeight;
Expand Down Expand Up @@ -774,7 +800,7 @@ export default function Image( {
} }
resizeRatio={ align === 'center' ? 2 : 1 }
>
{ img }
<ImageWrapper href={ href }>{ img }</ImageWrapper>
</ResizableBox>
);
}
Expand All @@ -788,14 +814,7 @@ export default function Image( {
{ /* Hide controls during upload to avoid component remount,
which causes duplicated image upload. */ }
{ ! 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 }>
{ img }
</a>
) : (
img
) }
{ img }
{ showCaption &&
( ! RichText.isEmpty( caption ) || isSelected ) && (
<RichText
Expand Down
Loading