ColorPicker: improve UX of dragging the handle when in popover on top of the editor #55149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Flaky tests detected in d008cab. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6488089157
|
ciampo
force-pushed
the
try/color-picker-dropdown-backdrop
branch
from
942d7d1
to
2f6d2f5
Compare
ciampo
changed the title
ciampo
commented
Oct 9, 2023
Comment on lines
+54
to
+55
| onPickerDragStart, | ||
| onPickerDragEnd, |
There was a problem hiding this comment.
These "props" are only exposed via internal context, therefore not causing an API change to the component
ciampo
commented
Oct 9, 2023
Comment on lines
49
to
69
| const onPointerLeave: EventListener = () => { | ||
| leftWhileDragging.current = isDragging.current; | ||
| }; | ||
|
|
||
| const onPointerEnter: EventListener = ( event ) => { | ||
| const noPointerButtonsArePressed = | ||
| ( event as PointerEvent ).buttons === 0; | ||
|
|
||
| if ( leftWhileDragging.current && noPointerButtonsArePressed ) { | ||
| onPointerUp( event ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This is to make sure that the backdrop would disappear even if the user moves the cursor away from the window and stops dragging. The backdrop should disappear, at the latest, when the cursor comes back on top of the editor's window.
ciampo
commented
Oct 9, 2023
| @@ -46,3 +48,57 @@ const Template: StoryFn< typeof ColorPicker > = ( { onChange, ...props } ) => { | |||
| }; | |||
|
|
|||
| export const Default = Template.bind( {} ); | |||
|
|
|||
| export const WithDropdown: StoryFn< typeof ColorPicker > = ( { | |||
There was a problem hiding this comment.
This story is around for testing reason and will be removed before merging
ciampo
commented
Oct 9, 2023
Comment on lines
185
to
214
| { renderToggle( args ) } | ||
| { isOpen && ( | ||
| <Popover | ||
| ref={ ( node ) => setPopoverRef( node ) } | ||
| position={ position } | ||
| onClose={ close } | ||
| onFocusOutside={ closeIfFocusOutside } | ||
| expandOnMobile={ expandOnMobile } | ||
| headerTitle={ headerTitle } | ||
| focusOnMount={ focusOnMount } | ||
| // This value is used to ensure that the dropdowns | ||
| // align with the editor header by default. | ||
| offset={ 13 } | ||
| anchor={ | ||
| ! popoverPropsHaveAnchor | ||
| ? fallbackPopoverAnchor | ||
| : undefined | ||
| } | ||
| variant={ variant } | ||
| { ...popoverProps } | ||
| className={ classnames( | ||
| 'components-dropdown__content', | ||
| popoverProps?.className, | ||
| contentClassName | ||
| ) } | ||
| > | ||
| { renderContent( args ) } | ||
| </Popover> | ||
| ) } |
There was a problem hiding this comment.
This part didn't actually change, just its indentation
ciampo
commented
Oct 9, 2023
| */ | ||
| createPortal( | ||
| <DropdownPointerEventsCapture | ||
| onClick={ () => setShowBackdrop( false ) } |
There was a problem hiding this comment.
The backdrop should always disappear when the dragging ends, but if it was to stick around for any reason, the first click would dismiss it.
This is mostly to make sure that the page doesn't end up in a state in which most of the UI isn't clickable
ciampo
commented
Oct 9, 2023
ciampo
requested review from
youknowriad,
ellatrix,
tyxla,
chad1008,
andrewserong,
brookewp and
t-hamano
ciampo
added
[Type] Bug
chad1008
reviewed
Oct 10, 2023
There was a problem hiding this comment.
This solution feels good to me and works well for the color/saturation picket. It does look like we still have the same problem of lost focus closing the modal if we try dragging the handle(s) of the hue or alpha sliders outside of the ColorPicker modal.
Screen.Recording.2023-10-10.at.12.56.13.mov
This doesn't feel like it would be an issue anywhere near as often, but can/should we apply the overlay to these two handles as well?
There was a problem hiding this comment.
This is testing great for the color picker for me, and I also like that this means the color picker handle is no longer cut off when you drag to the edge of the picker:
Feels nice and stable when dragging around with the mouse cursor sometimes dragging out from the edge of the picker (in my screengrab, ignore the jumping due to scrollbar appearing when the color contrast warning appears) 👍
2023-10-11.12.50.14.mp4
Just left a minor comment about a selector, but the overall direction looks promising to me!
|
Well spotted @t-hamano ! In fact, the global styles sidebar renders a Since Let me know what you think about it! |
|
Size Change: +388 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Oct 11, 2023
There was a problem hiding this comment.
This is a very neat hack 👍 Thanks! 🙌
I'm happy with proceeding with a solution like this, I only would recommend adding some documentation to explain to our future selves what we're doing. Extracting the functionality to a separate hook could help self-documenting, too.
Comment on lines
+35
to
+39
| const interactiveElements = [ | ||
| containerEl.querySelector( '.react-colorful__saturation' ), | ||
| containerEl.querySelector( '.react-colorful__hue' ), | ||
| containerEl.querySelector( '.react-colorful__alpha' ), | ||
| ].filter( ( el ) => !! el ) as Element[]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
| const Component = enableAlpha | ||
| ? RgbaStringColorPicker | ||
| : RgbStringColorPicker; | ||
| const rgbColor = useMemo( () => color.toRgbString(), [ color ] ); | ||
|
|
||
| const isDragging = useRef( false ); | ||
| const leftWhileDragging = useRef( false ); | ||
| useEffect( () => { |
There was a problem hiding this comment.
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?
| @@ -147,6 +147,7 @@ export function CustomColorPickerDropdown( { | |||
| const popoverProps = useMemo< DropdownProps[ 'popoverProps' ] >( | |||
| () => ( { | |||
| shift: true, | |||
| resize: false, | |||
There was a problem hiding this comment.
Why is the resize: false necessary?
There was a problem hiding this comment.
Added a comment in 8c95ac1
| const Component = enableAlpha | ||
| ? RgbaStringColorPicker | ||
| : RgbStringColorPicker; | ||
| const rgbColor = useMemo( () => color.toRgbString(), [ color ] ); | ||
|
|
||
| const isDragging = useRef( false ); | ||
| const leftWhileDragging = useRef( false ); | ||
| useEffect( () => { |
There was a problem hiding this comment.
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).
brookewp
approved these changes
Oct 12, 2023
andrewserong
approved these changes
Oct 12, 2023
github-actions bot
pushed a commit
to Automattic/gutenberg-sentry
that referenced
this pull request
Oct 12, 2023
… of the editor (WordPress#55149) * ColorPicker: expose drag start / end handlers via context * ColorPicker: remove overflow hidden rule * Dropdown: add some sort of backdrop to capture pointer events * Add test story * Prevent resizing on color palette's popover, which also triggers overflow: hidden * Improve backdrop position by using react portal * Cleanup * Aria-hidden * Cleanup * Scope querySelector within color picker s container element * Listen for drag events also on hue and alpha sliders * Remove temporary styles * Remove temporary storybook example * CHANGELOG * Move pointer events trap from Dropdown to Popover component * Use state to grab ColorPicker's container element to make sure the component re-renders * Remove overflow:hidden from PaletteEdit component's popover * Update CHANGELOG * Remove dead code * Update snapshot * Move newly added ColorPicker logic to a separate hook * Add inline comment around the resize: false change * Add more comments
retrofox
pushed a commit
that referenced
this pull request
Oct 12, 2023
… of the editor (#55149) * ColorPicker: expose drag start / end handlers via context * ColorPicker: remove overflow hidden rule * Dropdown: add some sort of backdrop to capture pointer events * Add test story * Prevent resizing on color palette's popover, which also triggers overflow: hidden * Improve backdrop position by using react portal * Cleanup * Aria-hidden * Cleanup * Scope querySelector within color picker s container element * Listen for drag events also on hue and alpha sliders * Remove temporary styles * Remove temporary storybook example * CHANGELOG * Move pointer events trap from Dropdown to Popover component * Use state to grab ColorPicker's container element to make sure the component re-renders * Remove overflow:hidden from PaletteEdit component's popover * Update CHANGELOG * Remove dead code * Update snapshot * Move newly added ColorPicker logic to a separate hook * Add inline comment around the resize: false change * Add more comments
retrofox
added a commit
that referenced
this pull request
Oct 12, 2023
* Add Plugin post excerpt component * introduce and expose PluginPostExcerpt * fill post excerpt panel * minor code enhancement * add plugin post excerpt test * prefix plugin with __experimental * Update packages/edit-post/src/components/sidebar/plugin-post-excerpt/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update packages/edit-post/src/components/sidebar/post-excerpt/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update packages/edit-post/src/components/sidebar/post-excerpt/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update packages/edit-post/src/components/sidebar/post-excerpt/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update packages/edit-post/src/components/sidebar/plugin-post-excerpt/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Update packages/edit-post/src/components/sidebar/plugin-post-excerpt/test/index.js Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * ColorPicker: improve UX of dragging the handle when in popover on top of the editor (#55149) * ColorPicker: expose drag start / end handlers via context * ColorPicker: remove overflow hidden rule * Dropdown: add some sort of backdrop to capture pointer events * Add test story * Prevent resizing on color palette's popover, which also triggers overflow: hidden * Improve backdrop position by using react portal * Cleanup * Aria-hidden * Cleanup * Scope querySelector within color picker s container element * Listen for drag events also on hue and alpha sliders * Remove temporary styles * Remove temporary storybook example * CHANGELOG * Move pointer events trap from Dropdown to Popover component * Use state to grab ColorPicker's container element to make sure the component re-renders * Remove overflow:hidden from PaletteEdit component's popover * Update CHANGELOG * Remove dead code * Update snapshot * Move newly added ColorPicker logic to a separate hook * Add inline comment around the resize: false change * Add more comments * remove test snapshots --------- Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
stokesman
added a commit
that referenced
this pull request
Mar 22, 2024
* Partially revert "ColorPicker: improve UX of dragging the handle when in popover on top of the editor (#55149)" This reverts commit 9da6f48. * Use pointer capture to improve drag gesture UX * Use the context system again * Update snapshot * Remove unnecessary context provider * Add changelog entry
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* Partially revert "ColorPicker: improve UX of dragging the handle when in popover on top of the editor (WordPress#55149)" This reverts commit 9da6f48. * Use pointer capture to improve drag gesture UX * Use the context system again * Update snapshot * Remove unnecessary context provider * Add changelog entry
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #49267
Alternative approach to #54164
What?
Fixes an issue with dragging the handle of the color picker when the
ColorPickercomponent is rendered inside aPopoveron top of the editor canvas (aniframe)Why?
The current UX is poor and leads to a frustrating interaction when the color picker closes unexpectedly in the middle of dragging its handle, should the user move the cursor on top of the editor canvas.
The reason why this bug is happening in the first place is because of how pointer events behave when moving the cursor over an
iframe. By default,iframes would absorb all pointer events — and therefore the color picker handle wouldn't update at all when moving the pointer on top of the editor canvas.But the editor canvas' iframe has been tweaked to bubble certain events (including mouse move), and therefore uses don't experience the behaviour explained in the paragraph above. Instead, it looks like the cursor handle isn't updated as smoothly, and (even worse), moving the cursor while dragging on top of the editor canvas can result in the
iframeitself receiving focus, which in turn causes thePopoveraround theColorPickerto automatically close (it closes when losing focus).How?
The approach tried in #54164 works, but it results in many API changes to components, an extra context being added and in general quite a lot of code.
This solution, instead, tries to prevent the problem from happening by working solely at
@wordpress/componentslevel.Basically, whenever a
ColorPickeris rendered inside aPopovercomponent, thePopovercomponent will render an additional backdrop element only while the color picker handle is being dragged. The backdrop serves as a pointer events "trap", avoiding them from ever reaching the underlyingiframeand therefore causing the bug described above.A better solution, in the future, could be to allow
Popoverto expose amodalprop (similarly to how thePopovercomponent fromariakitalready does), which would basically implement this same solution but in a more robust way.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before
color-picker-before.mp4
After (note: the backdrop is purposefully tinted to better showcase the solution proposed)
color-picker-after.mp4