Avoid accidentally closing (sub-)menu too early (#1734)

Resolves #1725. This PR introduces an on-close delay for (sub-)menus, which prevents the menu from closing too early in case the user moves out their mouse cursor from the respective menu item. I’ve described some subtle problems and corner-cases below. Let me know how the current timing parameters feel for you, so that we can tweak them if need be. ## Now https://github.com/tiny-pilot/tinypilot/assets/83721279/6a30adc2-b75d-4f82-a2ca-d078b5378a21 ## Before https://github.com/tiny-pilot/tinypilot/assets/83721279/98e2cf31-c25e-4aa6-bb96-06a866617be2 ([Photo of TinyPilot user, captured throughout this screen recording](https://github.com/tiny-pilot/tinypilot/assets/83721279/29c769ef-bb8e-47af-ab43-2732d091d880)) # Notes - I don’t think we need `-webkit` and friends anymore for compatibility. It seems that the `transition` directive should [be supported for long enough by now](https://caniuse.com/?search=transition). - I moved the comment about the “System” menu down one line, just above the respective `<li>` element. I think it should have been there in the first place. ## Delaying the transition I slightly delayed the fade-out in the initial part of the transition, otherwise the menu would start to disappear immediately. This would feel a bit weird, because if you move the mouse cursor back quickly, the menu seems to fade in again out of nowhere. The initial delaying alleviates this effect. The video below demonstrates the feeling with the default, linear transition, i.e., without a [`cubic-bezier` easing function](https://developer.mozilla.org/en-US/docs/Web/CSS/easing-function). (Alternatively, we could also use a `linear` easing function, but that still has poorer browser support right now, since it’s newer.) By the way, see [this tool for a visualisation what `cubic-bezier(0.5, 0, 1, 0.25)` looks like mathematically](https://cubic-bezier.com/#.5,0,1,.25). https://github.com/tiny-pilot/tinypilot/assets/83721279/6b419939-e462-4d3d-a1af-17347f04f3f9 ## Transition duration I tried to find a good compromise between keeping the menu open long enough so that it prevents the accidental closing scenario, yet still quick enough so that it doesn’t feel sluggish and sticky. The video below demonstrates the feeling when the transition duration is too long. https://github.com/tiny-pilot/tinypilot/assets/83721279/ce2e7b81-b88e-4b5f-806f-156ed15b7d9f ## Flicker prevention Chrome and Firefox seem to prevent transitions on page load automatically. Safari (and maybe other browser too) don’t do this, however, so you would see the menus briefly flickering when loading the page. Therefore, we have to workaround that with a CSS/JS hack. I was only able to test on MacOS with Chrome, Firefox and Safari. Unfortunately, there is always a slight risk with these kind of hacks that browsers do weird things. This hack technique doesn’t seem uncommon at least. The video below demonstrates how it looks without such a workaround (in Safari). https://github.com/tiny-pilot/tinypilot/assets/83721279/22535fd5-d5f0-4db0-b444-3052b3ad9fe0 <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1734"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
tiny-pilot · Feb 1, 2024 · 34161c3 · 34161c3
1 parent 6f6bc42
commit 34161c3
Showing 1 changed file with 51 additions and 4 deletions.
diff --git a/app/templates/custom-elements/menu-bar.html b/app/templates/custom-elements/menu-bar.html
@@ -27,8 +27,30 @@
     li:hover > ul {
       visibility: visible;
       opacity: 1;
-      -webkit-transition: 0.3s;
-      transition: 0.3s;
+      transition: 300ms;
+    }
+
+    li > ul {
+      /* For the fade-out duration, we need to find a good compromise between
+         keeping the menu open long enough so that it doesn’t immediately
+         disappear when the user leaves the menu area for a split second
+         (e.g., if they accidentally “overshoot” with their cursor.) But it
+         also shouldn’t be so long that the menu feels sluggish or sticky. */
+      transition: 300ms;
+      /* For the fade-out timing, slow down the transition in the initial
+         moment when the cursor leaves the menu area, and only make the
+         fade-out kick in after that initial “threshold”. Otherwise, if the
+         user would accidentally leave the menu area for a split second, the
+         menu would _immediately_ start to disappear, which is distracting. */
+      transition-timing-function: cubic-bezier(0.5, 0, 1, 0.25);
+    }
+
+    .prevent-flickering-on-page-load li > ul {
+      /* Some browsers (e.g., Safari) apply transitions on page load as well,
+         in which case the menu would briefly flicker when opening the page.
+         Therefore, we suppress transitions here, and remove this CSS class via
+         JS in the connectCallback after the DOM has finished loading. */
+      transition: none !important;
     }
 
     :host {
@@ -178,8 +200,12 @@
 
   <div class="logo"><img src="/img/logo.svg" alt="TinyPilot logo" /></div>
 
-  <!-- The “System” menu is used for managing the TinyPilot device itself. -->
-  <ul class="groups header-item" role="menubar">
+  <ul
+    class="groups header-item prevent-flickering-on-page-load"
+    id="menu"
+    role="menubar"
+  >
+    <!-- The “System” menu is used for managing the TinyPilot device itself. -->
     <li class="group" role="presentation">
       <a role="menuitem">System</a>
       <ul class="items" role="group">
@@ -339,6 +365,13 @@
             template.content.cloneNode(true)
           );
 
+          this._undoInitialFlickerPrevention =
+            this._undoInitialFlickerPrevention.bind(this);
+          document.addEventListener(
+            "DOMContentLoaded",
+            this._undoInitialFlickerPrevention
+          );
+
           // Handle “simple”/standard menu items that just emit a custom event
           // without further ado.
           this.shadowRoot
@@ -386,6 +419,13 @@
           }
         }
 
+        disconnectedCallback() {
+          document.removeEventListener(
+            "DOMContentLoaded",
+            this._undoInitialFlickerPrevention
+          );
+        }
+
         set isKeyboardVisible(isVisible) {
           const menuItem = this.shadowRoot.getElementById("keyboard-menu-item");
           menuItem.classList.toggle("nav-selected", isVisible);
@@ -418,6 +458,13 @@
             );
           }
         }
+
+        _undoInitialFlickerPrevention() {
+          // (See CSS comment in .prevent-flickering-on-page-load class.)
+          this.shadowRoot
+            .getElementById("menu")
+            .classList.remove("prevent-flickering-on-page-load");
+        }
       }
     );
   })();