1950194 - Investigate why Esc keypresses don't propagate to callouts with noautohide

Status: NEW

(Firefox :: Messaging System, task, P1)

Description

[Shane Hughes [:aminomancer]]

This is related to bug 1950189. In my investigations there, I found that Esc keypress events can dismiss callouts even if they're not focused. We long ago noticed that feature callouts weren't behaving like other panels in this respect, and were not receiving these Esc events, but we didn't ascertain why. Back then, there was no autohide feature; noautohide was permanently enabled (i.e. autohide was permanently disabled). After testing the autohide feature, we found that the Esc events do reach the callout, but not if noautohide is enabled.

That is somewhat fortunate, because currently our main reason for caring about this is for callouts with no interactive elements. Without any interactive elements, they don't get focused, so it becomes more important to be able to dismiss them with Esc without needing to focus them. And they'd also need autohide, since otherwise they could only be dismissed by Esc key.

Callouts with interactive elements are generally auto-focused, which makes this less pressing. But we've been asked to make auto-focus configurable, since some PMs don't want it (bug 1892417). We also anticipate there will be situations where users move focus out of the callout (intentionally or not), and then want to dismiss the callout and hit Esc, but that does nothing because it no longer has focus. So closing the callout becomes a papercut that requires either mousing or tabbing back into the callout and then hitting Esc. Not critical, but a nuisance.

So we'd like to figure out why the propagation of these Esc key events seems to depend on the noautohide property. I've looked through the references to noautohide but haven't found anything yet. I'd like to at least understand it, so we can gauge the feasibility of closing this gap.

Comment 1

[Shane Hughes [:aminomancer]]

Emilio, does this behavior sound familiar? It might be intended behavior, but it does strike me as a little odd, since there's a separate ignorekeys attribute.

In case it helps, here's a test case:

1. Patch D239402 (without it, Esc is ignored without focus regardless of noautohide)
2. Set the pref browser.newtabpage.activity-stream.asrouter.devtoolsEnabled to true
3. Go to about:asrouter
4. Search for TEST_FEATURE_TOUR
5. In the big textbox, replace the content with this:

   {
     "weight": 100,
     "id": "TEST_FEATURE_TOUR",
     "template": "feature_callout",
     "groups": [],
     "content": {
       "id": "TEST_FEATURE_TOUR",
       "template": "multistage",
       "backdrop": "transparent",
       "transitions": false,
       "disableHistoryUpdates": true,
       "screens": [
         {
           "id": "FEATURE_CALLOUT_1",
           "anchors": [
             {
               "selector": "#PanelUI-menu-button",
               "panel_position": {
                 "anchor_attachment": "bottomcenter",
                 "callout_attachment": "topright"
               }
             }
           ],
           "content": {
             "position": "callout",
             "autohide": true,
             "title": {
               "raw": "Panel Feature Callout"
             },
             "subtitle": {
               "raw": "Hello!"
             }
           }
         }
       ]
     },
     "targeting": "providerCohorts.panel_local_testing == \"SHOW_TEST\"",
     "provider": "panel_local_testing"
   }
   

6. Hit the blue Modify button - the panel should show on the app menu button. The Esc key should be able to close it, even though it's not focused.
7. Now in the textbox, scroll down a little to find the "autohide": true line, and set it to false. Hit Modify again. This time, the Esc key will not be able to close it unless you click inside the callout.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

IgnoreKeys and co generally only seem to work on <menupopup>, not on <panel> (and that seems somewhat intentional by reading the code).

That said Esc is handled everywhere. Esc for panels should be handled here. Can you check if that's reached with the popup you care about? It might be that something has prevented the propagation of the event earlier or so... The event listener is a regular capturing event listener on the relevant document.

Comment 3

