ComboboxControl: Add more unit tests #65255

zaguiini · 2024-09-12T00:38:36Z

What?

We're clearing the filter and input values whenever the textbox gets focused. This PR modifies this behavior and only clears the filter value on blur and if there's no option selected.

Why?

The previous behavior was not intuitive and brought unintended consequences (i.e., calling an endpoint with an empty string because the onFilterValueChange callback was fired, despite nothing having changed.)

Testing Instructions

Open the Storybook component and start typing in the box. Blur it and the filter value should be gone. In the unit tests, check that relevant onFilterValueChange behaves as expected (i.e., the tests passes.)

Screencast

Registrazione.schermo.2024-09-12.alle.11.43.15.AM.mov

zaguiini · 2024-09-12T00:42:52Z

packages/components/src/combobox-control/index.tsx

-		onFilterValueChange( '' );
-		setInputValue( '' );


@adamsilverstein @youknowriad, do we have any context why that was added? I'm struggling to understand it.

I think I understand it now. We're clearing the input value so all the suggestions show up. But that's a bit weird because there is already a selected value, so I can't see a good reason not to narrow the options to this value and the user can progressively see more as they delete characters from the input. Thoughts?

github-actions · 2024-09-12T00:43:06Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: zaguiini <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-09-12T01:02:08Z

Flaky tests detected in 0423a7e.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10821944363
📝 Reported issues:

[Flaky Test] should not render Block Settings sections #64387 in /test/e2e/specs/widgets/customizing-widgets.spec.js

mirka · 2024-09-12T11:15:24Z

Could you explain your use case? I'm assuming you are trying to use the combobox as a way to show suggested values, and not a way to choose from a list of allowed values. Per the APG:

In some implementations, the popup presents allowed values, while in other implementations, the popup presents suggested values.

As I assume from the component README ("ComboboxControl is an enhanced version of a SelectControl, with the addition of being able to search for options using a search input") and the implementation, ComboboxControl currently only covers the use case for allowed values. I think the current logic makes sense for this use case.

There will need to be bigger changes if we want the component to support the use case for suggested values, so as not to break back compat. I'm not immediately sure if we want to undertake this enhancement in this version of ComboboxControl, or if this should wait until we do a version 2 of it based on Ariakit. (cc @ciampo)

zaguiini · 2024-09-12T12:49:06Z

Hey @mirka, here's the use case. Whenever the Combobox query changes, we're fetching new data from the API. But focusing the input clears the query, dispatching an unexpected API request.

I was looking at the APG and, maybe I missed it, but I didn't see anything related to clearing the input value?

"ComboboxControl is an enhanced version of a SelectControl, with the addition of being able to search for options using a search input"

That's exactly how I'm using it, no?

There will need to be bigger changes if we want the component to support the use case for suggested values, so as not to break back compat.

I'm not proposing a different use case, I just want not to dispatch the filter change whenever the input gets focused 😄

mirka · 2024-09-12T13:18:29Z

Got it. So if the use case is for constraining to allowed values, the first weirdness I'm noticing with the current ComboboxControl implementation is that onFilterValueChange is not called on blur. Instead it waits for the next focus event, even though the visible filter value is already cleared.

CleanShot.2024-09-12.at.22.13.40.mp4

My premise here is that the visible input value should be cleared on blur if an allowed value is not yet selected, because otherwise it looks like an invalid value is selected. Does this match your expectation of how the component should behave?

tyxla · 2024-09-12T13:20:01Z

My premise here is that the visible input value should be cleared on blur if an allowed value is not yet selected, because otherwise it looks like an invalid value is selected.

I second this. Leaving random values on blur makes it look as if they're selected, while they actually aren't.

zaguiini · 2024-09-12T13:20:59Z

Exactly @mirka @tyxla,

~~This was added as part of a second PR, but happy to move these changes over if we want to!~~

EDIT: Sorry, I confused things 😅 I'm happy to call it onBlur if there was no value selected 👍🏻

zaguiini · 2024-09-12T14:32:09Z

@mirka @tyxla This should be good for another round of review 🙇🏻

mirka · 2024-09-12T18:01:34Z

packages/components/src/combobox-control/index.tsx

+	useEffect( () => {
+		if ( ! inputHasFocus ) {
+			setInputValue( '' );
+		}
+	}, [ inputHasFocus ] );


