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

Jump to bottom

Update Dropdown’s toggle/popover coordination to be UA-agnostic #30786

Closed
wants to merge 9 commits into from
Closed

Update Dropdown’s toggle/popover coordination to be UA-agnostic #30786

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
1906f42
Fix Dropdown’s closing behavior when toggle is pressed
stokesman Apr 12, 2021
005efd1
Remove tooltip wrapper from button in ButtonBlockAppender
stokesman Apr 12, 2021
b5e33c4
Remove extraneous fragment in PostSchedule
stokesman Apr 12, 2021
51dd613
Return a single-child toggle for Dropdown in ColorEdit’s ColorOption
stokesman Apr 13, 2021
891e736
Update dropdown usage in Template Part block’s placeholder
stokesman Apr 13, 2021
b82696a
Keep toggle enabled when dropdown is open in Template Part block
stokesman Apr 13, 2021
4991346
Update dropdown test due to single-child toggle concern
stokesman Apr 13, 2021
4ab48b9
Update snapshots
stokesman Apr 17, 2021
bab64e2
Update changelog and Dropdown readme
stokesman Apr 19, 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
13 changes: 4 additions & 9 deletions packages/block-editor/src/components/button-block-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { Button, Tooltip, VisuallyHidden } from '@wordpress/components';
import { Button, VisuallyHidden } from '@wordpress/components';
import { forwardRef } from '@wordpress/element';
import { _x, sprintf } from '@wordpress/i18n';
import { Icon, plus } from '@wordpress/icons';
Expand Down Expand Up @@ -47,7 +47,7 @@ function ButtonBlockAppender(
}
const isToggleButton = ! hasSingleBlockType;

