Skip to content

Conversation

@stokesman
Copy link
Contributor

@stokesman stokesman commented Sep 8, 2020

The impetus is to fix #24460 which is a regression. The solution in this PR is simple and includes a test to prevent regressions but did require minor changes to quite a few tests. Also, though tangential to the issue, there is the removal of some extraneous code.

The core change is to InputControl, revising logic for value state updates and propagation. It no longer updates the internal value state from props while the input has focus. The change ensures that entry will be unimpeded by the validation logic of consuming components (such as RangeControl).

A new test is added for RangeControl specifying the allowance of invalid values in the input element until the loss of focus. It was confirmed to fail before the rest of the changes were added and so guards against future regressions.

Ancillary changes are the removal of RangeControl's wrapping of NumberControl and test updates to work with the new InputControl implementation detail.

How has this been tested?

  • Storybook
  • WordPress 5.5.
  • Unit tests

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [n/a] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [n/a] I've included developer documentation if appropriate.
  • [n/a?] I've updated all React Native files affected by any refactorings/renamings in this PR.

@stokesman stokesman force-pushed the update/input-and-range-controls branch from 3ea7cca to 4fe5a1f Compare September 8, 2020 15:19
@stokesman stokesman changed the title Update/input and range controls Update: input and range controls Sep 8, 2020
@stokesman stokesman marked this pull request as ready for review September 8, 2020 15:44
@stokesman stokesman marked this pull request as draft September 9, 2020 23:02
@stokesman stokesman marked this pull request as ready for review September 15, 2020 16:49
@stokesman stokesman force-pushed the update/input-and-range-controls branch from 4fe5a1f to a18fea4 Compare September 15, 2020 17:23
@stokesman stokesman force-pushed the update/input-and-range-controls branch from a18fea4 to f41e4ae Compare September 22, 2020 20:08
@jasmussen
Copy link
Contributor

This tests well for me:

rangecontrol

So in essence, all this needs is a code review. I'll kindly ask Dan if he can help. I've restarted the tests just in case the failure was random. Nice work!

@jasmussen jasmussen requested a review from talldan September 23, 2020 06:37
@talldan talldan added [Package] Components /packages/components [Type] Regression Related to a regression in the latest release First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels Sep 24, 2020
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

The change looks good to me, very thorough! In testing it solves the issue and I couldn't spot any unwanted side-effects.

Applying the curly braces mentioned in the comment before merging would be great. (actually I went ahead and did this, as it was such a small thing).

Also pinging @ItsJonQ to make you aware of the change, but I don't see any reason not to push forwards with this right now.

@talldan talldan merged commit a1df992 into WordPress:master Sep 24, 2020
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 24, 2020
@talldan talldan changed the title Update: input and range controls Fix difficult to use RangeControl when minimum value specified Sep 24, 2020
@stokesman
Copy link
Contributor Author

Oh, dang! Here I've been composing a long reply with some issues I've found and I was too slow 🐢 .

Looks like this PR should implement some affordance toward backward compatibility with the old behavior of RangeControl. Specifically not calling onChange when the value is out-of-range. There's also a issue with AnglePickerControl that surfaces because of the new behavior of InputControl not accepting external changes while focused. While I think it may be considered a bug with AnglePickerControl applying this PR means it needs a fix.

To detail the issues in blocks due to lack of backward compatibility they present in Image, Latest Comments and Spacer. To be clear these are not introduced by this PR just not fixed by it.

For Image, the issue can be reproduced by clearing the input in the range control used to adjust zoom while cropping. It will crash the block 🙀

For Latest Comments, the issue can be reproduced by entering an out-of-range value in the range control input for "Number of comments". This one is not so bad, the block warns: "Error loading block: Invalid parameter(s): attributes" but it is easily recovered.

For Spacer, there are three ways it can misbehave. The first can be reproduced by clearing the input in the range control used to adjust the height. If you don't have your console open you could miss it. React warns that: "Received NaN for the max attribute..." and life goes on. The second can be reproduced by saving the post while that input is cleared as in the previous example. Upon reloading the editor, the block will be invalid. The third can be reproduced by using the in-canvas resizable feature of the block while having cleared the input as in the previous examples. The resizing there works but the range control fails to update 😕

As for the issue with AnglePickerControl, it uses InputControl (by way of NumberControl). The problem presents when you focus the input element then manipulate the rotating knob. Focus never leaves the input and that makes it ingore the updates from outside. I think that input remaining focused is likely not right anyway but this PR will make a change there necessary.

talldan added a commit that referenced this pull request Sep 24, 2020
talldan added a commit that referenced this pull request Sep 24, 2020
@talldan
Copy link
Contributor

talldan commented Sep 24, 2020

@stokesman No worries. I've reverted the commit in #25601, lets work to fix those other issues and then introduce this change.

@stokesman
Copy link
Contributor Author

Thanks @talldan, this should handle it. I'm calling it a day and haven't tested it thoroughly but I'll do so tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Regression Related to a regression in the latest release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spacer Block: Difficult to enter values directly into input field

3 participants