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

Try obviating Popover pointer event trap #59449

Merged
merged 6 commits into from
Mar 22, 2024
Merged

Try obviating Popover pointer event trap #59449

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
513eeec
Partially revert "ColorPicker: improve UX of dragging the handle when…
stokesman Feb 28, 2024
4d4b915
Use pointer capture to improve drag gesture UX
stokesman Feb 28, 2024
067dc50
Use the context system again
stokesman Feb 29, 2024
9f94226
Update snapshot
stokesman Feb 29, 2024
78d23f0
Remove unnecessary context provider
stokesman Mar 21, 2024
21acd77
Add changelog entry
stokesman Mar 21, 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/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- `InputControl`: Ignore IME events when `isPressEnterToChange` is enabled ([#60090](https://github.com/WordPress/gutenberg/pull/60090)).

### Internal

- `Popover`, `ColorPicker`: Obviate pointer event trap #59449 ([#59449](https://github.com/WordPress/gutenberg/pull/59449)).

## 27.2.0 (2024-03-21)

- `Dropdown` : Add styling support for `MenuGroup` ([#59723](https://github.com/WordPress/gutenberg/pull/59723)).
Expand Down
28 changes: 3 additions & 25 deletions packages/components/src/color-picker/component.tsx
View file
Open in desktop
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, useMergeRefs } from '@wordpress/compose';
import { useDebounce } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

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

// Context
onPickerDragStart,
onPickerDragEnd,
...divProps
} = 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 );
};
} = useContextSystem( props, 'ColorPicker' );

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

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

