FocalPointPicker: Allow updating its value while dragging. #38247

razwan · 2022-01-26T16:24:10Z

In one of my projects I've created a HOC that adds a snapping functionality to the FocalPointPicker component but after #28676 it stopped working.

The onDrag event is a nice perk that should make possible most of the things that were accomplishable before. However, there's a slight change that doesn't allow me to do what I want and that's checking to see if the user is dragging when the componentDidUpdate runs before updating state.

I can't really see why that's the better thing to do or is it that we could go ahead without it without any worries.
@stokesman, would you mind dropping some knowledge on me?

I would really appreciate any feedback. Thanks!

…is interacting with it

github-actions · 2022-01-26T16:24:59Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @razwan! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

stokesman · 2022-01-31T20:38:26Z

Hi @razwan, This is indeed how the logic was before #28676. I believe that I made that change thinking it would be a minor optimization and can't recall any critical function of it. I didn't imagine it could have been a breaking change but your case proves otherwise.

I think changing it back (as in 8705154) should be fine. Though perhaps it would be nicer to intentionally support such use cases. For example, if the doDrag function were to check for a return value from onDrag and if found use it to update the state. Then without even making an HOC, I think it would be possible to create the snapping functionality. What do you think?

Here's my quick take on showing what I mean with the doDrag function (changes only the last two statements):

	doDrag( event ) {
		// Prevents text-selection when dragging.
		event.preventDefault();
		const value = this.getValueFromPoint(
			{ x: event.pageX, y: event.pageY },
			event.shiftKey
		);
		const modifiedValue = this.props.onDrag?.( value, event );
		this.updateValue( modifiedValue ?? value );
	}

…rag callback

razwan · 2022-02-01T13:26:54Z

Hi @stokesman! Thanks for your feedback!

Even though it feels safer, I believe that the change you're proposing is somewhat obscure rather than expected behaviour. It's nonetheless a way of accessing the updateValue method and it's pretty clear once you take a look at the source code. However, what makes me believe that you're change is the way to go, is that my codebase would work as intended with only one prop added to the component (rather than caching the values inside onDrag and updating the value prop when needed)

onDrag={ maybeSnapFocalPoint }

razwan · 2022-02-01T13:30:50Z

I've updated the pull request. @ajitbohra could you take a look at this? Thanks!

razwan · 2022-02-01T14:32:29Z

@stokesman any idea on why this test fails?

stokesman · 2022-02-01T18:59:30Z

That test failure is due the the implicit return of the event handlers in the test. This line:

gutenberg/packages/components/src/focal-point-picker/test/index.js

Line 48 in d257d32

handlers[ name ] = ( ...all ) => eventLogger( name, all );

Needs to add brackets around the function body:

	handlers[ name ] = ( ...all ) => {
 		eventLogger( name, all );
 	}

That does indicate this could be a breaking change for some consuming code inadvertently returning a value. I feel that is so unlikely that we don't need to worry about it but, if we do, guarding against it should be simple enough.

If this approach is deemed favorable, we'll want to update the docs for onDrag and add an entry to the changelog. Also, the title of the PR should be updated to reflect the approach. Again, all that can wait for a second opinion on the proposed change.

@ciampo, given this is component territory, I'll add you as a reviewer.

ciampo · 2022-02-02T16:07:30Z

Thank you for the ping, @stokesman ! @mirka or I will try to give this PR a look in the next days :)

ciampo · 2022-02-04T17:03:48Z

