fw/applib/ui: Open plain menu rows on a single tap

A tap on a not-selected row of a plain (non-center-focused) menu used
to only select it, so opening an item after a scroll took two taps.
Make plain menus behave like a phone list instead: a single tap selects
the row (selection_will_change honoured) and activates it in the same
gesture. A veto still changes nothing, and a redirect only selects the
redirected row - opening a row the finger never touched would be a
misfire. The commit uses MenuRowAlignNone so the content does not shift
as the activated window pushes.

Center-focused carousels keep the two-step model (tap centres, tap on
the centred row activates); the double-tap window machinery is now
carousel-only, since plain menus activate on the first tap.

Short-item action menus keep the two-step tap via the new internal
menu_layer_set_tap_select_only(): their rows hold several columns and
the row-granular tap hit-test cannot tell which one the finger meant.

Fixes FIRM-4102

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Joshua Jun <lets@throw.rocks>

Author: jplexer

src/fw/applib/ui/action_menu_layer.c

@@ -746,6 +746,10 @@ static void prv_update_aml_cache(ActionMenuLayer *aml, int selected_index) {
   const bool center_focused = !prv_aml_is_short(aml);
   menu_layer_set_center_focused(&aml->menu_layer, center_focused);
 #endif
+  // Short-item rows hold several columns, and the row-granular tap hit-test cannot tell which
+  // column the finger meant — keep the two-step tap (select, then activate) for those instead of
+  // the plain menus' tap-to-activate.
+  menu_layer_set_tap_select_only(&aml->menu_layer, prv_aml_is_short(aml));
 
   layer_mark_dirty(&aml->layer);
   menu_layer_reload_data(&aml->menu_layer);

src/fw/applib/ui/menu_layer.c

@@ -1495,6 +1495,13 @@ void menu_layer_set_center_focused(MenuLayer *menu_layer, bool center_focused) {
   menu_layer_update_caches(menu_layer);
 }
 
+void menu_layer_set_tap_select_only(MenuLayer *menu_layer, bool tap_select_only) {
+  if (!menu_layer) {
+    return;
+  }
+  menu_layer->tap_select_only = tap_select_only;
+}
+
 bool menu_layer_get_scroll_wrap_around(MenuLayer *menu_layer) {
   return menu_layer->scroll_wrap_around;
 }
@@ -1550,13 +1557,19 @@ void menu_layer_set_scroll_vibe_on_blocked(MenuLayer *menu_layer, bool scroll_vi
 // public per-menu gesture surface (and the unit-test entry points).
 //
 // Live scrolling happens on pan Updated. On a plain (non-center-focused) menu the selection is
-// frozen for the whole pan and never changes on liftoff — a pan scrolls, a tap selects. A
-// center-focused menu is a carousel instead: the focus stays pinned at the viewport centre, so the
-// row crossing the centre becomes the selection live during the pan (through the full
-// selection_will_change contract), and liftoff settles the selected row to the exact centre.
-
-// Two independent taps within this window on the same menu count as a "double tap" and activate the
-// last tap-selected row (there is no dedicated double-tap recognizer). CALIBRATION: ~300ms is the
+// frozen for the whole pan and never changes on liftoff — a pan only scrolls. A center-focused
+// menu is a carousel instead: the focus stays pinned at the viewport centre, so the row crossing
+// the centre becomes the selection live during the pan (through the full selection_will_change
+// contract), and liftoff settles the selected row to the exact centre.
+//
+// Taps differ by shape. A plain menu behaves like a phone list: a single tap on any row both
+// selects it (selection_will_change honoured) and activates it — one tap opens the item. A
+// carousel keeps the two-step model (a tap on an off-centre row centres it, a tap on the centred
+// row activates), bridged by the double-tap window below.
+
+// Two independent taps within this window on the same carousel count as a "double tap" and activate
+// the last tap-selected row (there is no dedicated double-tap recognizer). Plain menus activate on
+// the first tap, so the window only ever arms on center-focused menus. CALIBRATION: ~300ms is the
 // usual comfortable double-tap spacing; tune on hardware if it feels too eager/sluggish.
 #define DOUBLE_TAP_WINDOW_MS 300
 
@@ -1904,31 +1917,47 @@ void menu_layer_touch_handle_tap(MenuLayer *menu_layer, GPoint point_on_screen)
     return;
   }
 
-  // Priority 3 — select the tapped row through the full will_change contract (veto/redirect honoured)
-  // and centre it, WITHOUT activating. A veto (final == old selection) must change nothing: it must
-  // not re-centre the old selection (that mini-snap is exactly what Fix 1 removed from pans). Record
-  // the double-tap state only when a selection actually changes (normal or redirect).
+  // Priority 3 — the tapped row is not the selection: run the full will_change contract
+  // (veto/redirect honoured). A veto (final == old selection) must change nothing: no select, no
+  // activate, no re-centre of the old selection (that mini-snap is exactly what Fix 1 removed from
+  // pans), no animation cancel, and the double-tap window stays as it was — cancelling would abort
+  // an in-flight highlight animation and violate "veto changes nothing", so the cancel lives past
+  // the veto check.
   const MenuIndex old_index = menu_layer->selection.index;
   const MenuIndex final = prv_menu_run_will_change(menu_layer, candidate);
   if (menu_index_compare(&final, &old_index) == 0) {
-    // Vetoed: no select, no activate, no re-centre, no animation cancel, and the window stays as it
-    // was. Cancelling here would abort an in-flight highlight animation and violate "veto changes
-    // nothing", so the cancel lives in the select branch below.
     return;
   }
   prv_cancel_selection_animation(menu_layer);
-  // Animated: a center-focused menu plays the same jump/bounce as a button step (the nudge-in,
-  // half-way snap and bounce-out of prv_schedule_center_focus_animation); plain menus slide as
-  // before.
+
+  if (!menu_layer->center_focused) {
+    // Plain menus behave like a phone list: the tap selects AND opens the row in one gesture.
+    // Commit without scrolling — the row is already visible under the finger, and a re-centre
+    // would visibly shift the content just as the activated window pushes. Activate only when the
+    // contract kept the tapped row: a redirect selects the redirected row without opening it
+    // (opening a row the finger never touched would be a misfire), so there activation stays a
+    // deliberate second tap (priority 2). tap_select_only menus (short-item action menus, whose
+    // rows hold several columns the row-granular hit-test cannot tell apart) keep that two-step
+    // model for every row.
+    menu_layer_set_selected_index(menu_layer, final, MenuRowAlignNone, false);
+    if (!menu_layer->tap_select_only &&
+        menu_index_compare(&menu_layer->selection.index, &candidate) == 0) {
+      prv_menu_activate_selected(menu_layer);
+    }
+    return;
+  }
+
+  // Carousel: select the tapped row and centre it WITHOUT activating — the same jump/bounce as a
+  // button step (the nudge-in, half-way snap and bounce-out of
+  // prv_schedule_center_focus_animation); opening is the second tap (or the double-tap window).
   menu_layer_set_selected_index(menu_layer, final, MenuRowAlignCenter, true);
   // Record the committed (range-clamped) selection, not the raw will_change output: a client that
   // redirects to an out-of-range index would otherwise be handed that OOB index by a fast double tap
-  // (priority 1), diverging from priority 2 which activates the clamped selection.index. On a
-  // center-focused menu the index commit is deferred to the half-way point of the jump animation;
-  // until then the committed target lives in animation.new_selection (a priority-1 tap during the
-  // jump cancels it, which drives the animation to its end state and commits that same target).
-  const bool jump_in_flight = menu_layer->center_focused &&
-                              animation_is_scheduled(menu_layer->animation.animation);
+  // (priority 1), diverging from priority 2 which activates the clamped selection.index. The index
+  // commit is deferred to the half-way point of the jump animation; until then the committed
+  // target lives in animation.new_selection (a priority-1 tap during the jump cancels it, which
+  // drives the animation to its end state and commits that same target).
+  const bool jump_in_flight = animation_is_scheduled(menu_layer->animation.animation);
   menu_layer->last_selected_index = jump_in_flight ? menu_layer->animation.new_selection.index
                                                    : menu_layer->selection.index;
   prv_menu_set_last_select_ticks(menu_layer, now);

src/fw/applib/ui/menu_layer.h

@@ -413,10 +413,11 @@ typedef struct MenuLayer {
   } touch_nav_node;
 
   //! @internal
-  //! Tier-1 touch tap state for the "tap selects, second/double tap activates" behaviour: the last
-  //! row a tap-select committed and when (\ref rtc_get_ticks). A second tap within the double-tap
-  //! window activates that row without re-hit-testing (the row may have animated to the centre by
-  //! then). Declared unconditionally (not under \c CONFIG_TOUCH) so \c sizeof(MenuLayer) is identical
+  //! Tier-1 touch tap state for the center-focused "tap selects, second/double tap activates"
+  //! behaviour: the last row a tap-select committed and when (\ref rtc_get_ticks). A second tap
+  //! within the double-tap window activates that row without re-hit-testing (the row may have
+  //! animated to the centre by then). Plain menus activate on the first tap and never arm this
+  //! window. Declared unconditionally (not under \c CONFIG_TOUCH) so \c sizeof(MenuLayer) is identical
   //! on every board, and carved from \ref padding below like \ref touch_nav_node. The timestamp is
   //! split into two 32-bit halves rather than a single 8-byte \ref RtcTicks on purpose: a naturally
   //! 8-byte-aligned member would raise the struct's alignment to 8 and its size past the fixed
@@ -474,6 +475,14 @@ typedef struct MenuLayer {
   //! byte.
   bool touch_tap_swallow:1;
 
+  //! @internal
+  //! True when a tap on a not-selected row of a plain (non-center-focused) menu must only select
+  //! it, keeping activation a deliberate second tap, instead of the default select-and-activate.
+  //! For menus whose rows hold several items (the action menu's short-item columns), where the
+  //! row-granular tap hit-test cannot tell which item the finger meant. See
+  //! \ref menu_layer_set_tap_select_only. Packs into \ref touch_fling_active's byte.
+  bool tap_select_only:1;
+
   //! Add some padding to keep track of the \ref MenuLayer size budget.
   //! As long as the size stays within this budget, 2.x apps can safely use the 3.x MenuLayer type.
   //! The actual size check is generated from applib_malloc.json, not asserted here.
@@ -685,6 +694,15 @@ bool menu_layer_get_center_focused(MenuLayer *menu_layer);
 //! @see \ref menu_layer_get_center_focused
 void menu_layer_set_center_focused(MenuLayer *menu_layer, bool center_focused);
 
+//! @internal
+//! On a plain (non-center-focused) menu a touch tap on a not-selected row selects it and activates
+//! it in one gesture. Setting this makes such a tap only select, keeping activation a deliberate
+//! second tap — for menus whose rows hold several items (the action menu's short-item columns),
+//! where the row-granular tap hit-test cannot tell which item the finger meant.
+//! @param menu_layer The menu layer for which to enable or disable the behavior.
+//! @param tap_select_only true = a tap only selects, false (default) = a tap selects and activates.
+void menu_layer_set_tap_select_only(MenuLayer *menu_layer, bool tap_select_only);
+
 //! True, if the \ref MenuLayer can wrap around the first and last element.
 //! @see \ref menu_layer_set_scroll_wrap_around
 bool menu_layer_get_scroll_wrap_around(MenuLayer *menu_layer);

tests/fw/ui/test_action_menu_layer.c

@@ -236,18 +236,15 @@ void test_action_menu_layer__tap_on_selected_item_activates(void) {
   action_menu_layer_deinit(&s_aml);
 }
 
-// A tap on a different action selects it (no activation); a later tap on it activates it.
-void test_action_menu_layer__tap_other_item_selects_then_activates(void) {
+// A wide-item row holds exactly one action, so a tap on a different action selects it and
+// activates it in the same gesture (plain menus open on a single tap).
+void test_action_menu_layer__tap_other_item_selects_and_activates(void) {
   prv_init_aml_with_wide_items();
 
   menu_layer_touch_handle_tap(&s_aml.menu_layer, prv_tap_point_for_row(&s_aml.menu_layer, 2));
-  cl_assert_equal_i(s_select_count, 0);
   cl_assert_equal_i(s_aml.selected_index, 2);
   cl_assert_equal_i(s_selection_changed_count, 1);
   cl_assert(s_last_changed_item == &s_wide_items[2]);
-
-  prv_advance_past_double_tap_window();
-  menu_layer_touch_handle_tap(&s_aml.menu_layer, prv_tap_point_for_row(&s_aml.menu_layer, 2));
   cl_assert_equal_i(s_select_count, 1);
   cl_assert(s_last_selected_item == &s_wide_items[2]);