Page MenuHomePhabricator

[Timebox] CdxLookup with an initial value has unexpected behavior
Closed, ResolvedPublic2 Estimated Story Points

Description

If I'm using CdxLookup in a form to edit existing data, the v-model:selected and :initial-input-value properties may be set when the form loads, but there won't be a matching menu item unless it is specifically created or retrieved.

However, there being no menu-items causes unexpected behavior when the user blanks the input. Because the following if-clause:

Lookup.vue
		function onUpdateInput( newVal: string|number ) {
			// If there is a selection and it doesn't match the new value, clear it.
			if (
				selectedMenuItem.value &&
				selectedMenuItem.value.label !== newVal &&
				selectedMenuItem.value.value !== newVal
			) {
				modelWrapper.value = null;
			}
			// [...]

wont execute (no menu items), hence the @update:selected event won't trigger with null, and thus failing to signal that the selection has been removed.

This was part of the cause for T370326.


Acceptance criteria

  • The selection is cleared when the input changes and no longer matches the selection, whether or not there is a matching menu item for the selection

Event Timeline

CCiufo-WMF triaged this task as Medium priority.Jul 19 2024, 3:40 PM
CCiufo-WMF moved this task from Inbox to Up Next on the Design-System-Team board.
CCiufo-WMF renamed this task from CdxLookup with an initial value has unexpected behavior to [Timebox] CdxLookup with an initial value has unexpected behavior.Jul 22 2024, 5:22 PM
CCiufo-WMF set the point value for this task to 3.

The code snippet in the task description is part of the problem, but there's another important part to it, which is how selectedMenuItem is defined:

Lookup.vue
const selectedMenuItem = computed( () =>
	props.menuItems.find( ( item ) => item.value === props.selected )
);

In Michael's scenario, v-model:selected is set to something that doesn't match any of the menu items. This means selectedMenuItem.value is undefined. The onUpdateInput code quoted in the task description misinterprets this to mean there is no selection, which is what causes this bug.

Suggested new behavior: clearing the input should always clear the selection. More precisely, if the input is cleared and there was a selection, the selection should be set to null, even if the previous selection didn't match an existing menu item.

lwatson subscribed.

The code snippet in the task description is part of the problem, but there's another important part to it, which is how selectedMenuItem is defined:

Lookup.vue
const selectedMenuItem = computed( () =>
	props.menuItems.find( ( item ) => item.value === props.selected )
);

In Michael's scenario, v-model:selected is set to something that doesn't match any of the menu items. This means selectedMenuItem.value is undefined. The onUpdateInput code quoted in the task description misinterprets this to mean there is no selection, which is what causes this bug.

Suggested new behavior: clearing the input should always clear the selection. More precisely, if the input is cleared and there was a selection, the selection should be set to null, even if the previous selection didn't match an existing menu item.

I think this is probably the right move, but I want to call attention to some current ambiguity in this component – we might want to address some other things as well as part of this fix. I want to highlight three props in this component that have a somewhat unclear relationship IMO:

  • The Lookup component can optionally accept an array of menu-items as part of its initial state. These items will be presented as "suggestions" and are available as soon as the user clicks on the input (prior to typing or API requests, etc).
  • The Lookup component's model value (v-model:selected), which represents the selection, must be provided – null can be used to represent the lack of a selection.
  • Finally, there is an optional initialInputValue class – this is for the text input but not the actual selection state. It defaults to an empty string even if the model value is initially provided.

Question 1: should we allow Lookup to have BOTH an initial set of menuItems (pre-fetched suggestions) AND an initial selection state when the latter is not included in the former? I understand that there might be cases where there is a pre-populated selection and an empty set of initial menu items (when we expect to fetch them all on the server, etc). But is it a valid use-case to have a non-empty list of menu items and a non-null initial selection at the same time? Or should we look at adding some validation logic to enforce "proper" usage here?

Question 2: Should the initialInputValue prop derive its default value from the v-model:selected value that is initially passed to the component, if that value is not null? It seems strange to me that the input will still appear empty if a v-model:selected value is provided ; you have to provide the component the same information twice if you want the user to actually see something in the text field before the menu expands. This is less relevant to the bug in question, but seems like it is also worth addressing if we're going in anyway.

Anyway, once I'm clear on the intended behavior here, I can capture it in some new tests/demos and also update prop defaults/validation logic if necessary. This would be in addition to changing onUpdateInput so that a cleared string always sets the selection back to null.

Hey @Michael, can you leave some specific repro instructions on the task (or point me to a live example where things don't work properly)? I want to make sure I don't miss anything as I work on a fix.

Change #1068070 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] Lookup: always clear the selection when the input changes

https://gerrit.wikimedia.org/r/1068070

Change #1068070 merged by jenkins-bot:

[design/codex@main] Lookup: always clear the selection when the input changes

https://gerrit.wikimedia.org/r/1068070

Change #1070656 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v1.11.1 to v1.12.0

https://gerrit.wikimedia.org/r/1070656

Test wiki created on Patch demo by EGardner (WMF) using patch(es) linked to this task:
http://patchdemo.wmcloud.org/wikis/6bc5837bc4/w/

Change #1070656 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v1.11.1 to v1.12.0

https://gerrit.wikimedia.org/r/1070656