Hey @stokesman and @razwan, I had a quick look at this PR (and #28676) and, if I understand correctly:

before Declunkify focal point picker dragging #28676, FocalPointPicker used to call the onChange callback continuously while the user was dragging with their pointer
since Declunkify focal point picker dragging #28676, the onChange callback is only called at the end of the drag interaction, and a new callback was added (onDrag)
@razwan's "snap" functionality relied on the onChange callback, and how quickly it updated. But with the changes mentioned above, the snapping functionality breakes

If what I understand is correct (from my limited knowledge of the FocalPointPicker component), I see a couple of potential solutions

increase the frequency at which onDrag is called — that means that @razwan could rely on the onDrag callback to achieve the snapping functionality
implement the snapping functionality directly into the component (e.g. by adding something like a snapGrid prop)

Please do let me know if what I'm proposing makes sense at all, or if I missed something while going through this PR!

stokesman · 2022-02-04T18:03:54Z

Hi @ciampo, thanks for having a look. Your last solution suggestion makes sense but it seems the root of this issue wasn't clear. I'll try and clarify/summarize.

The change from #28676 that broke @razwan’s component was the addition to componentDidUpdate of a condition to avoid state updates from props while dragging:

gutenberg/packages/components/src/focal-point-picker/index.js

Lines 84 to 86 in ea79ef6

    
           if ( ! isDragging && ( value.x !== x || value.y !== y ) ) { 
        
           	this.setState( { percentages: this.props.value } ); 
        
           }

As the title of this PR still reflects, @razwan’s first commit 8705154 merely removed that condition. I agreed that it should be a safe change. Though I had the idea that we could ease the implementation of such a use case and suggested #38247 (comment) an alternate approach.

As I see it we have the following options (last one from @ciampo):

Remove the no-update-while-dragging condition 8705154
Add the update-state-by-optional-return-from-onDrag feature 467f3a3
implement the snapping functionality directly into the component (e.g. by adding something like a snapGrid prop)

I favor either of the latter which are the same general idea. @ciampo’s suggestion is probably a bit safer and more explicit.

ciampo · 2022-02-04T18:25:05Z

Your last solution suggestion makes sense but it seems the root of this issue wasn't clear. I'll try and clarify/summarize.

Ah, I see! Thank you for the clarification. As for the suggested approaches:

Remove the no-update-while-dragging condition 8705154

Wouldn't this cause a potential perf regression by calling setState very often?

Add the update-state-by-optional-return-from-onDrag feature 467f3a3

It looks like this solution requires the onDrag function to return a "modified value" of the coordinates — wouldn't that be a bit of an implicit change? I think that commonly, this sort of callbacks (like onDrag in this case) are not expected to return a value.

implement the snapping functionality directly into the component (e.g. by adding something like a snapGrid prop)

This solution looks clear enough

I favor either of the latter which are the same general idea

Same for me. Allowing onDrag to act as a "transform" of the coordinates is potentially powerful and gives more flexibility to the consumers of this component to implement their own functionality on top of it.

On the other hand, it'd be nice if FocalPointPicker offered some snapping functionality out-of-the-box — this last option would be my favourite.

razwan · 2022-02-07T15:39:58Z

Hi @ciampo! Thanks for tuning in!

I agree that implementing a snapping functionality directly into the FocalPointPicker would be favourable in some cases. But this is just one use case. There are plenty of ways in which the component could be extended. For instance you may want to restrict (for whatever reason) the area in which the focal point can be dragged. Also, the snapping functionality requires not only a snapGrid prop, but also some sort of threshold etc. To me, these are the kind of features that developers should be able to develop on top of the core component.

After giving it a second thought I favor dropping the isDragging condition preventing values updating since it doesn't create any troubles out of the box. If someone choses to use the onDrag callback, it's their responsibility to do that in a performant manner.

I don't see how if focal point is snapped (or sticky) while dragging then the performance would suffer. The performance is rather greatly impacted when you update the block attributes while dragging but that's a whole another issue.

Am I missing anything?

stokesman · 2022-02-07T18:15:25Z

… the snapping functionality requires not only a snapGrid prop, but also some sort of threshold etc.

I imagined that such a snapGrid prop would be take a function that receives the coordinates and can return modified ones. A better name for it would be something like resolvePoint. It is nicer than using onDrag in that there is no concern about cases where an existing implementation returns an incompatible value (e.g. our failing test). It could be made more comprehensive too by calling it from updateValue so that even changes from the input controls or keyboard events could also be affected (instead only the drag events).

While there really should be no performance concern for the first option. I simply prefer that we facilitate such use cases. As @razwan wrote #38247 (comment)

However, what makes me believe that you're change is the way to go, is that my codebase would work as intended with only one prop added to the component (rather than caching the values inside onDrag and updating the value prop when needed)

ciampo · 2022-02-08T14:20:42Z

There are plenty of ways in which the component could be extended... To me, these are the kind of features that developers should be able to develop on top of the core component.

This is definitely an interesting angle on the matter — allowing consumers to extend the core component is definitely a powerful pattern

When referring at options highlighted in this comment, if I understand correctly:

@razwan's preferred option would be what we called option no. 1 (8705154)
@stokesman suggests that we introduce a new prop (e.g. called resolvePoint), called from updateValue. This prop would be a function that accepts a point, and returns the transformed point coordinates.

If that's the case, my preference would be for @stokesman 's solution, which seems cleaner and still enables the extension of the component like @razwan 's wishes for.

…eturning new value from onDrag callback

…ng resolvePoint prop

razwan · 2022-02-08T16:19:12Z

This is definitely an interesting angle on the matter — allowing consumers to extend the core component is definitely a powerful pattern

There's a fine line there but I understand that under such heavy development most of the times the focus is on adding the features needed by the core blocks. However I enjoy components that are very little opinionated and highly extensible.

If that's the case, my preference would be for @stokesman 's solution, which seems cleaner and still enables the extension of the component like @razwan 's wishes for.

The resolvePoint solution seems more than reasonable to me. I've made changes to the PR accordingly. Have you can have a look @ciampo.

ciampo

There's a fine line there but I understand that under such heavy development most of the times the focus is on adding the features needed by the core blocks. However I enjoy components that are very little opinionated and highly extensible.

Makes sense. It would be actually cool if you could add a Storybook example in this PR implementing snapping through the new resolvePoint function!

The resolvePoint solution seems more than reasonable to me. I've made changes to the PR accordingly. Have you can have a look @ciampo.

Sure thing! Apart from the inline comment:

could you add an entry to the CHANGELOG ?
could you add a unit test for the resolvePoint function? A simple way would be to pass, as the resolvePoint prop, a spy function (and then check that it was called correctly). We could add multiple tests to verify that it gets called also while interacting with the keyboard

Thank you!

ciampo · 2022-02-08T17:46:35Z

packages/components/src/focal-point-picker/index.js

-		this.updateValue( value );
-		this.props.onDrag?.( value, event );
+		const resolvedValue = this.props.resolvePoint?.( value ) ?? value;
+		this.updateValue( resolvedValue );


Like @stokesman suggested earlier, should we call resolvePoint inside updateValue instead?

…t controls or keyboard ar affected

…lPointPicker

razwan · 2022-02-09T09:34:44Z

storybook
changelog
unit test

Done. @ciampo

ciampo · 2022-02-09T09:35:49Z

Done

Awesome! I'll try to give a proper look later today

packages/components/src/focal-point-picker/README.md

ciampo

Hey @razwan , thank you for working on this.

The code looks good and tests well — I just left a couple of comments, but we're quite close to being able to merge this!

packages/components/src/focal-point-picker/stories/index.js

ciampo · 2022-02-09T20:57:03Z

packages/components/src/focal-point-picker/test/index.js

+					resolvePoint={ () => {
+						spy();
+						return { x: 0.91, y: 0.42 };
+					} }


Q: when checking this test on my local machine, I tried to make the test fail by modifying the resolvePoint function to

resolvePoint={ ( value ) => { spy(); return value; } }

The test fails (as expected), but the values received by the onChange spy are not x: 0.25, y: 0.25, but instead are x: 0, y: 0. I wasn't expecting this behaviour, do you know why would this happen?

My guess is that the main reason is that the picker doesn't have width and height props. Also, fireEvent is called without any other parameters but am not sure if the case would be the same if the picker had set dimensions.

Ok, so I haven't been able to use mouseMove to create a proper emulation of a user interaction. However, I managed that by using fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } ); which triggers onChange with an "expected" value of x: 0.25, y: 0.24 making my test fail. That was because onChange was called with the same value updateValue would received.

I added a second parameter to the updateValue method which is a callback that is passed to the setState call. Moving the onChange call inside that callback fixed the issue and made the test pass. Everything looks fine now.

Thank you for the explanation, makes sense!

I suggest we add another unit test to check this onChange baheviour:

diff --git a/packages/components/src/focal-point-picker/test/index.js b/packages/components/src/focal-point-picker/test/index.js index 145ca0d019..ac4f1af0e3 100644 --- a/packages/components/src/focal-point-picker/test/index.js +++ b/packages/components/src/focal-point-picker/test/index.js @@ -76,6 +76,8 @@ describe( 'FocalPointPicker', () => { } } /> ); + + // Click and press arrow up const dragArea = getByRole( 'button' ); act( () => { fireEvent.mouseDown( dragArea ); @@ -98,5 +100,23 @@ describe( 'FocalPointPicker', () => { rerender( <Picker value={ { x: 0.93, y: 0.5 } } /> ); expect( xInput.value ).toBe( '93' ); } ); + + it( 'call onChange with the expected values', async () => { + const spyChange = jest.fn(); + const { getByRole } = render( + <Picker value={ { x: 0.14, y: 0.62 } } onChange={ spyChange } /> + ); + + // Click and press arrow up + const dragArea = getByRole( 'button' ); + act( () => { + fireEvent.mouseDown( dragArea ); + fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } ); + } ); + expect( spyChange ).toHaveBeenCalledWith( { + x: '0.14', + y: '0.61', + } ); + } ); } ); } );

