[UEPR-445] Topbar unit and integration tests by kbangelov · Pull Request #438 · scratchfoundation/scratch-editor

kbangelov · 2026-02-09T13:03:20Z

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

Copilot

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 useMenuNavigation hook 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

packages/scratch-gui/package.json

Copilot · 2026-02-13T10:59:51Z

packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx

+        store = configureStore({
+            reducer: {
+                locales: (state = {isRtl: false}) => state
+            }
+        });


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.

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?

I am okay with adding @reduxjs/toolkit since it's recommended to move onto it. Let's use it only here for now - I prefer us to migrate the rest of the codebase separately, as these PRs already carry a lot of changes with them

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

adzhindzhi · 2026-02-19T14:09:08Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+    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');
+    });


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"?

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

adzhindzhi · 2026-02-19T14:18:22Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+    const clickKey = async key => {
+        await driver.actions().sendKeys(key)
+            .perform();
+        await driver.sleep(SLEEP_TIME);


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?

In most cases we do want it, I think. Some tests occasionally fail when sleep is 0 because of not having enough time to react. I just tested with and without sleep (50ms vs 0) and in both cases the tests took just under 20 seconds to complete. So it doesn't seem to really prolong the test time.

adzhindzhi · 2026-02-19T14:55:16Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        // Explicit keyboard focus
+        await driver.executeScript('arguments[0].focus()', fileMenuButton);
+        await clickKey(Key.ENTER);


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).

Yeah that's another variant, I suppose. This current way of doing it seems more consistent from a readability standpoint though, at least in my view. Although there's not much separating either variant for me.

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

adzhindzhi · 2026-02-19T15:10:56Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+    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');
+    });


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".

Perhaps the second part isn't different enough for its own test so I'll just remove it.

adzhindzhi · 2026-02-19T15:14:46Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+            .sendKeys(Key.TAB)
+            .keyUp(Key.SHIFT)
+            .perform();
+        await driver.sleep(SLEEP_TIME);


Why do we need the sleep here?

Well I added it anywhere and here the click is a little bit more specific (because of shift) so I can't use clickKey(s)

KManolov3 · 2026-02-20T16:17:13Z

General question - how much execution time do these tests add to the existing test suites?

kbangelov · 2026-02-20T16:26:18Z

General question - how much execution time do these tests add to the existing test suites?

18s for integration and 1s for unit.

KManolov3

Great job! I really like how the tests have turned out, especially the unit ones. I think they will be very usefuil both for documentation and against regressions.

KManolov3 · 2026-02-20T16:23:07Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        await driver.executeScript('arguments[0].focus()', fileMenuButton);
+        await clickKey(Key.ENTER);
+
+        // ArrowUp should go to last item


Nitpick: this comment is redundant as the test name already implies this is the expected behavior

KManolov3 · 2026-02-23T10:47:07Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        const text = await activeElement.getText();
+        expect(text).toBe('Restore');
+
+        // ArrowUp should go to last item


Same as last comment

KManolov3 · 2026-02-23T10:52:13Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        const activeElement2 = await driver.switchTo().activeElement();
+        const text2 = await activeElement2.getText();
+
+        expect(text2).toBe('Turn on Turbo Mode');


A general note: it's not a great practice to depend on text for selecting elements - because if the text changes, that will cascade into test changes that may have nothing to do with the text itself. I know there are some other instances of similar logic in the editor's integration tests (e.g. we have findByText). I also think that it'll be out of the scope of this task to implement a more resillient mechanism for selecting items (e.g. aria attributes) - just outlying some thoughts.

KManolov3 · 2026-02-23T10:54:47Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        const activeElement = await driver.switchTo().activeElement();
+        const text = await activeElement.getText();
+
+        expect(text).toBe('Restore');


Would it make sense for this to also check the length of the edit menu subitems, and make the checks based on that length (e.g. we expect two items, we are on the first item after pressing enter, and are on the first item again after pressing arrow down twice)

Similar logic may be able to be used for some of the other tests' comparisons - for example 'ArrowUp moves focus to the last item in edit menu'

