[UEPR-445] Ensure topbar is navigable via tab navigation #403

kbangelov · 2025-12-19T14:15:32Z

Aims to resolve UEPR-445, not before clarifying things.

https://scratchfoundation.atlassian.net/browse/UEPR-445

Proposed Changes

Added tabIndex=0 to the elements in nav bar so they can be focused with tab
Made a context (menu-ref-context) that takes care of tracking which menus and submenus are currently open via their refs. All of the logic with the "isOpen" type of props has been replaced for these menus.
Made a base-menu to be extended by the menus that handles common key pressing, opening, closing logic and more.
Brought out the menu logic in their respective files for the purpose of making them work via base-menu and also for code clarity.

To be discussed additionally

I remain uncertain whether the menus should collapse when the innermost items are clicked and which ones. For now they just keep their current behavior.
At a couple of places in code when I was making new react elements to replace the long lines of code in menu-bar, I was unsure whether it's a good idea to pass them as a prop or copy-paste the logic directly within the class.
The behavior of using screen reader and clicking with cursor leads to improper reads, as opposed to just navigating with keyboard.
I made the observation that mode menu seems to act like the other dropdown menus, but it doesn't have an expansion arrow like them. It is not a very active feature of the app, have to make isTotallyNormal=true in gui.jsx to test it.
I have also replaced isOpenMenu logic with custom such from the Context. Should I end up removing that entirely from code? I mean I think I have done this entirely in this PR the way it is now but should I? Seems like it will become obsolete.
Whatever the comments address.

Tip for reviewing this PR

Start from base-menu and menu-ref-context to understand the basic idea behind the code.
Test well with a screen reader, since it's likely you'll have one better than my chrome extension.
I have a feeling switching language and testing buttons with screen reader doesn't quite work as intended. Have to look into that

Bugs

When navigating with screen reader in the edit menu and "Turn on/off Screen Reader" button is triggered, the updated name of that button is not read correctly immediately (old value is read instead as long as we don't focus away - if we come back, then it's read correctly).
Bad behavior when combining screen reader usage and clicking with cursor.

Missing

aria-selected
aria-disabled
translations of aria-labels and such
The four remaining disabled buttons in the menu bar are untouched. I'm wondering if they're disabled because the changes on them belong to www repo or because they can't be tested locally, in which case I will have to finish them in this PR. Just want to clarify that before touching them.

Copilot

Pull request overview

This PR implements keyboard navigation and accessibility improvements for the top navigation bar, enabling users to navigate menu items using Tab and arrow keys. The changes introduce a new context-based system for tracking open menus and managing focus states across nested menu hierarchies.

Key Changes:

Added tabIndex attributes and ARIA properties to make menu bar elements keyboard-navigable
Implemented arrow key navigation within dropdown menus (Settings, Language, Theme)
Created MenuRefContext to manage the state of open menus and focus tracking across the menu hierarchy

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
packages/scratch-gui/src/components/context-menu/menu-path-context.jsx	New context provider for tracking open menu references and navigation state
packages/scratch-gui/src/components/menu-bar/settings-menu.jsx	Converted to class component with keyboard navigation handlers
packages/scratch-gui/src/components/menu-bar/language-menu.jsx	Added keyboard navigation with arrow keys and Enter/Escape handlers
packages/scratch-gui/src/components/menu-bar/theme-menu.jsx	Added keyboard navigation similar to language menu
packages/scratch-gui/src/components/menu/menu.jsx	Added accessibility props (focusedRef, aria attributes, keyboard handlers)
packages/scratch-gui/src/components/menu-bar/menu-bar.jsx	Added tabIndex and ARIA attributes to menu bar items for keyboard access
packages/scratch-gui/src/components/gui/gui.jsx	Wrapped MenuBar with MenuRefProvider context
packages/scratch-gui/src/containers/gui.jsx	Contains commented test code

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

packages/scratch-gui/src/containers/gui.jsx

packages/scratch-gui/src/components/menu/menu.jsx

packages/scratch-gui/src/components/menu-bar/theme-menu.jsx

packages/scratch-gui/src/components/menu-bar/language-menu.jsx

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx

packages/scratch-gui/src/components/context-menu/menu-path-context.jsx

packages/scratch-gui/src/components/menu-bar/menu-bar.jsx

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

packages/scratch-gui/src/components/gui/gui.jsx

packages/scratch-gui/src/contexts/menu-ref-context.jsx

kbangelov · 2025-12-29T12:19:25Z

packages/scratch-gui/src/components/menu/menu.jsx

+        ref={menuRef}
+        aria-label={ariaLabel}
+        aria-selected={isSelected ?? null}
+        aria-disabled={isDisabled ?? null}


So this applies for both aria-selected and aria-disabled.