I'm not sure I understand what you're asking for. Doesn't the initial test cover this behaviour? Do you want me to remove the onChange part from that test and then make another one in the controllability section that only tests the onChange part?

I'm proposing to add another unit test aimed at testing the behaviour that I highlighted earlier and you fixed in your latest commits by using the setState callback to call onChange.

Earlier, when removing resolvePoint from this test, the onChange spy was receiving x: 0, y: 0 instead of the expected value. Thanks to your fix, the onChange would now receive the correct values. My proposal is to add an additional unit test to specifically test that (note that, in the new test, the resolvePoint prop is not specified)

Hope I managed to explain myself better :)

I'll add the unit test but I want to make sure we're on the same page.

By using mouseDown and mouseMove we still receive { x: 0.00, y: 0.00 } instead of the values with which the picker is initialised.

The bug that was fixed by moving the onChange inside the setState callback was related to the resolvePoint prop.

this.updateValue( next ); // next was being passed to resolvePoint here this.props.onChange( next ); // but not here

This unit test becomes a test to future proof the keyboard interaction (rather for the UP key press). Am I right?

By using mouseDown and mouseMove we still receive { x: 0.00, y: 0.00 } instead of the values with which the picker is initialised.

I see, I missed that detail. Do you think it's worth investigating this in a follow-up PR?