let inserterButton = (
return (
<Button
ref={ ref }
onFocus={ onFocus }
Expand All @@ -61,20 +61,15 @@ function ButtonBlockAppender(
aria-expanded={ isToggleButton ? isOpen : undefined }
disabled={ disabled }
label={ label }
showTooltip
tooltipPosition="bottom"
>
{ ! hasSingleBlockType && (
<VisuallyHidden as="span">{ label }</VisuallyHidden>
) }
<Icon icon={ plus } />
</Button>
);

if ( isToggleButton || hasSingleBlockType ) {
inserterButton = (
<Tooltip text={ label }>{ inserterButton }</Tooltip>
);
}
return inserterButton;
} }
isAppender
/>
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
aria-label="Custom color picker"
className="components-button is-link"
onClick={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
type="button"
>
Custom color
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/template-part/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,12 @@ export default function TemplatePartEdit( {
<BlockControls>
<ToolbarGroup className="wp-block-template-part__block-control-group">
<Dropdown
className="wp-block-template-part__preview-dropdown-button"
contentClassName="wp-block-template-part__preview-dropdown-content"
position="bottom right left"
renderToggle={ ( { isOpen, onToggle } ) => (
<ToolbarButton
aria-expanded={ isOpen }
onClick={ onToggle }
// Disable when open to prevent odd FireFox bug causing reopening.
// As noted in https://github.com/WordPress/gutenberg/pull/24990#issuecomment-689094119 .
disabled={ isOpen }
>
{ __( 'Replace' ) }
</ToolbarButton>
Expand Down
23 changes: 11 additions & 12 deletions packages/block-library/src/template-part/edit/placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,17 @@ export default function TemplatePartPlaceholder( {
) }
>
<Dropdown
className="wp-block-template-part__placeholder-preview-dropdown"
contentClassName="wp-block-template-part__placeholder-preview-dropdown-content"
position="bottom right left"
renderToggle={ ( { isOpen, onToggle } ) => (
<>
<Button
isPrimary
onClick={ onToggle }
aria-expanded={ isOpen }
>
{ __( 'Choose existing' ) }
</Button>
<Button isTertiary onClick={ onCreate }>
{ __( 'New template part' ) }
</Button>
</>
<Button
isPrimary
onClick={ onToggle }
aria-expanded={ isOpen }
>
{ __( 'Choose existing' ) }
</Button>
) }
renderContent={ ( { onClose } ) => (
<TemplatePartSelection
Expand All @@ -78,6 +74,9 @@ export default function TemplatePartPlaceholder( {
/>
) }
/>
<Button isTertiary onClick={ onCreate }>
{ __( 'New template part' ) }
</Button>
</Placeholder>
);
}
5 changes: 5 additions & 0 deletions packages/block-library/src/template-part/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
// Make dropdown margins the same as Placeholder component does for buttons
.wp-block-template-part__placeholder-preview-dropdown {
margin-right: $grid-unit-15;
margin-bottom: $grid-unit-15;
}

.wp-block-template-part__placeholder-preview-dropdown-content,
.wp-block-template-part__preview-dropdown-content {
Expand Down
8 changes: 8 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Unreleased

### Breaking Change

- The callback provided to `renderToggle` prop of `Dropdown` must return a single child in order to ensure expected toggling behavior when the toggle is pressed while the dropdown is open.

### Bug Fix

- Fix for broken toggling behavior exhibited by `Dropdown` and `DropdownMenu` in some browsers.

## 13.0.0 (2021-03-17)

### Breaking Change
Expand Down
35 changes: 16 additions & 19 deletions packages/components/src/color-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { isEmpty, kebabCase } from 'lodash';
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { useEffect, useState } from '@wordpress/element';
import { useEffect, useRef, useState } from '@wordpress/element';
import { edit, close, chevronDown, chevronUp, plus } from '@wordpress/icons';

/**
Expand All @@ -21,15 +21,6 @@ import Button from '../button';
import TextControl from '../text-control';
import BaseControl from '../base-control';

function DropdownOpenOnMount( { shouldOpen, isOpen, onToggle } ) {
useEffect( () => {
if ( shouldOpen && ! isOpen ) {
onToggle();
}
}, [] );
return null;
}

function ColorOption( {
color,
name,
Expand All @@ -56,6 +47,14 @@ function ColorOption( {
( isHover || isFocused || isEditingName || isShowingAdvancedPanel ) &&
! immutableColorSlugs.includes( slug );

const dropdownHandle = useRef();
useEffect( () => {
const { isOpen, onToggle } = dropdownHandle.current;
if ( isEditingColorOnMount && isOpen === false ) {
onToggle();
}
}, [ isEditingColorOnMount ] );

return (
<div
tabIndex={ 0 }
Expand All @@ -77,22 +76,20 @@ function ColorOption( {
>
<div className="components-color-edit__color-option-main-area">
<Dropdown
renderToggle={ ( { isOpen, onToggle } ) => (
<>
<DropdownOpenOnMount
shouldOpen={ isEditingColorOnMount }
isOpen={ isOpen }
onToggle={ onToggle }
/>
renderToggle={ ( { isOpen, onToggle } ) => {
if ( isOpen !== dropdownHandle.current?.isOpen ) {
dropdownHandle.current = { isOpen, onToggle };
}
return (
<CircularOptionPicker.Option
style={ { backgroundColor: color, color } }
aria-expanded={ isOpen }
aria-haspopup="true"
onClick={ onToggle }
aria-label={ __( 'Edit color value' ) }
/>
</>
) }
);
} }
renderContent={ () => (
<ColorPicker
color={ color }
Expand Down
8 changes: 8 additions & 0 deletions packages/components/src/color-palette/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ exports[`ColorPalette Dropdown should render it correctly 1`] = `
aria-label="Custom color picker"
isLink={true}
onClick={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
>
<button
aria-describedby={null}
Expand All @@ -126,6 +128,8 @@ exports[`ColorPalette Dropdown should render it correctly 1`] = `
aria-label="Custom color picker"
className="components-button is-link"
onClick={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
type="button"
>
Custom color
Expand Down Expand Up @@ -552,6 +556,8 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
aria-label="Custom color picker"
isLink={true}
onClick={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
>
<button
aria-describedby={null}
Expand All @@ -560,6 +566,8 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
aria-label="Custom color picker"
className="components-button is-link"
onClick={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
type="button"
>
Custom color
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/dropdown/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Dropdown

Dropdown is a React component to render a button that opens a floating content modal when clicked.
This components takes care of updating the state of the dropdown menu (opened/closed), handles closing the menu when clicking outside
and uses render props to render the button and the content.
Dropdown is a React component to render a button that opens a floating content dialog when clicked.
The component takes care of updating the state of the dropdown (opened/closed), handles closing
the popover when clicking outside and uses render props to render the button and the content.

## Usage

Expand Down Expand Up @@ -52,7 +52,9 @@ The direction in which the popover should open relative to its parent node. Spec

### renderToggle

A callback invoked to render the Dropdown Toggle Button.
A callback invoked to render the Dropdown Toggle Button. NOTE: The return must be a single child as it
will be cloned and have some event handlers added to ensure that a press of the toggle while the
dropdown is open will bypass the auto-closing behavior and defer control to the toggle.

- Type: `Function`
- Required: Yes
Expand Down
32 changes: 22 additions & 10 deletions packages/components/src/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { useRef, useEffect, useState } from '@wordpress/element';
import { useRef, useEffect, useState, cloneElement } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -56,16 +56,15 @@ export default function Dropdown( {
}

/**
* Closes the dropdown if a focus leaves the dropdown wrapper. This is
* intentionally distinct from `onClose` since focus loss from the popover
* is expected to occur when using the Dropdown's toggle button, in which
* case the correct behavior is to keep the dropdown closed. The same applies
* in case when focus is moved to the modal dialog.
* Closes the dropdown if focus leaves its popover unless its toggle was
* pressed or focus was moved to within a modal dialog. The former is to
* let the toggle act to close the dropdown and the latter is to enable
* the focus to return after the dialog is dismissed.
*/
function closeIfFocusOutside() {
const { ownerDocument } = containerRef.current;
if (
! containerRef.current.contains( ownerDocument.activeElement ) &&
! isPressingToggle.current &&
! ownerDocument.activeElement.closest( '[role="dialog"]' )
) {
close();
Expand All @@ -81,17 +80,30 @@ export default function Dropdown( {

const args = { isOpen, onToggle: toggle, onClose: close };

const isPressingToggle = useRef();
let toggleElement = renderToggle( args );
const { props } = toggleElement;
const pressHandlers = {
onMouseDown: ( event ) => {
isPressingToggle.current = true;
props.onMouseDown?.( event );
},
onMouseUp: ( event ) => {
isPressingToggle.current = false;
props.onMouseUp?.( event );
},
};
toggleElement = cloneElement( toggleElement, pressHandlers );
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever solutions, Unfortunately, this is a breaking change in WordPress and something we can't ship just like that. Potentially we can try to detect whether we receive just a single element.

The problem though is even if it's just a single element, there's no guarantee that this element supports these props. So for me instead of doing this like that, we could potentially add these two handlers as new args to the render callback and have the consumer add them.

Copy link
Contributor Author

@stokesman stokesman Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

there's no guarantee that this element supports these props

True, one of the adjustments made in this PR was a to ButtonBlockAppender which didn't work even though it already returned a single element.

we could potentially add these two handlers as new args to the render callback and have the consumer add them

I'm happy to polish up my draft PR #30397 which does just that.

Thank you for reviewing 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad, I found a one-liner solution for this in #31170. Pinging you here for the context. It's much appreciated if you'll have a look.

return (
<div
className={ classnames( 'components-dropdown', className ) }
ref={ containerRef }
>
{ renderToggle( args ) }
{ toggleElement }
{ isOpen && (
<Popover
position={ position }
onClose={ close }
onFocusOutside={ closeIfFocusOutside }
onClose={ closeIfFocusOutside }
expandOnMobile={ expandOnMobile }
headerTitle={ headerTitle }
focusOnMount={ focusOnMount }
Expand Down
16 changes: 8 additions & 8 deletions packages/components/src/dropdown/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,20 +65,20 @@ describe( 'Dropdown', () => {
<Dropdown
className="container"
contentClassName="content"
renderToggle={ ( { isOpen, onToggle, onClose } ) => [
renderToggle={ ( { isOpen, onToggle } ) => (
<button
key="open"
className="open"
aria-expanded={ isOpen }
onClick={ onToggle }
>
Toggleee
</button>,
<button key="close" className="close" onClick={ onClose }>
closee
</button>,
] }
renderContent={ () => null }
</button>
) }
renderContent={ ( { onClose } ) => (
<button className="close" onClick={ onClose }>
test
</button>
) }
/>
);

Expand Down
3 changes: 3 additions & 0 deletions packages/edit-post/src/components/header/more-menu/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ exports[`MoreMenu should match snapshot 1`] = `
label="Options"
onClick={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseUp={[Function]}
showTooltip={true}
tooltipPosition="bottom"
>
Expand All @@ -80,6 +82,7 @@ exports[`MoreMenu should match snapshot 1`] = `
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
onMouseUp={[Function]}
type="button"
>
<Icon
Expand Down
18 changes: 8 additions & 10 deletions packages/edit-post/src/components/sidebar/post-schedule/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,14 @@ export function PostSchedule() {
position="bottom left"
contentClassName="edit-post-post-schedule__dialog"
renderToggle={ ( { onToggle, isOpen } ) => (
<>
<Button
className="edit-post-post-schedule__toggle"
onClick={ onToggle }
aria-expanded={ isOpen }
isTertiary
>
<PostScheduleLabel />
</Button>
</>
<Button
className="edit-post-post-schedule__toggle"
onClick={ onToggle }
aria-expanded={ isOpen }
isTertiary
>
<PostScheduleLabel />
</Button>
) }
renderContent={ () => <PostScheduleForm /> }
/>
Expand Down