They seem to require specific roles for both itself and its container, such as role="option" inside role="listbox", or role="tab" inside role="tablist". For some reason I couldn't really get it to work. But it might be due to my limited screen reader. I'll try to figure out why that is.

packages/scratch-gui/src/components/menu/menu.jsx

packages/scratch-gui/src/components/menu-bar/file-menu.jsx

packages/scratch-gui/src/components/menu-bar/language-menu.jsx

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx

kbangelov · 2025-12-30T12:34:11Z

Ok. Also the tests currently fail. I am not sure I understand why, even with Copilot's help.

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

kbangelov · 2026-01-05T07:53:11Z

Additionally, the aria-labels don't seem to change after language switch, should be tested with a real screen reader.

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx

packages/scratch-gui/src/components/menu-bar/edit-menu.jsx

packages/scratch-gui/src/components/menu-bar/language-menu.jsx

packages/scratch-gui/src/contexts/menu-ref-context.jsx

adzhindzhi · 2026-01-06T09:17:33Z

packages/scratch-gui/src/contexts/menu-ref-context.jsx

+        ]);
+    }
+
+    push (ref, depth) {


I think it would be very helpful for maintainers if you could add comments explaining the intent of each of the push, pop, cut, etc. functions, since it’s currently not easy to infer.

packages/scratch-gui/src/contexts/menu-ref-context.jsx

adzhindzhi · 2026-01-06T10:13:12Z

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

+    }
+
+    handleKeyPressOpenMenu (e) {
+        if (e.key === 'ArrowDown') {


nitpick: Maybe we can extract the keys into an enum?

adzhindzhi · 2026-01-06T10:16:06Z

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

+    }
+
+    handleMove (direction) {
+        const newIndex = (this.state.focusedIndex + direction + this.itemRefs.length) % this.itemRefs.length;


nitpick: I think a comment here explaining that this line calculates the next focused item index, moving up or down, and wraps around the list so you never go out of bounds would be helpful for future maintainers.

adzhindzhi · 2026-01-06T10:34:09Z

packages/scratch-gui/src/components/menu-bar/edit-menu.jsx

+                    [styles.active]: this.isExpanded()
+                })}
+                onClick={this.handleOnOpen}
+                role="button"


nitpick: Does it make sense to use an actual button element? Same question for the other menu components.

adzhindzhi · 2026-01-06T10:49:38Z

packages/scratch-gui/src/components/menu-bar/edit-menu.jsx

