Polish and fix regressions in the new menu components.
Review Request #10936 — Created March 3, 2020 and submitted — Latest diff uploaded
The new
rb-c-menu
/RB.MenuView
component had a few design decisions
made early on that, upon further reflection, weren't really necessary,
and impacted other parts of the UI.It used
display: block/none
to control visibility, for a couple
reasons that made sense earlier in development, but weren't ultimately
needed. Sincedisplay
cannot be animated, and affected animations, it
was up toRB.MenuView
to more closely manage the opening/closing
states, adding some JS-specific intermediary classes to turndisplay
on/off at the right times. This actually broke the legacy.menu
class,
which broke opening review request action menus.Addressing this was actually pretty easy, and allowed for the deletion
of a lot of code. We're now once again usingvisibility
andopacity
to fully manage the display. The JS intermediary state classes are now
gone, and all thesetTimeout
calls inRB.MenuView
are gone as well.
This also allowedRB.MenuView.open()
andclose()
to consolidate
fully into_setOpened()
.It also fixes an issue with the key bindings. Previously,
RB.MenuView
captured all keyboard events, with the idea being that while you're
interacting with a menu, you shouldn't be triggering other elements
accidentally. That ended up not working as well in practice. This
prevented things like jumping to another part of the UI via a keyboard
shortcut (posing an accessibility issue), or even reloading the page.
It no longer swallows all events, only preventing default behavior and
bubbling if the event is being specifically handled by the menu.
Unit tests pass.
Tested both mouse-based and keyboard-based navigation and usage of the
menu. Verified that keyboard-based navigation didn't animate the menu
(ensuring that capability hasn't broken).Tested that the review request action menus worked.