…ue and unit test passes

ciampo

Thank you @razwan and @stokesman for the excellent collaboration effort!

I left a couple of minor comments, but as soon as they are addressed we can merge this PR 🚀

packages/components/CHANGELOG.md

ciampo · 2022-02-10T09:40:40Z

packages/components/src/focal-point-picker/test/index.js

+					resolvePoint={ () => {
+						spy();
+						return { x: 0.91, y: 0.42 };
+					} }


Thank you for the explanation, makes sense!

I suggest we add another unit test to check this onChange baheviour:

diff --git a/packages/components/src/focal-point-picker/test/index.js b/packages/components/src/focal-point-picker/test/index.js index 145ca0d019..ac4f1af0e3 100644 --- a/packages/components/src/focal-point-picker/test/index.js +++ b/packages/components/src/focal-point-picker/test/index.js @@ -76,6 +76,8 @@ describe( 'FocalPointPicker', () => { } } /> ); + + // Click and press arrow up const dragArea = getByRole( 'button' ); act( () => { fireEvent.mouseDown( dragArea ); @@ -98,5 +100,23 @@ describe( 'FocalPointPicker', () => { rerender( <Picker value={ { x: 0.93, y: 0.5 } } /> ); expect( xInput.value ).toBe( '93' ); } ); + + it( 'call onChange with the expected values', async () => { + const spyChange = jest.fn(); + const { getByRole } = render( + <Picker value={ { x: 0.14, y: 0.62 } } onChange={ spyChange } /> + ); + + // Click and press arrow up + const dragArea = getByRole( 'button' ); + act( () => { + fireEvent.mouseDown( dragArea ); + fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } ); + } ); + expect( spyChange ).toHaveBeenCalledWith( { + x: '0.14', + y: '0.61', + } ); + } ); } ); } );