/**
* 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( () => {
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[];

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 ) => {
export const Picker = ( { color, enableAlpha, onChange }: PickerProps ) => {
const Component = enableAlpha
? RgbaStringColorPicker
: RgbStringColorPicker;
const rgbColor = useMemo( () => color.toRgbString(), [ color ] );

useOnPickerDrag( { containerEl, onDragStart, onDragEnd } );

return (
<Component
color={ rgbColor }
onChange={ ( nextColor ) => {
onChange( colord( nextColor ) );
} }
// Pointer capture fortifies drag gestures so that they continue to
// work while dragging outside the component over objects like
// iframes. If a newer version of react-colorful begins to employ
// pointer capture this will be redundant and should be removed.
onPointerDown={ ( { currentTarget, pointerId } ) => {
currentTarget.setPointerCapture( pointerId );
Copy link
Member

Choose a reason for hiding this comment

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

TIL setPointerCapture!

} }
onPointerUp={ ( { currentTarget, pointerId } ) => {
currentTarget.releasePointerCapture( pointerId );
} }
/>
);
};
3 changes: 0 additions & 3 deletions packages/components/src/color-picker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ 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
155 changes: 57 additions & 98 deletions packages/components/src/popover/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,7 @@ import {
placementToMotionAnimationProps,
getReferenceElement,
} from './utils';
import {
contextConnect,
useContextSystem,
ContextSystemProvider,
} from '../context';
import { contextConnect, useContextSystem } from '../context';
import type { WordPressComponentProps } from '../context';
import type {
PopoverProps,
Expand Down Expand Up @@ -113,7 +109,7 @@ const getPopoverFallbackContainer = () => {
return container;
};

const UnconnectedPopover = (
const UnforwardedPopover = (
props: Omit<
WordPressComponentProps< PopoverProps, 'div', false >,
// To avoid overlaps between the standard HTML attributes and the props
Expand Down Expand Up @@ -390,100 +386,63 @@ const UnconnectedPopover = (
const isPositioned =
( ! shouldAnimate || animationFinished ) && x !== null && y !== null;

// In case a `ColorPicker` component is rendered as a child of `Popover`,
// the `Popover` component can be notified of when the user is dragging
// parts of the `ColorPicker` UI (this is possible because the `ColorPicker`
// component exposes the `onPickerDragStart` and `onPickerDragEnd` props
// via internal context).
// While the user is performing a pointer drag, the `Popover` will render
// a transparent backdrop element that will serve as a "pointer events trap",
// making sure that no pointer events reach any potential `iframe` element
// underneath (like, for example, the editor canvas in the WordPress editor).
const [ showBackdrop, setShowBackdrop ] = useState( false );
const contextValue = useMemo(
() => ( {
ColorPicker: {
onPickerDragStart() {
setShowBackdrop( true );
},
onPickerDragEnd() {
setShowBackdrop( false );
},
},
} ),
[]
);

let content = (
<>
{ showBackdrop && (
<div
className="components-popover-pointer-events-trap"
aria-hidden="true"
onClick={ () => setShowBackdrop( false ) }
/>
<motion.div
className={ classnames( className, {
'is-expanded': isExpanded,
'is-positioned': isPositioned,
// Use the 'alternate' classname for 'toolbar' variant for back compat.
[ `is-${
computedVariant === 'toolbar'
? 'alternate'
: computedVariant
}` ]: computedVariant,
} ) }
{ ...animationProps }
{ ...contentProps }
ref={ mergedFloatingRef }
{ ...dialogProps }
tabIndex={ -1 }
>
{ /* Prevents scroll on the document */ }
{ isExpanded && <ScrollLock /> }
{ isExpanded && (
<div className="components-popover__header">
<span className="components-popover__header-title">
{ headerTitle }
</span>
<Button
className="components-popover__close"
icon={ close }
onClick={ onClose }
/>
</div>
) }
<motion.div
className={ classnames( 'components-popover', className, {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 🙏

'is-expanded': isExpanded,
'is-positioned': isPositioned,
// Use the 'alternate' classname for 'toolbar' variant for back compat.
[ `is-${
computedVariant === 'toolbar'
? 'alternate'
: computedVariant
}` ]: computedVariant,
} ) }
{ ...animationProps }
{ ...contentProps }
ref={ mergedFloatingRef }
{ ...dialogProps }
tabIndex={ -1 }
>
{ /* Prevents scroll on the document */ }
{ isExpanded && <ScrollLock /> }
{ isExpanded && (
<div className="components-popover__header">
<span className="components-popover__header-title">
{ headerTitle }
</span>
<Button
className="components-popover__close"
icon={ close }
onClick={ onClose }
/>
</div>
) }
<div className="components-popover__content">
<ContextSystemProvider value={ contextValue }>
{ children }
</ContextSystemProvider>
<div className="components-popover__content">{ children }</div>
{ hasArrow && (
<div
ref={ arrowCallbackRef }
className={ [
'components-popover__arrow',
`is-${ computedPlacement.split( '-' )[ 0 ] }`,
].join( ' ' ) }
style={ {
left:
typeof arrowData?.x !== 'undefined' &&
Number.isFinite( arrowData.x )
? `${ arrowData.x }px`
: '',
top:
typeof arrowData?.y !== 'undefined' &&
Number.isFinite( arrowData.y )
? `${ arrowData.y }px`
: '',
} }
>
<ArrowTriangle />
</div>
{ hasArrow && (
<div
ref={ arrowCallbackRef }
className={ [
'components-popover__arrow',
`is-${ computedPlacement.split( '-' )[ 0 ] }`,
].join( ' ' ) }
style={ {
left:
typeof arrowData?.x !== 'undefined' &&
Number.isFinite( arrowData.x )
? `${ arrowData.x }px`
: '',
top:
typeof arrowData?.y !== 'undefined' &&
Number.isFinite( arrowData.y )
? `${ arrowData.y }px`
: '',
} }
>
<ArrowTriangle />
</div>
) }
</motion.div>
</>
) }
</motion.div>
);

const shouldRenderWithinSlot = slot.ref && ! inline;
Expand Down Expand Up @@ -533,7 +492,7 @@ const UnconnectedPopover = (
* ```
*
*/
export const Popover = contextConnect( UnconnectedPopover, 'Popover' );
export const Popover = contextConnect( UnforwardedPopover, 'Popover' );

function PopoverSlot(
{ name = SLOT_NAME }: { name?: string },
Expand Down
Loading
Loading