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: Caption toolbar is not dismissed when cropping image in place #30407

Closed
wants to merge 3 commits into from
Closed

Image block: Caption toolbar is not dismissed when cropping image in place #30407

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f7dc73
In order to dismiss the caption toolbar when cropping image in place …
ramonjd Mar 31, 2021
9733917
Addressed the linting issues that are relevant to the changes in the PR:
ramonjd Mar 31, 2021
1417e4a
Switch to onBlur event
Apr 7, 2021
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: 6 additions & 1 deletion packages/block-library/src/image/image-editing/cropper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { Spinner } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { MIN_ZOOM, MAX_ZOOM } from './constants';

import { useImageEditingContext } from './context';

export default function ImageCropper( {
Expand All @@ -23,6 +23,7 @@ export default function ImageCropper( {
clientWidth,
naturalHeight,
naturalWidth,
onClick,
} ) {
const {
isInProgress,
Expand Down Expand Up @@ -51,6 +52,10 @@ export default function ImageCropper( {
width: width || clientWidth,
height: editedHeight,
} }
onClick={ onClick }
onKeyDown={ onClick }
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure how accessible this is via keyboard anyway.

Are folks happy with adding a

// eslint-disable-next-line jsx-a11y/no-static-element-interactions

to disable for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any objections to disabling the no-static-element-interactions linting for this. It appears to be done in a dozen or more places already.

The accessibility concerns here aren't my strong suit so happy to defer to others there.

role="img"
aria-label={ __( 'Image editor cropping tool' ) }
>
<Cropper
image={ editedUrl || url }
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/image/image-editing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export default function ImageEditor( {
clientWidth,
naturalHeight,
naturalWidth,
onClick,
} ) {
return (
<>
Expand All @@ -30,6 +31,7 @@ export default function ImageEditor( {
clientWidth={ clientWidth }
naturalHeight={ naturalHeight }
naturalWidth={ naturalWidth }
onClick={ onClick }
/>
<BlockControls>
<ToolbarGroup>
Expand Down
2 changes: 2 additions & 0 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 @@ -211,6 +211,7 @@ export default function Image( {

function onImageClick() {
if ( captionFocused ) {
captionRef.current.blur();
setCaptionFocused( false );
}
}
Expand Down Expand Up @@ -454,6 +455,7 @@ export default function Image( {
clientWidth={ clientWidth }
naturalHeight={ naturalHeight }
naturalWidth={ naturalWidth }
onClick={ onImageClick }
/>
);
} else if ( ! isResizable || ! imageWidthWithinContainer ) {
Expand Down