Prevent Combobox dropdown list dismissal during text changed event

[avazirna]

Product Description

This addresses an issue introduced by this PR. QA reported that when users manually delete the selected item in the ComboboxWidget, the dropdown list is dismissed, preventing users from seeing the filtered items (see video below). This occurs because widgetEntryChanged() calls getAnswer(), and within the CommboboxWidget, this method dismisses the dropdown list.

Technical Summary

The solution here was to prevent the dismissal when the widgetEntryChanged() is called during an afterTextChanged() event.

Safety Assurance

Safety story

Successfully tested locally.

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR modifies ComboboxWidget.java to track whether an onTextChanged event is currently firing by introducing a private boolean flag. When afterTextChanged is invoked, the flag is set to true, widgetEntryChanged() is called, and the flag is reset to false. The getAnswer() method is updated to prevent dropdown dismissal when this flag indicates an active onTextChanged event, preserving the previous dismissal behavior at other times.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• ComboboxWidget.java: Verify the timing logic of the flag state around afterTextChanged and getAnswer() to ensure dropdown dismissal is correctly deferred and doesn't introduce unintended UI behavior during text input
• Test coverage: Confirm whether existing tests cover the modified dropdown dismissal logic under various text-change scenarios

Possibly related PRs

• Backmerge Commcare 2.61.1 #3447: Modifies ComboboxWidget.java's afterTextChanged behavior to invoke widgetEntryChanged(), directly related to the callback pattern used in this PR
• Emit widget changed event on manual ComboboxWidget item deletion #3445: Modifies ComboboxWidget.java's TextWatcher behavior to replace clearWarningMessage() with widgetEntryChanged(), shares similar widget callback patterns

Suggested reviewers

• shubham1g5
• Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers product context, technical solution, and local testing; however, it lacks critical details on automated test coverage and a comprehensive QA plan.	Add specific automated test cases that verify the dropdown remains visible during text changes and add detailed QA plan with test scenarios and ticket links.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: preventing dropdown dismissal during text change events in ComboboxWidget.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch prevent-dropdown-dismiss-during-text-change

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

app/src/org/commcare/views/widgets/ComboboxWidget.java (2)

32-32: Flag introduction is fine; consider a slightly clearer name (optional).

The added boolean cleanly scopes the behavior change and is localized to this widget. If you touch this again, consider a slightly shorter/more precise name (e.g. isDuringTextChangeEvent) to reduce line length and better reflect that it’s toggled in afterTextChanged, not onTextChanged specifically.

---

97-102: Guard flag reset with try/finally to avoid stale state on exception.

If widgetEntryChanged() ever throws, isEntryChangedEventFiredDuringOnTextChanged stays true and subsequent getAnswer() calls will never dismiss the dropdown. Wrapping the call in try/finally makes this more robust:

@Override
public void afterTextChanged(Editable s) {
-    isEntryChangedEventFiredDuringOnTextChanged = true;
-    widgetEntryChanged();
-    isEntryChangedEventFiredDuringOnTextChanged = false;
+    boolean previous = isEntryChangedEventFiredDuringOnTextChanged;
+    isEntryChangedEventFiredDuringOnTextChanged = true;
+    try {
+        widgetEntryChanged();
+    } finally {
+        isEntryChangedEventFiredDuringOnTextChanged = previous;
+    }
}

This also handles any future nested text-change flows correctly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c687dae and 3de6103.

📒 Files selected for processing (1)

• app/src/org/commcare/views/widgets/ComboboxWidget.java (3 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Lint Code Base

🔇 Additional comments (1)

app/src/org/commcare/views/widgets/ComboboxWidget.java (1)

121-123: Conditional dropdown dismissal matches the described UX fix.

Gating dismissDropDown() on !isEntryChangedEventFiredDuringOnTextChanged cleanly solves the reported issue: text-driven widgetEntryChanged() calls won’t close the dropdown, while focus changes, item clicks, and other getAnswer() call sites keep the old behavior. This looks consistent with the PR objective.

[conroy-ricketts]

Nit: Can we shorten this variable name? Maybe processingWidgetEntryTextChange or processingTextChangeEvent

[avazirna]

Yes, we can.

[avazirna]

Superseded by #3452