[UEPR-445] Topbar unit and integration tests#438
[UEPR-445] Topbar unit and integration tests#438kbangelov wants to merge 10 commits intoscratchfoundation:release/UEPR-297-accessibility-improvementsfrom
Conversation
kbangelov
changed the base branch from
develop
to
release/accessibility-improvements
kbangelov
force-pushed
the
release/accessibility-improvements
branch
2 times, most recently
from
e37597f to
f2902ce
Compare
kbangelov
changed the base branch from
release/accessibility-improvements
to
release/UEPR-297-accessibility-improvements
kbangelov
force-pushed
the
task/uepr-445-topbar-tests
branch
from
98bf7f1 to
10cec89
Compare
kbangelov
force-pushed
the
task/uepr-445-topbar-tests
branch
from
10cec89 to
814f74d
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for keyboard navigation in the Scratch GUI menu bar, supporting the accessibility initiative (UEPR-445). It introduces 13 unit tests for a menu navigation hook and 17 integration tests covering File, Edit, and Settings menus.
Changes:
- Added unit tests for the
useMenuNavigationhook covering keyboard interactions, RTL support, and focus management - Added integration tests for menu bar keyboard navigation including submenu interactions and RTL language switching
- Added @reduxjs/toolkit dependency for test infrastructure
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx | Unit tests for the menu navigation hook covering keyboard shortcuts, focus management, and RTL behavior |
| packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js | Integration tests for keyboard navigation across File, Edit, and Settings menus including submenu interactions |
| packages/scratch-gui/package.json | Adds @reduxjs/toolkit dependency for test store configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx
Outdated
Show resolved
Hide resolved
| store = configureStore({ | ||
| reducer: { | ||
| locales: (state = {isRtl: false}) => state | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test file uses configureStore from @reduxjs/toolkit to create a Redux store, which is inconsistent with the rest of the codebase. All other unit tests in the project use redux-mock-store (e.g., test/unit/components/menu-bar.test.jsx, test/unit/components/error-boundary-hoc.test.jsx). Consider using redux-mock-store for consistency with established testing patterns.
There was a problem hiding this comment.
It's a a deprecated method and this new one is the official recommendation. Yes, it's inconsistent and it seems to work the other way too, but I don't think it's a big deal. Perhaps I could change the others too?
packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js
Outdated
Show resolved
Hide resolved
kbangelov
force-pushed
the
task/uepr-445-topbar-tests
branch
2 times, most recently
from
1b2b384 to
a7b81d7
Compare
kbangelov
force-pushed
the
task/uepr-445-topbar-tests
branch
from
a7b81d7 to
b0af01a
Compare
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await driver.executeScript('document.activeElement.blur()'); |
There was a problem hiding this comment.
Why do we need to blur after all tests have completed?
| test('Tab focuses file menu on 3 clicks', async () => { | ||
| // Pressing tab 3 times should focus the file menu button | ||
| await clickKeys([Key.TAB, Key.TAB, Key.TAB]); | ||
|
|
||
| const activeElement = await driver.switchTo().activeElement(); | ||
| expect(await activeElement.getAttribute('aria-label')).toBe('File menu'); | ||
| }); |
There was a problem hiding this comment.
Hm, this is a bit specific. I wonder if it would make more sense to check the "order of navigation", instead of the File Menu in particular. E.g. first we focus the logo, then Settings, then File menu, etc.? Or something like "Pressing Tab navigates through the menu items"?
| const clickKey = async key => { | ||
| await driver.actions().sendKeys(key) | ||
| .perform(); | ||
| await driver.sleep(SLEEP_TIME); | ||
| }; | ||
|
|
||
| const clickKeys = async keys => { | ||
| for (const key of keys) { | ||
| await clickKey(key); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Maybe we could add these to the SeleniumHelper? If we want to write other tests for keyboard navigation, it'd be helpful to reuse them. It'd also be nice to add some error handling in case something goes wrong.
| const clickKey = async key => { | ||
| await driver.actions().sendKeys(key) | ||
| .perform(); | ||
| await driver.sleep(SLEEP_TIME); |
There was a problem hiding this comment.
Do we need to sleep after each key press? Or is there a particular issue you were avoiding that way? I wonder if we could avoid "sleeping" unless we definitely need to, to not increase the tests run time too much?
| // Explicit keyboard focus | ||
| await driver.executeScript('arguments[0].focus()', fileMenuButton); | ||
| await clickKey(Key.ENTER); |
There was a problem hiding this comment.
I wonder if we could achieve the same by using fileMenuButton.sendKeys(Key.ENTER), instead of manually focusing and then sending the key to the active element. Although for more complex test cases we'd still need to switch to the currently active element, and use sendKeys on it (e.g. for ArrowUp/Down navigation).
| test('Space opens the Language submenu', async () => { | ||
| const settingsMenuButton = await findByXpath(SETTINGS_MENU_XPATH); | ||
| await driver.executeScript('arguments[0].focus()', settingsMenuButton); | ||
| await clickKeys([Key.ENTER, Key.SPACE]); |
There was a problem hiding this comment.
Maybe we should test pressing Space twice? Because we've already tested the Enter interaction?
| test('Tab closes File menu', async () => { | ||
| const fileMenuButton = await findByXpath(FILE_MENU_XPATH); | ||
| await driver.executeScript('arguments[0].focus()', fileMenuButton); | ||
| await clickKeys([Key.ENTER, Key.TAB]); | ||
|
|
||
| const activeElement = await driver.switchTo().activeElement(); | ||
| expect(await activeElement.getAttribute('aria-label')).toBe('Edit menu'); | ||
|
|
||
| await loadUri(uri); | ||
|
|
||
| const fileMenuButton2 = await findByXpath(FILE_MENU_XPATH); | ||
| await driver.executeScript('arguments[0].focus()', fileMenuButton2); | ||
| await clickKeys([Key.ENTER, Key.ARROW_DOWN, Key.TAB]); | ||
|
|
||
| const activeElement2 = await driver.switchTo().activeElement(); | ||
| expect(await activeElement2.getAttribute('aria-label')).toBe('Edit menu'); | ||
| }); |
There was a problem hiding this comment.
The fact that we need to reload in the middle of the test makes me think that it'd make more sense to either divide these into two different test cases, e.g. Tab closes File Menu and Tab closes Tab menu after navigation, or test only the second one, since it also test whether "Tab closes the menu".
| .sendKeys(Key.TAB) | ||
| .keyUp(Key.SHIFT) | ||
| .perform(); | ||
| await driver.sleep(SLEEP_TIME); |
There was a problem hiding this comment.
Why do we need the sleep here?
2 participants
Resolves
Part of https://scratchfoundation.atlassian.net/browse/UEPR-445
Proposed Changes
Unit and integration tests to ensure safety.
Reason for Changes
Part of accessibility initiative for Scratch.
Test coverage
13 Unit tests for the hook and 17 Integration tests for the Settings, File and Edit menus