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.

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

ColorPicker: improve UX of dragging the handle when in popover on top of the editor #55149

Merged
merged 23 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d508487
ColorPicker: expose drag start / end handlers via context
ciampo Oct 8, 2023
d6b7e84
ColorPicker: remove overflow hidden rule
ciampo Oct 8, 2023
64cfbef
Dropdown: add some sort of backdrop to capture pointer events
ciampo Oct 8, 2023
37ff16f
Add test story
ciampo Oct 8, 2023
d6b387b
Prevent resizing on color palette's popover, which also triggers over…
ciampo Oct 8, 2023
49030c6
Improve backdrop position by using react portal
ciampo Oct 9, 2023
7e342bf
Cleanup
ciampo Oct 9, 2023
1c5ae3d
Aria-hidden
ciampo Oct 9, 2023
54146e2
Cleanup
ciampo Oct 9, 2023
74e1418
Scope querySelector within color picker s container element
ciampo Oct 11, 2023
fc32af5
Listen for drag events also on hue and alpha sliders
ciampo Oct 11, 2023
90147e6
Remove temporary styles
ciampo Oct 11, 2023
fdf61a0
Remove temporary storybook example
ciampo Oct 11, 2023
c9a6923
CHANGELOG
ciampo Oct 11, 2023
2b8f56f
Move pointer events trap from Dropdown to Popover component
ciampo Oct 11, 2023
d475284
Use state to grab ColorPicker's container element to make sure the co…
ciampo Oct 11, 2023
e3a7be0
Remove overflow:hidden from PaletteEdit component's popover
ciampo Oct 11, 2023
9318bf8
Update CHANGELOG
ciampo Oct 11, 2023
7a4afc1
Remove dead code
ciampo Oct 11, 2023
a065cf8
Update snapshot
ciampo Oct 11, 2023
e483ec2
Move newly added ColorPicker logic to a separate hook
ciampo Oct 11, 2023
8c95ac1
Add inline comment around the resize: false change
ciampo Oct 11, 2023
d008cab
Add more comments
ciampo Oct 11, 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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

### Bug Fix

