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: Add support for "more" dropdown for additional tools in Write mode #66605

Merged
merged 6 commits into from
Nov 1, 2024
Merged

Image block: Add support for "more" dropdown for additional tools in Write mode #66605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a0550dc
Image block: Add support for "more" dropdown for additional tools in …
ntsekouras Oct 30, 2024
68e5deb
update design
ntsekouras Oct 31, 2024
4e4122e
fix tests
ntsekouras Oct 31, 2024
49d5d22
fix e2e test
ntsekouras Oct 31, 2024
bd10056
keep state inside BlockControls
ntsekouras Oct 31, 2024
56f1ae4
remove pressed status
ntsekouras Nov 1, 2024
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
4 changes: 4 additions & 0 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 @@ -159,6 +159,10 @@ figure.wp-block-image:not(.wp-block) {
}
}

.wp-block-image__toolbar_content_textarea__container {
padding: $grid-unit;
}

.wp-block-image__toolbar_content_textarea {
// Corresponds to the size of the textarea in the block inspector.
width: 250px;
Expand Down
267 changes: 159 additions & 108 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 @@ -10,11 +10,14 @@ import {
TextControl,
ToolbarButton,
ToolbarGroup,
Dropdown,
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalUseCustomUnits as useCustomUnits,
Placeholder,
MenuItem,
ToolbarItem,
DropdownMenu,
Popover,
} from '@wordpress/components';
import { useViewportMatch } from '@wordpress/compose';
import { useSelect, useDispatch } from '@wordpress/data';
Expand All @@ -32,10 +35,9 @@ import {
} from '@wordpress/block-editor';
import { useEffect, useMemo, useState, useRef } from '@wordpress/element';
import { __, _x, sprintf, isRTL } from '@wordpress/i18n';
import { DOWN } from '@wordpress/keycodes';
import { getFilename } from '@wordpress/url';
import { getBlockBindingsSource, switchToBlockType } from '@wordpress/blocks';
import { crop, overlayText, upload } from '@wordpress/icons';
import { crop, overlayText, upload, chevronDown } from '@wordpress/icons';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';

Expand Down Expand Up @@ -69,6 +71,10 @@ const scaleOptions = [
},
];

const WRITEMODE_POPOVER_PROPS = {
placement: 'bottom-start',
};

// If the image has a href, wrap in an <a /> tag to trigger any inherited link element styles.
const ImageWrapper = ( { href, children } ) => {
if ( ! href ) {
Expand All @@ -94,6 +100,148 @@ const ImageWrapper = ( { href, children } ) => {
);
};

function ContentOnlyControls( {
attributes,
setAttributes,
lockAltControls,
lockAltControlsMessage,
lockTitleControls,
lockTitleControlsMessage,
} ) {
// Use internal state instead of a ref to make sure that the component
// re-renders when the popover's anchor updates.
const [ popoverAnchor, setPopoverAnchor ] = useState( null );
const [ isAltDialogOpen, setIsAltDialogOpen ] = useState( false );
const [ isTitleDialogOpen, setIsTitleDialogOpen ] = useState( false );
return (
<>
<ToolbarItem ref={ setPopoverAnchor }>
{ ( toggleProps ) => (
<DropdownMenu
icon={ chevronDown }
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'More' ) }
toggleProps={ {
...toggleProps,
description: __( 'Displays more controls.' ),
} }
popoverProps={ WRITEMODE_POPOVER_PROPS }
>
{ ( { onClose } ) => (
<>
<MenuItem
onClick={ () => {
setIsAltDialogOpen( true );
onClose();
} }
>
{ _x(
'Alternative text',
'Alternative text for an image. Block toolbar label, a low character count is preferred.'
) }
</MenuItem>
<MenuItem
onClick={ () => {
setIsTitleDialogOpen( true );
onClose();
} }
>
{ __( 'Title text' ) }
</MenuItem>
Comment on lines +132 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

As these open another popover they should probably have aria-haspopup - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup.

When pressing escape from the alt or title popovers, focus doesn't go back to these buttons, which is a bit awkward and it's not really clear what happened, was the change successful or not? A user has to reopen it to find out if the changes stuck which then takes two presses.

Copy link
Contributor Author

@ntsekouras ntsekouras Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added the aria prop here.

Regarding the design/UX questions I'd also defer to @jasmussen or @jameskoster . It's fairly new so it would need a bit time to see if we have some different feedback from users.

While focus doesn't go back to the button that triggered it, there is no focus loss though. The use case you describe requires two clicks but I'm not sure if someone who writes something and clicks escape goes back to a dialog to see if it worked - it should have worked, no?

Finally I followed some RichText tools approach (which was the same as the suggested design to close the main menu and open a dialog). That doesn't mean that it might be wrong, but if it is we should update that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've no strong opinions on the behavior, full-modal or otherwise, and would support whichever solves the problem.

</>
) }
</DropdownMenu>
) }
</ToolbarItem>
{ isAltDialogOpen && (
<Popover
placement="bottom-start"
anchor={ popoverAnchor }
onClose={ () => setIsAltDialogOpen( false ) }
offset={ 13 }
variant="toolbar"
>
<div className="wp-block-image__toolbar_content_textarea__container">
<TextareaControl
className="wp-block-image__toolbar_content_textarea"
label={ __( 'Alternative text' ) }
value={ attributes.alt || '' }
onChange={ ( value ) =>
setAttributes( { alt: value } )
}
disabled={ lockAltControls }
help={
lockAltControls ? (
<>{ lockAltControlsMessage }</>
) : (
<>
<ExternalLink
href={
// translators: Localized tutorial, if one exists. W3C Web Accessibility Initiative link has list of existing translations.
__(
'https://www.w3.org/WAI/tutorials/images/decision-tree/'
)
}
>
{ __(
'Describe the purpose of the image.'
) }
</ExternalLink>
<br />
{ __( 'Leave empty if decorative.' ) }
</>
)
}
__nextHasNoMarginBottom
/>
</div>
</Popover>
) }
{ isTitleDialogOpen && (
<Popover
placement="bottom-start"
anchor={ popoverAnchor }
onClose={ () => setIsTitleDialogOpen( false ) }
offset={ 13 }
variant="toolbar"
>
<div className="wp-block-image__toolbar_content_textarea__container">
<TextControl
__next40pxDefaultSize
className="wp-block-image__toolbar_content_textarea"
__nextHasNoMarginBottom
label={ __( 'Title attribute' ) }
value={ attributes.title || '' }
onChange={ ( value ) =>
setAttributes( {
title: value,
} )
}
disabled={ lockTitleControls }
help={
lockTitleControls ? (
<>{ lockTitleControlsMessage }</>
) : (
<>
{ __(
'Describe the role of this image on the page.'
) }
<ExternalLink href="https://www.w3.org/TR/html52/dom.html#the-title-attribute">
{ __(
'(Note: many devices and browsers do not display this text.)'
) }
</ExternalLink>
</>
)
}
/>
</div>
</Popover>
) }
</>
);
}