Any reason this is in an effect? Seems like we could reset the input value in the onBlur.

Yeah, I observed the suggestions flicker before hiding, so the effect prevents this from happening.

mirka · 2024-09-12T19:35:36Z

packages/components/src/combobox-control/index.tsx

+		if ( ! currentOption ) {
+			onFilterValueChange( '' );
+		}


If we restrict the onFilterValueChange reset to when there is no current option, this raises the problem of the visual input value and the consumer's filter value going out of sync.

The problem now dovetails with what you're suggesting in #65256. That behavior is not wrong or bad, and ideally our APIs would be flexible enough to support that behavior. But can we say that it is universally better or more correct over the current behavior on trunk? For example, a lot (most?) users who click on an already value-selected ComboboxControl are going to want to change that value to something else entirely. Having to delete the current input value manually is an added step.

So my impression at this point is that we should wait until the version 2 rewrite to Ariakit, so consumers can customize behavior in a much more modular way. Otherwise, with the WP ecosystem constraints we're working within, it's hard to justify shipping a potentially breaking behavioral change unless it can be clearly established as a bug fix.

Is there anything else we can do to help support your use case now, without major changes like in #65256?

I see what you're saying, and I agree entirely.

Is there anything else we can do to help support your use case now

My use case is I don't want to refetch the options whenever the Combobox Control gets focused. It should only refetch the options on textinput change, otherwise I want to use whatever we have. I'm using onFilterValueChange to detect changes to the input so that I can fetch the options from the API.

If there's another way to do it, that's fine by me. I thought of checking if the input is empty and then not fetching anything, but then there's no way to reset the options because there's no way to differentiate an organic backspace vs. the input cleared by our API. That is, unless we add a new flag to onFilterValueChange, which might not be too bad.

If it's just about reducing unnecessary API calls, can you handle it with a simple caching layer on your end? Cache the result of whatever API you call for an empty string, i.e. the unfiltered list of options.

Or is it not really about the API calls, and you actually want to show the previously filtered list of options even though the input field is visually empty?

Or is it not really about the API calls, and you actually want to show the previously filtered list of options even though the input field is visually empty?

It's this case. I'm not too concerned about the API calls themselves, but I want to preserve the list of options.

And that would be an acceptable UX even without the changes in #65256? I'd think you need a way to signal that there is a filter value, and provide a way to clear it.

I'm saying this because adding more arguments to onFilterValueChange is a non-breaking change we could potentially make, but #65256 is not.

Yeah, that sounds good. I'll explore that option.

I'm starting to think about that flag more.

Let's start by noting that the inputValue (internal) will be cleared every time the input is blurred.

If the input gets blurred and no option is selected, clicking the input again will render an empty textbox. Therefore, it makes no sense to keep the previous options because the input was closed, and no option was selected.

If the input gets blurred and there is a selected option, clicking the input again will still render an empty textbox. But if we introduce the flag, only the subset of filtered options will be presented on focus (because the callback was not called/called with a flag to ignore the change in the filter), despite the textbox itself displaying no previous filter.

So, I'm starting to think these changes don't make sense. Initially, I felt that the Combobox should respect the existing value of the filter whenever opening the suggestion list, much like a SelectControl, but I no longer think that's the case.

If there are no objections, I'll close this PR. That means we'll miss the tests, but I don't think it's a big deal because they were asserting use cases that don't make sense 😄

Agreed. I'm afraid we'll have to wait for ComboboxControlV2 to safely customize this kind of behavior.

That means we'll miss the tests, but I don't think it's a big deal because they were asserting use cases that don't make sense 😄

FWIW I think we can keep the "calls onFilterValueChange whenever the textbox changes" and "clears the textbox value if there is no selected value on blur" tests, with minor changes so it asserts the actual behavior on trunk.

mirka · 2024-09-12T19:37:47Z

packages/components/src/combobox-control/test/index.tsx

I really appreciate the added tests here. I think we'll want to merge them in some form, regardless of the outcome of this PR.

…ring" This reverts commit 43158d2.

This reverts commit 2a62872.

This reverts commit 0423a7e.

mirka

As discussed, I salvaged some of the tests 👍

Combobox Control: Do not clear values when focusing the textbox

0423a7e

zaguiini added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Sep 12, 2024

zaguiini requested a review from adamsilverstein September 12, 2024 00:38