- Render a "mouse event trap" when using a `ColorPicker` inside a `Popover` to prevent issues when rendering on top of `iframes` ([#55149](https://github.com/WordPress/gutenberg/pull/55149)).
- `Modal`: fix closing when contained iframe is focused ([#51602](https://github.com/WordPress/gutenberg/pull/51602)).

### Internal
Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/color-palette/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,10 @@ export function CustomColorPickerDropdown( {
const popoverProps = useMemo< DropdownProps[ 'popoverProps' ] >(
() => ( {
shift: true,
// Disabling resize as it would otherwise cause the popover to show
// scrollbars while dragging the color picker's handle close to the
// popover edge.
resize: false,
Copy link
Member

Choose a reason for hiding this comment

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

Why is the resize: false necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in 8c95ac1

...( isRenderedInSidebar
? {
// When in the sidebar: open to the left (stacking),
Expand Down
28 changes: 25 additions & 3 deletions packages/components/src/color-picker/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import namesPlugin from 'colord/plugins/names';
* WordPress dependencies
*/
import { useCallback, useState, useMemo } from '@wordpress/element';
import { useDebounce } from '@wordpress/compose';
import { useDebounce, useMergeRefs } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -49,8 +49,24 @@ const UnconnectedColorPicker = (
onChange,
defaultValue = '#fff',
copyFormat,

// Context
onPickerDragStart,
onPickerDragEnd,
Comment on lines +54 to +55
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These "props" are only exposed via internal context, therefore not causing an API change to the component

...divProps
} = useContextSystem( props, 'ColorPicker' );
} = useContextSystem<
ColorPickerProps & {
onPickerDragStart?: ( event: MouseEvent ) => void;
onPickerDragEnd?: ( event: MouseEvent ) => void;
}
>( props, 'ColorPicker' );

const [ containerEl, setContainerEl ] = useState< HTMLElement | null >(
null
);
const containerRef = ( node: HTMLElement | null ) => {
setContainerEl( node );
};

// Use a safe default value for the color and remove the possibility of `undefined`.
const [ color, setColor ] = useControlledValue( {
Expand All @@ -77,11 +93,17 @@ const UnconnectedColorPicker = (
);

return (
<ColorfulWrapper ref={ forwardedRef } { ...divProps }>
<ColorfulWrapper
ref={ useMergeRefs( [ containerRef, forwardedRef ] ) }
{ ...divProps }
>
<Picker
containerEl={ containerEl }
onChange={ handleChange }
color={ safeColordColor }
enableAlpha={ enableAlpha }
onDragStart={ onPickerDragStart }
onDragEnd={ onPickerDragEnd }
/>
<AuxiliaryColorArtefactWrapper>
<AuxiliaryColorArtefactHStackHeader justify="space-between">
Expand Down
98 changes: 96 additions & 2 deletions packages/components/src/color-picker/picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,112 @@ import { colord } from 'colord';
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { useMemo, useEffect, useRef } from '@wordpress/element';
/**
* Internal dependencies
*/
import type { PickerProps } from './types';

export const Picker = ( { color, enableAlpha, onChange }: PickerProps ) => {
/**
* Track the start and the end of drag pointer events related to controlling
* the picker's saturation / hue / alpha, and fire the corresponding callbacks.
* This is particularly useful to implement synergies like the one with the
* `Popover` component, where a pointer events "trap" is rendered while
* the user is dragging the pointer to avoid potential interference with iframe
* elements.
*
* @param props
* @param props.containerEl
* @param props.onDragStart
* @param props.onDragEnd
*/
const useOnPickerDrag = ( {
containerEl,
onDragStart,
onDragEnd,
}: Pick< PickerProps, 'containerEl' | 'onDragStart' | 'onDragEnd' > ) => {
const isDragging = useRef( false );
const leftWhileDragging = useRef( false );
useEffect( () => {
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to move this functionality to a separate hook? I think it makes sense because it's separate and independent enough to be its own hook, but also because it can help document the behavior better. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e483ec2

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to add some comments that explain what we're doing here. The documentation could also include what else we're doing to achieve the mouse trap hack (like the additional CSS and the backdrop show and hide trick).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d008cab

if ( ! containerEl || ( ! onDragStart && ! onDragEnd ) ) {
return;
}
const interactiveElements = [
containerEl.querySelector( '.react-colorful__saturation' ),
containerEl.querySelector( '.react-colorful__hue' ),
containerEl.querySelector( '.react-colorful__alpha' ),
].filter( ( el ) => !! el ) as Element[];
Comment on lines +40 to +44
Copy link
Member

Choose a reason for hiding this comment

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

Any chance there might be differences in the list of the existing interactive elements on the separate useEffect runs? If yes, should those somehow be listed as useEffect dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These elements should always be rendered in the component, as they are part of the react-colorful pickers. So I think we're good on this front :)


if ( interactiveElements.length === 0 ) {
return;
}

const doc = containerEl.ownerDocument;

const onPointerUp: EventListener = ( event ) => {
isDragging.current = false;
leftWhileDragging.current = false;
onDragEnd?.( event as MouseEvent );
};

const onPointerDown: EventListener = ( event ) => {
isDragging.current = true;
onDragStart?.( event as MouseEvent );
};

const onPointerLeave: EventListener = () => {
leftWhileDragging.current = isDragging.current;
};

// Try to detect if the user released the pointer while away from the
// current window. If the check is successfull, the dragEnd callback will
// called as soon as the pointer re-enters the window (better late than never)
const onPointerEnter: EventListener = ( event ) => {
const noPointerButtonsArePressed =
( event as PointerEvent ).buttons === 0;

if ( leftWhileDragging.current && noPointerButtonsArePressed ) {
onPointerUp( event );
}
};

// The pointerdown event is added on the interactive elements,
// while the remaining events are added on the document object since
// the pointer wouldn't necessarily be hovering the initial interactive
// element at that point.
interactiveElements.forEach( ( el ) =>
el.addEventListener( 'pointerdown', onPointerDown )
);
doc.addEventListener( 'pointerup', onPointerUp );
doc.addEventListener( 'pointerenter', onPointerEnter );
doc.addEventListener( 'pointerleave', onPointerLeave );

return () => {
interactiveElements.forEach( ( el ) =>
el.removeEventListener( 'pointerdown', onPointerDown )
);
doc.removeEventListener( 'pointerup', onPointerUp );
doc.removeEventListener( 'pointerenter', onPointerEnter );
doc.removeEventListener( 'pointerleave', onPointerUp );
};
}, [ onDragStart, onDragEnd, containerEl ] );
};

export const Picker = ( {
color,
enableAlpha,
onChange,
onDragStart,
onDragEnd,
containerEl,
}: PickerProps ) => {
const Component = enableAlpha
? RgbaStringColorPicker
: RgbStringColorPicker;
const rgbColor = useMemo( () => color.toRgbString(), [ color ] );

useOnPickerDrag( { containerEl, onDragStart, onDragEnd } );

return (
<Component
color={ rgbColor }
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/color-picker/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ export const ColorfulWrapper = styled.div`
align-items: center;
width: 216px;
height: auto;
overflow: hidden;
}

.react-colorful__saturation {
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/color-picker/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export interface PickerProps {
color: Colord;
enableAlpha: boolean;
onChange: ( nextColor: Colord ) => void;
containerEl: HTMLElement | null;
onDragStart?: ( event: MouseEvent ) => void;
onDragEnd?: ( event: MouseEvent ) => void;
}

export interface ColorInputProps {
Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/palette-edit/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ function ColorPickerPopover< T extends Color | Gradient >( {
() => ( {
shift: true,
offset: 20,
// Disabling resize as it would otherwise cause the popover to show
// scrollbars while dragging the color picker's handle close to the
// popover edge.
resize: false,
placement: 'left-start',
...receivedPopoverProps,
className: classnames(
Expand Down
Loading
Loading