+}
+
+EditMenu.propTypes = {
+    intl: intlShape.isRequired,


In order to use intl from the props you need to call injectIntl to the component, for example:

EditMenuIntl = injectIntl(EditMenu); export default EditMenuIntl;

Same comment for the other menus where intl is used from the props.

Edit: I just noticed that you're passing it from the MenuBar component. I still think it's better to inject it here rather than passing it from the parent. By the way, if we switch to function components we can use intl from the useIntl hook.

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx

packages/scratch-gui/src/contexts/menu-ref-context.jsx

KManolov3 · 2026-01-05T15:29:12Z

packages/scratch-gui/src/contexts/menu-ref-context.jsx

+import PropTypes from 'prop-types';
+import bindAll from 'lodash.bindall';
+
+export const MenuRefContext = React.createContext(null);


Can we leave a comment for how this is used, e.g. "Context for manipulating and accessing active menu element. Ensures there is only one open menu stack at a given time. ... "

KManolov3 · 2026-01-06T10:09:30Z

package-lock.json

      "integrity": "sha512-NLhDfH/h4O6UOy+0LSso42xvYypClINuMNBVVzX4vX98TmTaTUxwRbXdhucbFMd2qLaCTcLq/PdYrvi8onw90w==",
      "dev": true,
      "license": "MIT",
+      "peer": true,


I'd avoid committing these package-lock changes, given that they aren't any package.json changes.

KManolov3 · 2026-01-06T10:23:05Z

packages/scratch-gui/src/components/gui/gui.jsx

-                    username={username}
-                    accountMenuOptions={accountMenuOptions}
-                />}
+                {!menuBarHidden && <MenuRefProvider>


Do we only need to manage menu refs on the Menu Bar level? If there are other cases where we'll be managing menu refs outside of here, we'd want to move this to an outer level - otherwise it's fine to stay as-is.

I suppose I should have done it here but I do move it in my next task. Although it is illogical in the long term, I think it's fine to stay there in this PR.

KManolov3 · 2026-01-06T10:31:11Z

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

+import bindAll from 'lodash.bindall';
+import PropTypes from 'prop-types';
+
+/* Subclasses must implement (some optionally):


This comment is hard to maintain (if a method is added or renamed) so I'd prefer not having it. Instead, maybe the base menu-specific logic (as this component contains just logic, without any presentation) can be extracted in a hook and reused in the different menu components.
See https://react.dev/learn/reusing-logic-with-custom-hooks

Also let's ensure the name of this hook is related to the accessibility/tab navigation logic it encapsulates

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

KManolov3 · 2026-01-06T11:13:57Z

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

@@ -0,0 +1,131 @@
+import {MenuRefContext} from '../context-menu/menu-ref-context';


General note - after selecting an item in a menu, and the menu closes, we'd want to keep the focus on the top-level menu element (e.g. keep focus on the "Settings" element after selecting a language

KManolov3 · 2026-01-06T11:20:41Z

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

+    }
+
+    handleKeyPressOpenMenu (e) {
+        if (e.key === 'ArrowDown') {


I imagine this would depend on whether the menu is horizontal or vertical? I am fine with not adding support for horizontal menus, given we don't have them, but maybe it'd still be useful to add a comment.

packages/scratch-gui/src/components/menu-bar/base-menu.jsx

KManolov3 · 2026-01-06T11:47:30Z

packages/scratch-gui/src/components/menu-bar/edit-menu.jsx

+                            onClick={this.props.onRestoreOption(handleRestore)}
+                            menuRef={this.restoreRef}
+                            onParentKeyPress={this.handleKeyPressOpenMenu}
+                            isDisabled={!restorable}


Why was this needed to be added? Because the element was still focusable even if it was disabled? Can we do it either entirely true css styles or through the prop - or do we really need both?

No I just wanted to add aria-disabled and aria-expanded for the menus, I did mention I had problems with doing so in a comment of my own, will try to figure it out.

packages/scratch-gui/src/components/menu-bar/file-menu.jsx

packages/scratch-gui/src/components/menu/menu.jsx

packages/scratch-gui/src/components/menu-bar/language-menu.jsx

KManolov3 · 2026-01-06T13:12:51Z

packages/scratch-gui/src/components/menu-bar/menu-bar.jsx

                        {(this.props.canChangeColorMode || this.props.canChangeLanguage || this.props.canChangeTheme) &&
                        (<SettingsMenu
+                            menuRef={this.settingsRef}
+                            depth={1}


Would there be any way to avoid passing the depth explicitly, but calculating it based on what's in the context when it's opened (and exposing a method from the context to access it)?

KManolov3 · 2026-01-06T13:49:40Z

packages/scratch-gui/src/reducers/menus.js

    openThemeMenu,
-    closeThemeMenu,
-    themeMenuOpen
+    closeThemeMenu


Are the XMenuOpen reducers redundant now? If that's the case, is there any reason to continue supporting this file at all?

Yes, they seem to be redundant. I'll remove all the old logic for the ones in the menu bar. We'll eventually delete the file but for now there are still some menus there outside of the scope of the PR

KManolov3 · 2026-01-06T14:02:18Z

packages/scratch-gui/src/contexts/menu-ref-context.jsx

+        ]);
+    }
+
+    push (ref, depth) {


What do you think about naming these closer to how they're used, rather than toward a data structure namespace - so e.g. open, closeCurrent, closeFrom or something with similar semantics?

KManolov3 · 2026-01-06T14:05:44Z

packages/scratch-gui/src/components/menu-bar/mode-menu.jsx

+                >
+                    <MenuSection>
+                        <MenuItem
+                            onClick={onSetMode('NOW')}


Since we're not refering these in several files, let's extract them in an enum

KManolov3 · 2026-01-06T14:07:01Z

packages/scratch-gui/src/components/menu-bar/mode-menu.jsx

+    description: 'ARIA label for mode menu'
+});
+
+export class ModeMenu extends BaseMenu {


Let's make sure that we don't change any of the previous behaviour here.

adzhindzhi · 2026-01-06T15:24:34Z

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx

+        this.colorRef = React.createRef();
+        this.itemRefs = [
+            ...(this.props.canChangeLanguage ? [this.languageRef] : []),
+            ...(this.props.canChangeTheme && this.props.availableThemesLength > 1 ? [this.themeRef] : []),


I noticed that the themes menu is not navigable through the keyboard. This is most likely caused by the fact that availableThemesLength is 1 on the initial render, because the membership data has not loaded from the session yet. Since we are only setting the itemRefs in the constructor, the themes menu never becomes a part of the items. This most likely won't be an issue if we switch to a function component, so that's another reason for doing so 😅

adzhindzhi · 2026-01-06T15:51:18Z

Ok. Also the tests currently fail. I am not sure I understand why, even with Copilot's help.

I think the test failures are because MenuBar is rendered in the tests without the new context provider. With the recent changes, the context ends up being null, so the component can’t render correctly.

…menus

kbangelov added 2 commits December 15, 2025 11:46

chore: initial commit - making elements focusable and react to enter

a2f0390

feat: added some accessability with arrow logic

06d2d62

kbangelov requested a review from a team as a code owner December 19, 2025 14:15

kbangelov requested review from KManolov3 and adzhindzhi December 19, 2025 14:23

KManolov3 requested a review from cwillisf December 19, 2025 14:26

KManolov3 marked this pull request as draft December 19, 2025 14:26

KManolov3 requested a review from Copilot December 19, 2025 14:27

Copilot AI reviewed Dec 19, 2025

View reviewed changes

kbangelov added 6 commits December 19, 2025 17:28

chore: addressed copilot stuff

34b18df

feat: refactored dropdown menu logic

1c05d24

feat: more dropdowns in new files and stuff

9914e61

chore: completed functionality and refactored code inconsistencies

328b234

chore: added aria-expanded everywhere on dropdowns

0c20327

chore: code redacting

6f3a80d

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/base-menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/base-menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/gui/gui.jsx Outdated Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/contexts/menu-ref-context.jsx Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu/menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/file-menu.jsx Outdated Show resolved Hide resolved

chore: cleanup of isOpen logic for menus, since it is reimplemented

05fb1da

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/file-menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Dec 29, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/language-menu.jsx Show resolved Hide resolved

kbangelov marked this pull request as ready for review December 29, 2025 13:16

kbangelov added 3 commits December 29, 2025 15:17

chore: adjusted key press logic to match wanted functionality

333f1f8

Merge branch 'develop' into task/uepr-445-ensure-navigable-toolbar

f1ea8f6

chore: post-merge accessibility readjustments

2ce4eac

kbangelov commented Dec 30, 2025

View reviewed changes

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx Outdated Show resolved Hide resolved

kbangelov added 2 commits December 30, 2025 14:35

chore: minor code changes

fdac2f4

chore: translated labels and some more adjustments

77f2ac6

kbangelov commented Jan 5, 2026

View reviewed changes

packages/scratch-gui/src/components/menu-bar/base-menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Jan 5, 2026

View reviewed changes

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx Outdated Show resolved Hide resolved

chore: undeleted a line

3f518b5

kbangelov commented Jan 5, 2026

View reviewed changes

packages/scratch-gui/src/components/menu-bar/edit-menu.jsx Outdated Show resolved Hide resolved

kbangelov commented Jan 5, 2026

View reviewed changes

packages/scratch-gui/src/components/menu-bar/language-menu.jsx Outdated Show resolved Hide resolved

adzhindzhi reviewed Jan 6, 2026

View reviewed changes

KManolov3 reviewed Jan 6, 2026

View reviewed changes

adzhindzhi reviewed Jan 6, 2026

View reviewed changes

kbangelov added 6 commits January 6, 2026 17:56

chore: moved file

83bdad7

chore: beginning to remove now obsolete old logic related to opening …

5640a76

…menus

chore: refactored menu navigation logic via a hook

4f92988

chore: more refactoring

287e1a5

chore: removed old menus logic

19fef13

chore: package-lock.json back to original

d2fd053

kbangelov force-pushed the task/uepr-445-ensure-navigable-toolbar branch from 081c531 to d2fd053 Compare January 8, 2026 14:10

kbangelov added 2 commits January 8, 2026 16:16

chore: deleting rows

016f46f

chore: refactored some menuRef, itemRef code

d75933d

kbangelov force-pushed the task/uepr-445-ensure-navigable-toolbar branch from d63eb5a to d75933d Compare January 9, 2026 08:33

kbangelov added 5 commits January 9, 2026 10:36

chore: passing down remix message

e186ba9

chore: added some isRequired-s

37c9974

chore: brought some elements inside file menu

fa75484

chore: fixed some aria labels

8626a1a

chore: context is function instead of class now

965434b

scratchfoundation deleted a comment from KManolov3 Jan 9, 2026

		@@ -0,0 +1,131 @@
		import {MenuRefContext} from '../context-menu/menu-ref-context';

[UEPR-445] Ensure topbar is navigable via tab navigation #403

Are you sure you want to change the base?

[UEPR-445] Ensure topbar is navigable via tab navigation #403

Uh oh!

Conversation

kbangelov commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Aims to resolve UEPR-445, not before clarifying things.

Proposed Changes

To be discussed additionally

Tip for reviewing this PR

Bugs

Missing

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kbangelov Dec 29, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kbangelov commented Dec 30, 2025

Uh oh!

Uh oh!

kbangelov commented Jan 5, 2026

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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 Jan 6, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbangelov commented Dec 19, 2025 •

edited

Loading

kbangelov Dec 29, 2025 •

edited

Loading

KManolov3 Jan 6, 2026 •

edited

Loading

kbangelov Jan 7, 2026 •

edited

Loading