zaguiini self-assigned this Sep 12, 2024

zaguiini requested a review from ajitbohra as a code owner September 12, 2024 00:38

zaguiini commented Sep 12, 2024

View reviewed changes

zaguiini mentioned this pull request Sep 12, 2024

Combobox Control: Sync props.value with internal inputValue #65256

Closed

zaguiini requested a review from youknowriad September 12, 2024 01:20

youknowriad requested review from mirka, ciampo and DaniGuardiola September 12, 2024 10:32

mirka requested a review from a team September 12, 2024 10:50

Call onFilterValueChange on blur if no value is selected

2a62872

zaguiini force-pushed the combobox/do-not-call-query-change-on-focus branch from 4455012 to 2a62872 Compare September 12, 2024 14:36

Clear inputValue on the background to avoid suggestion flickering

43158d2

zaguiini changed the title ~~Combobox Control: Do not clear values when focusing the textbox~~ Combobox Control: Only clear filter value on blur Sep 12, 2024

zaguiini changed the title ~~Combobox Control: Only clear filter value on blur~~ Combobox Control: Clear filter value on blur if no option has been selected Sep 12, 2024

zaguiini changed the title ~~Combobox Control: Clear filter value on blur if no option has been selected~~ Combobox Control: Only clear filter value on blur if no option has been selected Sep 12, 2024

mirka reviewed Sep 12, 2024

View reviewed changes

mirka added 2 commits September 18, 2024 21:38

Revert "Clear inputValue on the background to avoid suggestion flicke…

18f821f

…ring" This reverts commit 43158d2.

Revert "Call onFilterValueChange on blur if no value is selected"

acc634f

This reverts commit 2a62872.

mirka added 2 commits September 18, 2024 21:38

Revert "Combobox Control: Do not clear values when focusing the textbox"

3c5e001

This reverts commit 0423a7e.

Tweak tests

2aef386

mirka self-assigned this Sep 18, 2024

mirka changed the title ~~Combobox Control: Only clear filter value on blur if no option has been selected~~ ComboboxControl: Add more unit tests Sep 18, 2024

mirka approved these changes Sep 18, 2024

View reviewed changes

mirka enabled auto-merge (squash) September 18, 2024 13:39

mirka merged commit b6ba559 into trunk Sep 18, 2024
61 checks passed

mirka deleted the combobox/do-not-call-query-change-on-focus branch September 18, 2024 13:51

github-actions bot added this to the Gutenberg 19.3 milestone Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ComboboxControl: Add more unit tests #65255

ComboboxControl: Add more unit tests #65255

zaguiini commented Sep 12, 2024 •

edited

Loading

zaguiini Sep 12, 2024

zaguiini Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024

mirka commented Sep 12, 2024

zaguiini commented Sep 12, 2024

mirka commented Sep 12, 2024

tyxla commented Sep 12, 2024

zaguiini commented Sep 12, 2024 •

edited

Loading

zaguiini commented Sep 12, 2024 •

edited

Loading

mirka Sep 12, 2024

zaguiini Sep 13, 2024

mirka Sep 12, 2024

zaguiini Sep 13, 2024

mirka Sep 13, 2024

zaguiini Sep 13, 2024

mirka Sep 16, 2024

zaguiini Sep 16, 2024

zaguiini Sep 16, 2024 •

edited

Loading

mirka Sep 18, 2024

mirka Sep 12, 2024

mirka left a comment

ComboboxControl: Add more unit tests #65255

ComboboxControl: Add more unit tests #65255

Conversation

zaguiini commented Sep 12, 2024 • edited Loading

What?

Why?

Testing Instructions

Screencast

Choose a reason for hiding this comment

zaguiini Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 12, 2024 • edited Loading

github-actions bot commented Sep 12, 2024

mirka commented Sep 12, 2024

zaguiini commented Sep 12, 2024

mirka commented Sep 12, 2024

tyxla commented Sep 12, 2024

zaguiini commented Sep 12, 2024 • edited Loading

zaguiini commented Sep 12, 2024 • edited Loading

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

zaguiini Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

zaguiini commented Sep 12, 2024 •

edited

Loading

zaguiini Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

zaguiini commented Sep 12, 2024 •

edited

Loading

zaguiini commented Sep 12, 2024 •

edited

Loading

zaguiini Sep 16, 2024 •

edited

Loading