KManolov3 · 2026-02-23T10:57:10Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        expect(text).toBe('Restore');
+    });
+
+    test('Enter opens the Language submenu', async () => {


Enter opens the Language submenu should probably omit the "Language" from its naming (and possibly its logic as well, if we can check whether the first item is a submenu). If it's quite a bit easier to check directly for the "Language" submenu, I am okay with leaving this check, but generally let's try to abstract ourselves from the concrete layout where possible - the goal of these test cases is to check the menu tab navigation logic.

Same comment about naming for other testcases referencing a concrete menu.

When you execute the tests, the name is formed from the describe clause + the test name. So this would be:
Menu bar keyboard navigation Enter opens the Language submenu which doesn't make sense as a normal sentence.

It might be better to have it as "can open submenu with Enter", so that the whole test becomes Menu bar keyboard navigation can open submenu with Enter

What do you think?

KManolov3 · 2026-02-23T13:03:06Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        const settingsMenuButton = await findByXpath(SETTINGS_MENU_XPATH);
+        await driver.executeScript('arguments[0].focus()', settingsMenuButton);
+        await clickKeys([Key.ENTER, Key.ARROW_RIGHT]);
+        const persianMenuItem = await findByXpath(


Any chance we can depend on aria labels here, as we do in the tests above?

KManolov3 · 2026-02-23T13:07:31Z

packages/scratch-gui/test/integration/menu-bar-keyboard-nav.test.js

+        const text = await activeElement.getText();
+        expect(text).toBe('فارسی');
+        // Suspiciously fragile logic, might need to
+        // be altered in case of language option changes


I generally agree, though I think this might be one of the more resilient selectors 😄

KManolov3 · 2026-02-23T13:12:50Z

packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx

+        store = configureStore({
+            reducer: {
+                locales: (state = {isRtl: false}) => state
+            }
+        });


I am okay with adding @reduxjs/toolkit since it's recommended to move onto it. Let's use it only here for now - I prefer us to migrate the rest of the codebase separately, as these PRs already carry a lot of changes with them

KManolov3 · 2026-02-23T13:40:29Z

packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx

+    //   <li data-menu-item>Item 2</li>
+    //  </div>
+    // <div>
+    test('handleOnOpen focuses first item inside submenu wrapper, not the button and then refocuses on close', () => {


-> focuses first item inside submenu wrapper with handleOnOpen, skipping parent menu button ?

KManolov3 · 2026-02-23T13:42:20Z

packages/scratch-gui/test/unit/hooks/use-menu-navigation.test.jsx

+            expect(buttonFocusSpy).not.toHaveBeenCalled();
+        });
+    });
+});


Let's also add a test checking that items without data-menu-item / with data-menu-item false are skipped over

kbangelov requested a review from a team as a code owner February 9, 2026 13:03

kbangelov marked this pull request as draft February 9, 2026 13:06

kbangelov changed the base branch from develop to release/accessibility-improvements February 10, 2026 10:56

kbangelov force-pushed the release/accessibility-improvements branch 2 times, most recently from e37597f to f2902ce Compare February 13, 2026 09:00

scratchfoundation deleted a comment from github-actions bot Feb 13, 2026

kbangelov changed the base branch from release/accessibility-improvements to release/UEPR-297-accessibility-improvements February 13, 2026 10:09

kbangelov force-pushed the task/uepr-445-topbar-tests branch from 98bf7f1 to 10cec89 Compare February 13, 2026 10:40

kbangelov requested review from KManolov3 and adzhindzhi February 13, 2026 10:41

kbangelov marked this pull request as ready for review February 13, 2026 10:41

kbangelov force-pushed the task/uepr-445-topbar-tests branch from 10cec89 to 814f74d Compare February 13, 2026 10:52

kbangelov requested a review from Copilot February 13, 2026 10:55

Copilot started reviewing on behalf of kbangelov February 13, 2026 10:55 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

kbangelov force-pushed the task/uepr-445-topbar-tests branch 2 times, most recently from 1b2b384 to a7b81d7 Compare February 13, 2026 12:00

kbangelov added 10 commits February 13, 2026 14:22

chore: initial integration tests batch

fbbdd00

chore: completed integration tests

2623da2

chore: unit tests

8fa33b5

chore: old package lock

f024171

chore: brought back package-lock changes because of new dependency

c5f766f

chore: updated package lock

ea51fc4

chore: installed dependency for testing

e663af8

chore: package-lock.json reverse changes

ce99b9f

chore: installed dependency

2fcc10a

chore: addressing copilot comments

b0af01a

kbangelov force-pushed the task/uepr-445-topbar-tests branch from a7b81d7 to b0af01a Compare February 13, 2026 12:24

adzhindzhi reviewed Feb 19, 2026

View reviewed changes

chore: brought out methods and improved tests

4a105c7

kbangelov requested a review from adzhindzhi February 20, 2026 15:56

chore: updated import

bcb42bd

KManolov3 reviewed Feb 23, 2026

View reviewed changes

Comments

Conversation

kbangelov commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolves

Proposed Changes

Reason for Changes

Test coverage

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbangelov Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KManolov3 commented Feb 20, 2026

Uh oh!

kbangelov commented Feb 20, 2026

Uh oh!

KManolov3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

kbangelov commented Feb 9, 2026 •

edited

Loading

kbangelov Feb 20, 2026 •

edited

Loading