[Shane Hughes [:aminomancer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

IgnoreKeys and co generally only seem to work on <menupopup>, not on <panel> (and that seems somewhat intentional by reading the code).

That said Esc is handled everywhere. Esc for panels should be handled here. Can you check if that's reached with the popup you care about? It might be that something has prevented the propagation of the event earlier or so... The event listener is a regular capturing event listener on the relevant document.

Thanks, that seems reasonable. Here's what I did:

--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -19,6 +19,7 @@
 #include "nsCSSFrameConstructor.h"
 #include "nsGlobalWindowOuter.h"
 #include "nsIContentInlines.h"
+#include "nsIConsoleService.h"
 #include "nsLayoutUtils.h"
 #include "nsViewManager.h"
 #include "nsITimer.h"
@@ -1258,6 +1259,15 @@ void nsXULPopupManager::HidePopup(Element* aPopup, HidePopupOptions aOptions,

   if (foundPopup) {
     if (foundPopup->IsNoAutoHide()) {
+      nsCOMPtr<nsIConsoleService> console(
+          do_GetService(NS_CONSOLESERVICE_CONTRACTID));
+      if (console) {
+        nsAtom* idAtom = aPopup->GetID();
+        nsAutoCString id;
+        idAtom->ToUTF8String(id);
+        console->LogStringMessage(
+            nsString(u"Hiding popup "_ns + NS_ConvertUTF8toUTF16(id)).get());
+      }
       // If this is a noautohide panel, remove it but don't close any other
       // panels.
       popupToHide = aPopup;
@@ -2539,9 +2549,30 @@ bool nsXULPopupManager::HandleKeyboardEventWithKeyCode(
   uint32_t keyCode = aKeyEvent->KeyCode();

   // Escape should close panels, but the other keys should have no effect.
+  nsCOMPtr<nsIConsoleService> console(
+      do_GetService(NS_CONSOLESERVICE_CONTRACTID));
+  if (console) {
+    RefPtr<dom::Element> popup = aTopVisibleMenuItem->Element();
+    nsAtom* idAtom = popup->GetID();
+    nsAutoCString id;
+    idAtom->ToUTF8String(id);
+    console->LogStringMessage(
+        nsString(u"Handling keyboard event for popup "_ns +
+                 NS_ConvertUTF8toUTF16(id))
+            .get());
+  }
   if (aTopVisibleMenuItem &&
       aTopVisibleMenuItem->GetPopupType() != PopupType::Menu) {
     if (keyCode == KeyboardEvent_Binding::DOM_VK_ESCAPE) {
+      if (console) {
+        RefPtr<dom::Element> popup = aTopVisibleMenuItem->Element();
+        nsAtom* idAtom = popup->GetID();
+        nsAutoCString id;
+        idAtom->ToUTF8String(id);
+        console->LogStringMessage(
+            nsString(u"Maybe hiding popup "_ns + NS_ConvertUTF8toUTF16(id))
+                .get());
+      }
       HidePopup(aTopVisibleMenuItem->Element(), {HidePopupOption::IsRollup});
       aKeyEvent->StopPropagation();
       aKeyEvent->StopCrossProcessForwarding();

So I created 3 console messages. Message 1 is when the keyboard event is handled. Message 2 is when Esc specifically is handled and the popup passes the aTopVisibleMenuItem && aTopVisibleMenuItem->GetPopupType() != PopupType::Menu check. Message 3 is when nsXULPopupManager::HidePopup is invoked and the popup passes the foundPopup->IsNoAutoHide() check.

When noautohide is disabled, I see Message 1 and Message 2, but not Message 3, and the panel is indeed closed. The event never reaches my custom handler in this case.

When noautohide is enabled, I see none of the 3 messages, and the panel is not closed. The event does reach my handler, but because it's not focused, the panel is not dismissed. Of course, we could eliminate that focus check. But we intend to get rid of this custom handler altogether, and bring the callout panel more in line with the rest of the browser. We've heard from Jamie Teh that panel autofocus behavior is going to be disabled by default, so we basically want to remove all the custom handling and let the panel be dismissed on Esc by whatever logic will do that for other, more normal panels. Probably wise to not reinvent the wheel anyway, if there's already much lower-level logic for when to handle e.g. an Esc keypress that bubbles up from within a tabbed browser.

So, I'm not sure why noautohide would prevent all 3 messages from being reached. It is checked before Message 3, but I don't see any noautohide checks upstream of Messages 1 and 2. Here's the relevant markup of the panel, once it's shown:

<panel
  class="panel-no-padding"
  orient="vertical"
  noautofocus="true"
  flip="slide"
  type="arrow"
  consumeoutsideclicks="never"
  norolluponanchor="true"
  position="bottomcenter topright"
  show-arrow=""
  noautohide="true"
  id="feature-callout"
  aria-describedby="#feature-callout .welcome-text"
  side="top"
  hasbeenopened="true"
  animate="open"
  arrow-position="top-end"
  panelopen="true"
/>

Oh, and just to be clear, if I click inside the noautohide panel and then hit Escape, it shows Message 3, but not Messages 1 and 2. This seems to be because that custom handler I mentioned previously is calling hidePopup(), confirmed by breakpoints. So it seems like the XULPopupManager event listener is not picking up the keypress.

Edit: Removing consumeoutsideclicks="never" from the panel did not resolve the issue.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ok, so I found it, and it is intentional. In here, we only get no-autohide tooltips (here).

So the question is is there a way of doing what you want? And the answer I'm not super sure about. I guess one option would be to tweak nsXULPopupManager::KeyDown to also handle noautohide panels. That might be fine I think...

Totally unrelated, but aria-describedby is not a CSS selector. You need something else there.

Comment 5

[Shane Hughes [:aminomancer]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Ok, so I found it, and it is intentional. In here, we only get no-autohide tooltips (here).

So the question is is there a way of doing what you want? And the answer I'm not super sure about. I guess one option would be to tweak nsXULPopupManager::KeyDown to also handle noautohide panels. That might be fine I think...

Ahh... Well, let me give that a try. Thanks for looking into this!

Totally unrelated, but aria-describedby is not a CSS selector. You need something else there.

Wow, I think you might have incidentally solved our longstanding screen reader issue lmao. Thank you.

---

@emcminn, it sounds like we might have a fix for our screen reader woes in bug 1950862. From the MDN article, it seems like we might need to give .welcome-text an id like multi-stage-message-text or something. aria-live="assertive" might also be necessary.

We might also investigate what happens if we remove these ARIA attributes. Screen readers might read all the relevant content automatically if we stop instructing them to read specific elements (which we're currently doing with invalid ids). I guess the problem is that the content we want read is not necessarily always in .welcome-text. It may sometimes be a LinkParagraph or some kind of tile, which is outside of .welcome-text. If screen readers don't automatically read that stuff without focus, then we might need some way to dynamically set aria-describedby. It could be a message JSON property I guess. So this is something we should play around with, now that we know we're definitely doing it incorrectly.

I'll also add this context to bug 1950862.