export default function Image( {
temporaryURL,
attributes,
Expand Down Expand Up @@ -625,112 +773,15 @@ export default function Image( {
// Add some extra controls for content attributes when content only mode is active.
// With content only mode active, the inspector is hidden, so users need another way
// to edit these attributes.
<BlockControls group="other">
<Dropdown
popoverProps={ { position: 'bottom right' } }
renderToggle={ ( { isOpen, onToggle } ) => (
<ToolbarButton
onClick={ onToggle }
aria-haspopup="true"
aria-expanded={ isOpen }
onKeyDown={ ( event ) => {
if ( ! isOpen && event.keyCode === DOWN ) {
event.preventDefault();
onToggle();
}
} }
>
{ _x(
'Alternative text',
'Alternative text for an image. Block toolbar label, a low character count is preferred.'
) }
</ToolbarButton>
) }
renderContent={ () => (
<TextareaControl
className="wp-block-image__toolbar_content_textarea"
label={ __( 'Alternative text' ) }
value={ alt || '' }
onChange={ updateAlt }
disabled={ lockAltControls }
help={
lockAltControls ? (
<>{ lockAltControlsMessage }</>
) : (
<>
<ExternalLink
href={
// translators: Localized tutorial, if one exists. W3C Web Accessibility Initiative link has list of existing translations.
__(
'https://www.w3.org/WAI/tutorials/images/decision-tree/'
)
}
>
{ __(
'Describe the purpose of the image.'
) }
</ExternalLink>
<br />
{ __(
'Leave empty if decorative.'
) }
</>
)
}
__nextHasNoMarginBottom
/>
) }
<BlockControls group="block">
<ContentOnlyControls
attributes={ attributes }
setAttributes={ setAttributes }
lockAltControls={ lockAltControls }
lockAltControlsMessage={ lockAltControlsMessage }
lockTitleControls={ lockTitleControls }
lockTitleControlsMessage={ lockTitleControlsMessage }
/>
{ title && (
<Dropdown
popoverProps={ { position: 'bottom right' } }
renderToggle={ ( { isOpen, onToggle } ) => (
<ToolbarButton
onClick={ onToggle }
aria-haspopup="true"
aria-expanded={ isOpen }
onKeyDown={ ( event ) => {
if (
! isOpen &&
event.keyCode === DOWN
) {
event.preventDefault();
onToggle();
}
} }
>
{ __( 'Title' ) }
</ToolbarButton>
) }
renderContent={ () => (
<TextControl
__next40pxDefaultSize
className="wp-block-image__toolbar_content_textarea"
__nextHasNoMarginBottom
label={ __( 'Title attribute' ) }
value={ title || '' }
onChange={ onSetTitle }
disabled={ lockTitleControls }
help={
lockTitleControls ? (
<>{ lockTitleControlsMessage }</>
) : (
<>
{ __(
'Describe the role of this image on the page.'
) }
<ExternalLink href="https://www.w3.org/TR/html52/dom.html#the-title-attribute">
{ __(
'(Note: many devices and browsers do not display this text.)'
) }
</ExternalLink>
</>
)
}
/>
) }
/>
) }
</BlockControls>
) }
<InspectorControls>
Expand Down
5 changes: 4 additions & 1 deletion test/e2e/specs/editor/various/pattern-overrides.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,7 +1131,10 @@ test.describe( 'Pattern Overrides', () => {
/\/wp-content\/uploads\//
);
await editor.showBlockToolbar();
await editor.clickBlockToolbarButton( 'Alternative text' );
await editor.clickBlockToolbarButton( 'More' );
await page
.getByRole( 'menuitem', { name: 'Alternative text' } )
.click();
await page
.getByRole( 'textbox', { name: 'alternative text' } )
.fill( 'Test Image' );
Expand Down
Loading