Fix touch events for menu actions on mobile.

Review Request #14163 — Created Sept. 12, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

Menu actions, such as the "Close" and "Update" review request actions,
were broken on mobile. A touch on an action item within the menu would
just close the menu instead of activating the action. This happened
because the touch handler for menu actions always just opened/closed the
menu regardless of whether touch was on an item or not.

This change fixes this, and updates a similar touch handler for the
unified banner. Previously, the unified banner touch handler would check
whether the touch was on a ink-c-menu__item-label. This was always
guaranteed to correctly determine whether the touch was on a menu item
because the label encompassed the entire element of the menu item.
However this could change in the future, and then we'd have a bug where
touches on menu items only work properly when the touch is exactly on
the label.

This change also alphabetically sorts a set of keys in the menu action
view JS file.

  • Tested opening/closing the "Close" and "Update" menus, and tapping
    on actions in them.
  • Tested opening/closing the unified banner's "Review" menu, and tapping
    on actions in it.
Summary ID
Fix touch events for menu actions on mobile.
Menu actions, such as the "Close" and "Update" review request actions, were broken on mobile. A touch on an action item within the menu would just close the menu instead of activating the action. This happened because the touch handler for menu actions always just opened/closed the menu regardless of whether touch was on an item or not. This change fixes this, and updates a similar touch handler for the unified banner. Previously, the unified banner touch handler would check whether the touch was on a `ink-c-menu__item-label`. This was always guaranteed to correctly determine whether the touch was on a menu item because the label encompassed the entire element of the menu item. However this could change in the future, and then we'd have a bug where touches on menu items only work properly when the touch was exactly on the label.
f698bc473181c4fd0a56420f2e1c686e42d186ff
Description From Last Updated

Hmm, this is cute, but I worry that it's not super robust. For example, a custom menu item might add …

daviddavid
david
  1. 
      
  2. Show all issues

    Hmm, this is cute, but I worry that it's not super robust. For example, a custom menu item might add links or other elements that don't have a classname that starts with this.

    Can we instead do something like this?

    const $target = $(e.target);
    
    if (!($target.hasClass('ink-c-menu__item') ||
          $target.parents('.ink-c-menu__item')) {
        ...
    }
    $(e.target).parents('.ink-c-menu__item')
    
    1. Ah good call. Do you think I'd need to worry about nested menus? Like if menu A has menu B as one of its items, and we wanna tap on menu B to close it... it wouldn't close because it has a .ink-c-menu__item ancestor right?

    2. IIRC we decided with Ink that we didn't want to do nested menus, and it doesn't have support for such. That said, I think for this particular case, this solution would still be correct--taps on the parent of nested items within the menu shouldn't be causing the entire menu to close either.

    3. Sounds good, I'll go with this.

  3. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (15d470a)