Closed
Bug 1366205
Opened 7 years ago
Closed 7 years ago
Add test coverage for keyboard navigation in panelviews
Categories
(Firefox :: Toolbars and Customization, enhancement, P1)
Firefox
Toolbars and Customization
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
(Blocks 1 open bug)
Details
(Keywords: access, Whiteboard: [photon-structure])
Attachments
(1 file)
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•7 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) |
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29ed157a7ca2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in
before you can comment on or make changes to this bug.
Description
•