Add test coverage for keyboard navigation in panelviews

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks 1 bug, {access})

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-structure])

Attachments

(1 attachment)

Assignee

Description

2 years ago
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.
Flags: qe-verify-
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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. :-(
Attachment #8870892 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by mdeboer@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29ed157a7ca2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Keywords: access
Depends on: 1425972
No longer depends on: 1425972
You need to log in before you can comment on or make changes to this bug.