From 6e85fa8fcc701aefd83cfd2f1883a2f72c586386 Mon Sep 17 00:00:00 2001
From: evykassirer <evy@zulip.com>
Date: Thu, 26 Sep 2024 18:17:08 -0700
Subject: [PATCH] input_pill: Highlight/focus pill before removing it.

Select the pill on the first backspace and delete the whole pill on
the second backspace. If the pill is already highlighted from
left-pressing, then backspace would delete it right away.

We're making this change because it can be quite annoying to
re-type out a pill that's accidentally deleted, and users might
think pills are editable and accidentally delete the whole thing
with a backspace stroke.
---
 web/src/input_pill.ts        | 16 ++++++++++------
 web/tests/input_pill.test.js | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/web/src/input_pill.ts b/web/src/input_pill.ts
index 7ae041484b..683287ba8a 100644
--- a/web/src/input_pill.ts
+++ b/web/src/input_pill.ts
@@ -214,8 +214,7 @@ export function create<ItemType extends {type: string}>(
             return undefined;
         },
 
-        // this will remove the last pill in the container -- by default tied
-        // to the "Backspace" key when the value of the input is empty.
+        // This will remove the last pill in the container.
         // If quiet is a truthy value, the event handler associated with the
         // pill will not be evaluated. This is useful when using clear to reset
         // the pills.
@@ -317,16 +316,21 @@ export function create<ItemType extends {type: string}>(
             const selection = window.getSelection();
             // If no text is selected, and the cursor is just to the
             // right of the last pill (with or without text in the
-            // input), then backspace deletes the last pill.
+            // input), then backspace highlights or deletes the last pill.
             if (
                 e.key === "Backspace" &&
                 (funcs.value(this).length === 0 ||
                     (selection?.anchorOffset === 0 && selection?.toString()?.length === 0))
             ) {
                 e.preventDefault();
-                funcs.removeLastPill("backspace");
-
-                return;
+                const pill = store.pills.at(-1);
+                // We focus the pill first first, as a signal that the pill
+                // is about to be deleted. The deletion will then happen through
+                // `removePill` from the event handler on the pill.
+                if (pill) {
+                    assert(!pill.$element.is(":focus"));
+                    pill.$element.trigger("focus");
+                }
             }
 
             // if one is on the ".input" element and back/left arrows, then it
diff --git a/web/tests/input_pill.test.js b/web/tests/input_pill.test.js
index 8152c8ad2e..8dbf5b9396 100644
--- a/web/tests/input_pill.test.js
+++ b/web/tests/input_pill.test.js
@@ -405,9 +405,18 @@ run_test("insert_remove", ({mock_template}) => {
         };
     }
 
+    let color_focused;
+    function handle_event(color) {
+        return (event) => {
+            assert.equal(event, "focus");
+            color_focused = color;
+        };
+    }
+
     const pills = widget._get_pills_for_testing();
     for (const pill of pills) {
         pill.$element.remove = set_colored_removed_func(pill.item.color_name);
+        pill.$element.trigger = handle_event(pill.item.color_name);
     }
 
     let key_handler = $container.get_on_handler("keydown", ".input");
@@ -422,13 +431,13 @@ run_test("insert_remove", ({mock_template}) => {
         },
     );
 
-    assert.ok(removed);
-    assert.equal(color_removed, "YELLOW");
-
-    assert.deepEqual(widget.items(), [items.blue, items.red]);
+    // The first backspace focuses the pill, the second removes it.
+    assert.ok(!removed);
+    assert.equal(color_focused, "YELLOW");
+    const yellow_pill = pills.find((pill) => pill.item.color_name === "YELLOW");
+    $container.set_find_results(".pill:focus", yellow_pill.$element);
 
     let next_pill_focused = false;
-
     const $next_pill_stub = {
         trigger(type) {
             if (type === "focus") {
@@ -436,6 +445,22 @@ run_test("insert_remove", ({mock_template}) => {
             }
         },
     };
+    yellow_pill.$element.next = () => $next_pill_stub;
+
+    key_handler = $container.get_on_handler("keydown", ".pill");
+    key_handler.call(
+        {},
+        {
+            key: "Backspace",
+            preventDefault: noop,
+        },
+    );
+    assert.ok(removed);
+    assert.equal(color_removed, "YELLOW");
+    assert.ok(next_pill_focused);
+
+    next_pill_focused = false;
+    assert.deepEqual(widget.items(), [items.blue, items.red]);
 
     const $focus_pill_stub = {
         next: () => $next_pill_stub,