Bug 1545766: PanelMultiView: Don't override keyboard navigation in embedded documents. r=Gijs
☠☠ backed out by 47ea779dc66b ☠ ☠
authorJames Teh <jteh@mozilla.com>
Wed, 01 May 2019 04:01:35 +0000
changeset 530864 fbc294a6fe785b56a5ef52b5b1ea61d511bebc52
parent 530863 9a0ce3016f03a914367e86c8339a8b885e309c83
child 530865 041741ce164674137968c39aab0a9c80e84c99db
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1545766
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1545766: PanelMultiView: Don't override keyboard navigation in embedded documents. r=Gijs Extension panels contain embedded documents; i.e. a <browser> element. We want users to be able to tab to these and we want them to be focused automatically if a subview is opened from the keyboard, so treat them as tabbable. However, once an embedded document is focused, we can't manage keyboard navigation inside it, so don't try. Previously, we tried, which meant keys were overridden even though they didn't do anything, breaking keyboard navigation in extensions altogether. Differential Revision: https://phabricator.services.mozilla.com/D28442
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -1406,17 +1406,19 @@ var PanelView = class extends Associated
 
   /**
    * Determine whether an element can only be navigated to with tab/shift+tab,
    * not the arrow keys.
    */
   _isNavigableWithTabOnly(element) {
     let tag = element.localName;
     return tag == "menulist" || tag == "textbox" || tag == "input"
-           || tag == "textarea";
+           || tag == "textarea"
+           // Allow tab to reach embedded documents in extension panels.
+           || tag == "browser";
   }
 
   /**
    * Make a TreeWalker for keyboard navigation.
    *
    * @param {Boolean} arrowKey If `true`, elements only navigable with tab are
    *        excluded.
    */
