Color Picker: improve UX when dragging the handle on top of editor canvas #54164
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
ciampo
added
[Type] Enhancement
|
@jameskoster in my tests, this approach seems to show the handle, and also allows the handle to be dragged around when the cursor is moved beyond the boundaries of the popover. Unfortunately, though, in my tests, the handle jumps around instead of moving smoothly when dragging the cursor outside of the popover. Even worse, after some amount of dragging, the popover with the color picker closes unexpectedly Here's a video to show what I mean: color.picker.popover.cursor.drag.mp4As I suspected, the reason for the handle "jitteriness" and the unexpected popover closure could be related to the fact that the pointer-events happen on top of an iframe — I tried quickly hiding the iframe in the dev tools and the dragging experience improved immediately. I treated this as a timeboxed effort on my end, and I don't really currently have much more time to look into this. |
|
That's a shame. I could live with the slight mis-positioning of the handle as it is still a usability improvement on trunk imo, but the intermittent disappearing act is going to be way too frustrating. Thanks for giving it a shot. |
ciampo
force-pushed
the
fix/color-picker-popover-overflow
branch
from
c7a3e6c to
fcc2cf0
Compare
|
Size Change: +719 B (0%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
|
I spent some extra time working on this, and I can confirm that the issue is caused by the underlying I put together a bit of a hacky workaround: when the user starts to drag the pointer on the saturation picker, we search for the Kapture.2023-09-29.at.10.39.19.mp4Now, the next steps could be to refine this approach into a more robust solution. Here's what I had in mind:
We'll also need to be careful because, if for any reason the code isn't able to restore pointer events on the iframe, the whole canvas would become impossible to interact with for our users. Pinging a few folks to see if the approach highlighted above seems good at least on paper (@youknowriad @jsnajdr @tyxla ) |
ciampo
commented
Sep 29, 2023
| // issue from happening. | ||
| const disableIframePointerEvents = () => { | ||
| iframe.style.pointerEvents = 'none'; | ||
| window.addEventListener( 'pointerup', restoreIframePointerEvents ); |
There was a problem hiding this comment.
Using window for the pointerup event because we'd need to make sure that pointer events on the iframe are restored whenever the pointer is released
|
@ciampo In the Iframe component, we do have a |
I wasn't aware of that, but it makes sense — without the event bubbling, I guess the color picker's handle wouldn't move at all, while it currently moves (albeit in a jittery and buggy way). But unfortunately, it still doesn't seem to fix the problem with the popover closing unexpectedly. Somehow, dragging the pointer around while tweaking the color picker's color somehow causes the I'll see if I can work out a solution. Regarding this part:
I was wondering what would the best way be for components across the editor to toggle the iframe not reacting to pointer events — maybe using React context? |
|
I think @ellatrix would be a good one to involve in this discussion |
ciampo
changed the title
ciampo
force-pushed
the
fix/color-picker-popover-overflow
branch
from
fcc2cf0 to
1afc9f6
Compare
…ps (prop name TBD)
… popover closes unexpectedly
ciampo
force-pushed
the
fix/color-picker-popover-overflow
branch
from
1afc9f6 to
4c93446
Compare
|
I went ahead and started to implement the solution described in #54164 (comment) to get a better feeling for it:
The approach seems to work, although:
I guess I'd like to discuss with @ellatrix if there are any better approaches to this — for example, I'd like to understand why is the iframe currently gaining focus while dragging the mouse around. |
|
Probably the iframe focus is native browser behaviour, I don't think we're explicitly setting focus on drag over. Maybe disable pointer events for everything outside the color picker while dragging? |
|
Testing an alternative approach in #55149 |
|
Closing in favour of #55149 |
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.
What?
Fixes #49267
Why?
As explained in #49267, currently when a
ColorPickeris rendered inside aPopover:This creates a sub-optimal user experience when using the color picker in the editor.
How?
overflowCSS propertiesresizeoption for thePopoverwhen rendering theColorPicker(which was also applying some unwantedoverflowrules)Testing Instructions
TBD
Screenshots or screencast