…olvePoint prop

ciampo

Thank you @razwan for you work! We can merge this PR as soon as conflicts are solved and CI is green 🚀

stokesman · 2022-02-10T17:06:15Z

Thanks for the work on this @razwan.

We can merge this PR as soon as conflicts are solved and CI is green

Doing so. Thanks for the reviews @ciampo.

github-actions · 2022-02-10T17:07:08Z

Congratulations on your first merged pull request, @razwan! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

razwan · 2022-02-10T17:41:44Z

Thanks @ciampo and @stokesman for your help!
I've just connected my github account on wordpress.org and hopefully I get on the contributors list 😅

On to the next one. See you around!

ciampo · 2022-02-11T09:48:46Z

Thanks @ciampo and @stokesman for your help! I've just connected my github account on wordpress.org and hopefully I get on the contributors list 😅

On to the next one. See you around!

It's been great to collaborate! Please feel free to ping me or @mirka for any @wordpress/components-related issue or PR !

Update values of focal point picker with incoming props even if user …

8705154

…is interacting with it

razwan requested a review from ajitbohra as a code owner January 26, 2022 16:24

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 26, 2022

razwan added 2 commits February 1, 2022 15:20

Merge branch 'trunk' into update/focal-point-picker-incoming-props

f41d120

Update current value of focal point picker with value returned by onD…

467f3a3

…rag callback

stokesman added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Feb 1, 2022

stokesman requested a review from ciampo February 1, 2022 18:59

ciampo requested a review from mirka February 2, 2022 16:06

Use resolvePoint prop to update focal point picker value instead of r…

5ed3b2e

…eturning new value from onDrag callback

razwan changed the title ~~Update values FocalPointPicker w/ incoming props even if user is interacting~~ Add resolvePoint prop to FocalPointPicker component to allow updating value while dragging Feb 8, 2022

razwan added 3 commits February 8, 2022 18:08

Call onDrag with the maybe modified value returned by resolvePoint

4aac1f6

Add docs for the recently added resolvePoint prop

95cb4b4

Reverse order of updateValue and onDrag to be the same as before addi…

3119b56

…ng resolvePoint prop

ciampo reviewed Feb 8, 2022

View reviewed changes

Call resolvePoint from updateValue so that even changes from the inpu…

54d8cc0

…t controls or keyboard ar affected

razwan added 3 commits February 9, 2022 10:25

Add storybook example of implementing snapping functionality for foca…

b82a001

…lPointPicker

Add mention of the resolvePoint prop addition in the changelog

7205351

Add unit test for resolvePoint prop

f74c789

Fix unit test for FocalPointPicker resolvePoint prop

541c658

stokesman reviewed Feb 9, 2022

View reviewed changes

packages/components/src/focal-point-picker/README.md Outdated Show resolved Hide resolved

ciampo reviewed Feb 9, 2022

View reviewed changes

