[Gradient Tool]: Fix closing of popover when the angle control is clicked #40735
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
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
mirka,
ciampo,
jasmussen,
jorgefilipecosta,
andrewserong and
aaronrobertshaw
|
Size Change: -10 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
stokesman
approved these changes
May 2, 2022
There was a problem hiding this comment.
I tested this with Gutenberg.run and the issue is fixed. The changes make sense.
Coincidentally, were #40518 merged it would allow fixing this by removing any focus handling here but so far we're not to looking to backport that one so this is a great fix in the meantime.
stokesman
reviewed
May 2, 2022
| @@ -28,12 +28,11 @@ function AngleCircle( { value, onChange, ...props } ) { | |||
|
|
|||
| const changeAngleToPosition = ( event ) => { | |||
There was a problem hiding this comment.
I just noticed this is the handler for onDragMove and onDragEnd (in addition to being called by onDragStart) so it's executing event.target.focus() way more than necessary. Perhaps even the old blur() would work if it were only called from onDragStart. Not that I'm suggesting that blur() should be used.
There was a problem hiding this comment.
Good point! I didn't notice any performance penalty in my testing, but I suppose we could add a check before calling .focus() to only do that if the element isn't already focused?
There was a problem hiding this comment.
Definitely agree here. I noticed this as well that these events are tied together in a bit weird fashion. I'll land this as is to fix the issue for now, and we need to check how to separate/handle the drag events better.
andrewserong
approved these changes
May 3, 2022
There was a problem hiding this comment.
Thanks for fixing this up @ntsekouras!
I couldn't find the reason for making the popover close and my guess is that there is someone else listening to that event
I was curious so did a little bit of digging into the Popover component:
In the Popover, the dialog props are expanded to the wrapper element of the Popover here, and the props are retrieved from useDialog here. Within useDialog, the props that contain the blur handlers are retrieved via a call to useFocusOutside here, and in useFocusOutside this is where onBlur is set.
So, that appears to be how the Popover is set up to watch for blur events within its container. There's a bunch of other handlers in useFocusOutside to cancel the call to the onFocusOutside callback for onFocus, onMouseDown, onMouseUp, etc. The onBlur callback queues the check via a setTimeout to give the other handlers a chance to cancel / prevent the blur callback from firing, which appears to be how we get the "focus outside" behaviour.
The approach here to skip blurring the element and instead call .focus() to intentionally focus the desired element makes sense to me as a way to fix the underlying issue by avoiding blurring from within the Popover.
Perhaps, separately, we might need to add additional logic to the useFocusOutside hook to also take into account the drag event handlers (e.g. onDragMove, onDragEnd, etc) so that they're recognised as valid reasons not to call the focus outside callback.
At any rate, this change tests well, and LGTM!
|
Thanks for the reviews and also for digging @andrewserong!
That sounds like a plan. |
ntsekouras
deleted the
fix/gradient-popover-closing-on-clicking-angle-circle
branch
|
I cherry picked this change into |
adamziel
pushed a commit
that referenced
this pull request
May 3, 2022
|
Thank you folks for collaborating on this PR! I didn't get to leave a review, but changes LGTM 🚀 Could we just add a CHANGELOG entry for this bug fix in a follow-up ? |
ntsekouras
added a commit
that referenced
this pull request
May 3, 2022
ntsekouras
added a commit
that referenced
this pull request
May 4, 2022
ntsekouras
added a commit
that referenced
this pull request
May 4, 2022
adamziel
removed
the
Backport to WP 6.7 Beta/RC
19 tasks
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: #40661
From the issue:
How?
In this PR there were changes that affected InputControl and how it updates/syncs values. That PR added a line of code in angle circle control to programmatically
blurthe active element.I couldn't find the reason for making the popover close and my guess is that there is someone else listening to that event and/or a rerender somewhere is triggered. I'd love to learn why it was happening if anyone knows or debugs a bit 😄 --cc @ciampo @aaronrobertshaw
What I did here was explicitly focus the current node(angle circle) if clicked, to contain the triggered event with the same effect.
Testing Instructions
Before
After
Screen.Recording.2022-04-29.at.5.07.21.PM.mov