1366205 - Add test coverage for keyboard navigation in panelviews

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[Mike de Boer [:mikedeboer]]

In bug 1354144, we implemented (custom) keyboard navigation in panel views using the arrow and Enter keys.
Adding test coverage was split-off, because bugs are cheap and QE verification is nice to have there.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1366205 - add a browser mochitest with full coverage of the new menu panel keyboard navigation feature.

This also fixes two issues I found whilst writing the tests:
1. Exclude hidden items from the set of navigable buttons and
2. Exclude disabled items from the set of navigable buttons whilst navigating,
because they may get disabled in the meantime (like with the edit controls).

Review commit: https://reviewboard.mozilla.org/r/142464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142464/

Comment 2

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8870892 [details]
Bug 1366205 - add a browser mochitest with full coverage of the new menu panel keyboard navigation feature.

https://reviewboard.mozilla.org/r/142464/#review146396

Very nice. Bunch of minor comments, but I love that we're getting test coverage for this. Thanks!

::: browser/components/customizableui/PanelMultiView.jsm:810
(Diff revision 1)
> -            if (buttonIndex > maxIdx)
> +          do {
> +            buttonIndex = navMap.selected = (navMap.selected + (isDown ? 1 : -1));
> +          } while (buttons[buttonIndex] && buttons[buttonIndex].disabled)
> +          if (isDown && buttonIndex > maxIdx)
> -              buttonIndex = 0;
> +            buttonIndex = 0;
> -          } else {
> +          if (!isDown && buttonIndex < 0)

Nit: else if

::: browser/components/customizableui/test/browser.ini:152
(Diff revision 1)
>  [browser_1161838_inserted_new_default_buttons.js]
>  [browser_bootstrapped_custom_toolbar.js]
>  [browser_customizemode_contextmenu_menubuttonstate.js]
>  [browser_exit_background_customize_mode.js]
>  [browser_overflow_use_subviews.js]
> +[browser_panel_keynav.js]

Nit: panel_keyboard_navigation ? :-)

::: browser/components/customizableui/test/browser_panel_keynav.js:55
(Diff revision 1)
> +  // Navigate to the 'Help' button, which points to a subview.
> +  EventUtils.synthesizeKey("KEY_ArrowUp", { code: "ArrowUp" });
> +  Assert.equal(document.commandDispatcher.focusedElement, buttons[buttons.length - 1],
> +    "The last button should be focused after navigating upward");

Can you future-proof this for the to-be-added exit/quit button (and/or future reorgs) by sticking the synthesize thing into a loop that checks if it's got to the help button yet? Same thing in the next test, of course.

::: browser/components/customizableui/test/browser_panel_keynav.js:72
(Diff revision 1)
> +
> +  EventUtils.synthesizeKey("KEY_ArrowDown", { code: "ArrowDown" });
> +  Assert.equal(document.commandDispatcher.focusedElement, helpButtons[0],
> +    "Pressing downwards should select the first button");
> +
> +  // The first button is the back button. Hittin Enter should navigate us back.

Can we insert a loop here that checks we navigate through the subview buttons correctly, similar to the main view ones?

::: browser/components/customizableui/test/browser_panel_keynav.js:85
(Diff revision 1)
> +    EventUtils.synthesizeKey("KEY_ArrowUp", { code: "ArrowUp" });
> +    focusedElement = document.commandDispatcher.focusedElement;
> +    Assert.equal(focusedElement, buttons[buttons.length - (i + 1)],
> +      "The correct button should be focused after navigating upward");
> +  }
> +  Assert.equal(focusedElement.id, "appMenu-find-button", "Find button should be selected");

Again, I'd kind of like this to not be tied to the current structure of the menu with magic numbers...

::: browser/components/customizableui/test/head.js:325
(Diff revision 1)
>    return new Promise((resolve, reject) => {
>      let win = aSubview.ownerGlobal;
>      let timeoutId = win.setTimeout(() => {
>        reject("Subview (" + aSubview.id + ") did not show within 20 seconds.");
>      }, 20000);
>      function onViewShowing(e) {
> -      aSubview.removeEventListener("ViewShowing", onViewShowing);
> +      aSubview.removeEventListener(aEventName, onViewShowing);
>        win.clearTimeout(timeoutId);
>        resolve();
>      }
> -    aSubview.addEventListener("ViewShowing", onViewShowing);
> +    aSubview.addEventListener(aEventName, onViewShowing);

While here, can we sub this entire thing for:

return BrowserTestUtils.waitForEvent(aSubView, aEventName);

? Or does that break the world? These timeout rejections are known to break stuff. :-(

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8870892 [details]
Bug 1366205 - add a browser mochitest with full coverage of the new menu panel keyboard navigation feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142464/diff/1-2/

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8870892 [details]
Bug 1366205 - add a browser mochitest with full coverage of the new menu panel keyboard navigation feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142464/diff/2-3/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29ed157a7ca2
add a browser mochitest with full coverage of the new menu panel keyboard navigation feature. r=Gijs

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29ed157a7ca2