@@ -1560,43 +1562,48 @@ var PanelView = class extends Associated
    *
    * @param {KeyEvent} event
    */
   keyNavigation(event) {
     if (!this.active) {
       return;
     }
 
+    let focus = this.document.activeElement;
+    // Make sure the focus is actually inside the panel. (It might not be if
+    // the panel was opened with the mouse.) If it isn't, we don't care
+    // about it for our purposes.
+    // We use Node.compareDocumentPosition because Node.contains doesn't
+    // behave as expected for anonymous content; e.g. the input inside a
+    // textbox.
+    if (focus && !(this.node.compareDocumentPosition(focus)
+                   & Node.DOCUMENT_POSITION_CONTAINED_BY)) {
+      focus = null;
+    }
+
+    // Extension panels contain embedded documents. We can't manage
+    // keyboard navigation within those.
+    if (focus && focus.tagName == "browser") {
+      return;
+    }
+
     let stop = () => {
       event.stopPropagation();
       event.preventDefault();
     };
 
     // If the focused element is only navigable with tab, it wants the arrow
     // keys, etc. We shouldn't handle any keys except tab and shift+tab.
     // We make a function for this for performance reasons: we only want to
     // check this for keys we potentially care about, not *all* keys.
     let tabOnly = () => {
       // We use the real focus rather than this.selectedElement because focus
       // might have been moved without keyboard navigation (e.g. mouse click)
       // and this.selectedElement is only updated for keyboard navigation.
-      let focus = this.document.activeElement;
-      if (!focus) {
-        return false;
-      }
-      // Make sure the focus is actually inside the panel.
-      // (It might not be if the panel was opened with the mouse.)
-      // We use Node.compareDocumentPosition because Node.contains doesn't
-      // behave as expected for anonymous content; e.g. the input inside a
-      // textbox.
-      if (!(this.node.compareDocumentPosition(focus)
-            & Node.DOCUMENT_POSITION_CONTAINED_BY)) {
-        return false;
-      }
-      return this._isNavigableWithTabOnly(focus);
+      return focus && this._isNavigableWithTabOnly(focus);
     };
 
     let keyCode = event.code;
     switch (keyCode) {
       case "ArrowDown":
       case "ArrowUp":
         if (tabOnly()) {
           break;
--- a/browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
+++ b/browser/components/customizableui/test/browser_PanelMultiView_keyboard.js
@@ -18,32 +18,35 @@ let gMainMenulist;
 let gMainTextbox;
 let gMainButton2;
 let gMainButton3;
 let gMainTabOrder;
 let gMainArrowOrder;
 let gSubView;
 let gSubButton;
 let gSubTextarea;
+let gDocView;
+let gDocBrowser;
 
 async function openPopup() {
   let shown = BrowserTestUtils.waitForEvent(gMainView, "ViewShown");
   PanelMultiView.openPopup(gPanel, gAnchor, "bottomcenter topright");
   await shown;
 }
 
 async function hidePopup() {
   let hidden = BrowserTestUtils.waitForEvent(gPanel, "popuphidden");
   PanelMultiView.hidePopup(gPanel);
   await hidden;
 }
 
-async function showSubView() {
-  let shown = BrowserTestUtils.waitForEvent(gSubView, "ViewShown");
-  gPanelMultiView.showSubView(gSubView);
+async function showSubView(view = gSubView) {
+  let shown = BrowserTestUtils.waitForEvent(view, "ViewShown");
+  // We must show with an anchor so the Back button is generated.
+  gPanelMultiView.showSubView(view, gMainButton1);
   await shown;
 }
 
 async function expectFocusAfterKey(aKey, aFocus) {
   let res = aKey.match(/^(Shift\+)?(.+)$/);
   let shift = Boolean(res[1]);
   let key;
   if (res[2].length == 1) {
@@ -69,16 +72,18 @@ add_task(async function setup() {
   gPanel.appendChild(gPanelMultiView);
 
   gMainView = document.createXULElement("panelview");
   gMainView.id = "testMainView";
   gPanelMultiView.appendChild(gMainView);
   gMainButton1 = document.createXULElement("button");
   gMainButton1.id = "gMainButton1";
   gMainView.appendChild(gMainButton1);
+  // We use this for anchoring subviews, so it must have a label.
+  gMainButton1.setAttribute("label", "gMainButton1");
   gMainMenulist = document.createXULElement("menulist");
   gMainMenulist.id = "gMainMenulist";
   gMainView.appendChild(gMainMenulist);
   let menuPopup = document.createXULElement("menupopup");
   gMainMenulist.appendChild(menuPopup);
   let item = document.createXULElement("menuitem");
   item.setAttribute("value", "1");
   item.setAttribute("selected", "true");
@@ -106,16 +111,28 @@ add_task(async function setup() {
   gSubButton = document.createXULElement("button");
   gSubView.appendChild(gSubButton);
   gSubTextarea = document.createElementNS("http://www.w3.org/1999/xhtml",
                                           "textarea");
   gSubTextarea.id = "gSubTextarea";
   gSubView.appendChild(gSubTextarea);
   gSubTextarea.value = "value";
 
+  gDocView = document.createXULElement("panelview");
+  gDocView.id = "testDocView";
+  gPanelMultiView.appendChild(gDocView);
+  gDocBrowser = document.createXULElement("browser");
+  gDocBrowser.id = "gDocBrowser";
+  gDocBrowser.setAttribute("type", "content");
+  gDocBrowser.setAttribute("src",
+    'data:text/html,<textarea id="docTextarea">value</textarea><button id="docButton"></button>');
+  gDocBrowser.setAttribute("width", 100);
+  gDocBrowser.setAttribute("height", 100);
+  gDocView.appendChild(gDocBrowser);
+
   registerCleanupFunction(() => {
     gAnchor.remove();
     gPanel.remove();
   });
 });
 
 // Test that the tab key focuses all expected controls.
 add_task(async function testTab() {
@@ -265,8 +282,39 @@ add_task(async function testActivation()
   }
   await openPopup();
   await expectFocusAfterKey("ArrowDown", gMainButton1);
   checkActivated(gMainButton1, () => EventUtils.synthesizeKey("KEY_Enter"), "pressing enter");
   checkActivated(gMainButton1, () => EventUtils.synthesizeKey(" "), "pressing space");
   checkActivated(gMainButton1, () => EventUtils.synthesizeKey("KEY_Enter", {code: "NumpadEnter"}), "pressing numpad enter");
   await hidePopup();
 });
+
+// Test that tab and the arrow keys aren't overridden in embedded documents.
+add_task(async function testTabArrowsBrowser() {
+  await openPopup();
+  await showSubView(gDocView);
+  let backButton = gDocView.querySelector(".subviewbutton-back");
+  backButton.id = "docBack";
+  await expectFocusAfterKey("Tab", backButton);
+  let doc = gDocBrowser.contentDocument;
+  // Documents don't have an id property, but expectFocusAfterKey wants one.
+  doc.id = "doc";
+  await expectFocusAfterKey("Tab", doc);
+  // Make sure tab/arrows aren't overridden within the embedded document.
+  let textarea = doc.getElementById("docTextarea");
+  // Tab should really focus the textarea, but default tab handling seems to
+  // skip everything inside the browser element when run in this test. This
+  // behaves as expected in real panels, though. Force focus to the textarea
+  // and then test from there.
+  textarea.focus();
+  is(doc.activeElement, textarea, "textarea focused");
+  EventUtils.synthesizeKey("KEY_End");
+  is(textarea.selectionStart, 5, "selectionStart 5 after End");
+  EventUtils.synthesizeKey("KEY_ArrowLeft");
+  is(textarea.selectionStart, 4, "selectionStart 4 after ArrowLeft");
+  is(doc.activeElement, textarea, "textarea still focused");
+  let docButton = doc.getElementById("docButton");
+  expectFocusAfterKey("Tab", docButton);
+  // Make sure tab leaves the document and reaches the Back button.
+  expectFocusAfterKey("Tab", backButton);
+  await hidePopup();
+});