razwan and others added 5 commits February 9, 2022 23:38

Fix typo in FocalPointPicker story

720a1c6

Update resolvePoint description in FocalPointPicker readme

518475a

Better unit test for resolvePoint

6d62ff0

Add callback to updateValue so onChange gets called with resolved val…

40f9e9e

…ue and unit test passes

Merge branch 'trunk' into update/focal-point-picker-incoming-props

3bd8847

ciampo approved these changes Feb 10, 2022

View reviewed changes

razwan added 2 commits February 10, 2022 13:52

Move changelog entry to unreleased section

9ac9517

Add unit test to test onChange after keyboard interaction without res…

9ea5d61

…olvePoint prop

ciampo approved these changes Feb 10, 2022

View reviewed changes

Merge branch 'trunk' into update/focal-point-picker-incoming-props

38c6446

stokesman merged commit e60cc60 into WordPress:trunk Feb 10, 2022

github-actions bot added this to the Gutenberg 12.7 milestone Feb 10, 2022

razwan deleted the update/focal-point-picker-incoming-props branch February 10, 2022 18:07

cbravobernal changed the title ~~Add resolvePoint prop to FocalPointPicker component to allow updating value while dragging~~ FocalPointPicker: Add resolvePoint prop to allow updating its value while dragging. Feb 25, 2022

cbravobernal changed the title ~~FocalPointPicker: Add resolvePoint prop to allow updating its value while dragging.~~ FocalPointPicker: Allow updating its value while dragging. Feb 26, 2022

stokesman mentioned this pull request Jul 18, 2022

Refactor FocalPointPicker to function component #39168

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FocalPointPicker: Allow updating its value while dragging. #38247

FocalPointPicker: Allow updating its value while dragging. #38247

razwan commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

stokesman commented Jan 31, 2022 •

edited

Loading

razwan commented Feb 1, 2022

razwan commented Feb 1, 2022

razwan commented Feb 1, 2022

stokesman commented Feb 1, 2022

ciampo commented Feb 2, 2022

ciampo commented Feb 4, 2022

stokesman commented Feb 4, 2022

ciampo commented Feb 4, 2022

razwan commented Feb 7, 2022

stokesman commented Feb 7, 2022

ciampo commented Feb 8, 2022

razwan commented Feb 8, 2022

ciampo left a comment

ciampo Feb 8, 2022

razwan commented Feb 9, 2022

ciampo commented Feb 9, 2022

ciampo left a comment

ciampo Feb 9, 2022

razwan Feb 9, 2022

razwan Feb 9, 2022

ciampo Feb 10, 2022

razwan Feb 10, 2022

ciampo Feb 10, 2022

razwan Feb 10, 2022

ciampo Feb 10, 2022

ciampo left a comment

ciampo Feb 10, 2022

ciampo left a comment

stokesman commented Feb 10, 2022

github-actions bot commented Feb 10, 2022

razwan commented Feb 10, 2022

ciampo commented Feb 11, 2022

FocalPointPicker: Allow updating its value while dragging. #38247

FocalPointPicker: Allow updating its value while dragging. #38247

Conversation

razwan commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

stokesman commented Jan 31, 2022 • edited Loading

razwan commented Feb 1, 2022

razwan commented Feb 1, 2022

razwan commented Feb 1, 2022

stokesman commented Feb 1, 2022

ciampo commented Feb 2, 2022

ciampo commented Feb 4, 2022

stokesman commented Feb 4, 2022

ciampo commented Feb 4, 2022

razwan commented Feb 7, 2022

stokesman commented Feb 7, 2022

ciampo commented Feb 8, 2022

razwan commented Feb 8, 2022

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razwan commented Feb 9, 2022

ciampo commented Feb 9, 2022

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

stokesman commented Feb 10, 2022

github-actions bot commented Feb 10, 2022

razwan commented Feb 10, 2022

ciampo commented Feb 11, 2022

stokesman commented Jan 31, 2022 •

edited

Loading