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

Copilot · 2025-12-19T14:28:25Z

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

 };

 GUI.defaultProps = {
+    // isTotallyNormal: true, - for testing only


Remove commented-out test code before merging. This appears to be temporary debugging code that should not be committed.

Suggested change

// isTotallyNormal: true, - for testing only

// Set to true only in specific testing scenarios.

Copilot · 2025-12-19T14:28:25Z

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

    expanded: PropTypes.bool,
-    onClick: PropTypes.func
+    onClick: PropTypes.func,
+    focusedRef: PropTypes.object,


Using PropTypes.object is discouraged. Consider using PropTypes.shape({ current: PropTypes.instanceOf(Element) }) for ref objects, or create a custom PropType validator if this will remain in the code.

Suggested change

focusedRef: PropTypes.object,

focusedRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }),

Copilot · 2025-12-19T14:28:25Z

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

    onClick: PropTypes.func,
-    theme: PropTypes.string
+    theme: PropTypes.string,
+    focusedRef: PropTypes.object,


Using PropTypes.object is discouraged. Consider using PropTypes.shape({ current: PropTypes.instanceOf(Element) }) for ref objects, or create a custom PropType validator.

Copilot · 2025-12-19T14:28:26Z

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

+        this.setState({focusedIndex: -1}, () => {
+            this.setFocusedRef(this.props.focusedRef);
+        });
+        closeThemeMenu();


This appears to call closeThemeMenu() directly instead of dispatching it. Based on the import, this should be this.props.onRequestClose() or the function should be dispatched. The current implementation will not update the Redux state correctly.

Suggested change

closeThemeMenu();

this.props.onRequestClose();

Copilot · 2025-12-19T14:28:26Z

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

+        this.setState({focusedIndex: -1}, () => {
+            this.setFocusedRef(this.props.focusedRef);
+        });
+        closeLanguageMenu();


This appears to call closeLanguageMenu() directly instead of dispatching it. This should likely use the Redux dispatch mechanism or call an appropriate prop function to properly update state.

Suggested change

closeLanguageMenu();

this.props.dispatch(closeLanguageMenu());

Copilot · 2025-12-19T14:28:26Z

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


 LanguageMenu.propTypes = {
    currentLocale: PropTypes.string,
+    focusedRef: PropTypes.object,


Using PropTypes.object is discouraged. Consider using PropTypes.shape({ current: PropTypes.instanceOf(Element) }) for ref objects, or create a custom PropType validator.

Suggested change

focusedRef: PropTypes.object,

focusedRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}),

Copilot · 2025-12-19T14:28:26Z

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

+        this.state = {focusedIndex: -1};
+        this.languageRef = React.createRef();
+        this.themeRef = React.createRef();
+        // harcoded logic because of only two options


Corrected spelling of 'harcoded' to 'hardcoded'.

Suggested change

// harcoded logic because of only two options

// hardcoded logic because of only two options

Copilot · 2025-12-19T14:28:27Z

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

+    // printChain () {
+    //     console.log(this.state.openRefs);
+    // }


Remove commented-out debugging code. The printChain method and its reference in the context value should be deleted before merging.

Copilot · 2025-12-19T14:28:27Z

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

                            <img
+                                role="button"
+                                aria-label="Go Home"
+                                tabIndex="0"


Use numeric value instead of string for tabIndex. Change tabIndex=\"0\" to tabIndex={0} for consistency with React conventions.

Suggested change

tabIndex="0"

tabIndex={0}

kbangelov · 2025-12-29T11:31:59Z

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

+        this.context.print();
+    }
+
+    handleOnClose () {


uncertain whether the menus should collapse when they are clicked and which ones. For now they just keep their current behaviour.

kbangelov · 2025-12-29T11:49:55Z

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

+        ]);
+
+        this.state = {focusedIndex: -1};
+        this.menuRef = props.menuRef;


I think it's fine to leave it like this for shorter lines on usage down below. Idk

kbangelov · 2025-12-29T11:51:05Z

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

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


So I wasn't really sure where I should wrap this. It is only used in the menu bar so far, perhaps later it could easily be changed for entire editor. I'm not sure whether there will be conflicting contexts.

kbangelov · 2025-12-29T11:51:47Z

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

Also not sure in which folder to put the file

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.

kbangelov · 2025-12-29T12:22:08Z

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

 );

 MenuItem.propTypes = {
+    menuRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}),


This repeats quite a few times. I should probably define it somewhere but I'm not really sure where and whether we have such a file.

kbangelov · 2025-12-29T12:23:02Z

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

+                id="gui.menuBar.saveAsCopy"
+            />
+        );
+        const remixMessage = (


there's a duplicate definition so this can't be the final version of the PR. But I wonder if I should pass it down as a prop?

kbangelov · 2025-12-29T12:47:06Z

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

+        this.createRef = React.createRef();
+        this.remixRef = React.createRef();
+        this.loadFromComputerRef = React.createRef();
+        this.saveToComputerRef = React.createRef();


The following question applies for the other constructors of custom menus as well.

Should the logic be this manual? Seems fine to me, since we're talking about menu items with specific behavior. Can't really think of a better alternative.

kbangelov · 2025-12-29T12:49:46Z

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

        // If we are using hover rather than clicks for submenus, scroll the selected option into view
        if (!this.props.menuOpen && this.selectedRef) {
            this.selectedRef.scrollIntoView({block: 'center'});
        }


Comment is regarding the handleMouseOver method. Should I also focus on the selected language menu item when just hovering? I, personally, don't think that's a good idea.

kbangelov · 2025-12-30T11:05:30Z

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] : []),


this logic repeats a lot with item refs, so it is typed twice in every such menu - once here and once in the render method. Perhaps it could be cleaned up with a sort of condition variable, because someone making a change might forget about accessibility.

kbangelov · 2025-12-30T12:16:37Z

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

-    onRequestOpen,
-    settingsMenuOpen
-}) => {
-    const enabledColorModesMap = useMemo(() => Object.keys(colorModeMap).reduce((acc, colorMode) => {


I have removed the useMemo usage. It will probably rerender too often this way. It was just not allowed in a render() method, I was just not sure what workaround is available here. I thought it'd be easier to ask and resolve it quickly afterwards.

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

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

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/context-menu/menu-ref-context.jsx

Copy link

Contributor Author

kbangelov Dec 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure in which folder to put the file

kbangelov commented Dec 29, 2025

View reviewed changes

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

05fb1da

kbangelov commented Dec 29, 2025

View reviewed changes

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

kbangelov added 2 commits December 30, 2025 14:35

chore: minor code changes

fdac2f4

chore: translated labels and some more adjustments

77f2ac6

	// isTotallyNormal: true, - for testing only
	// Set to true only in specific testing scenarios.

	focusedRef: PropTypes.object,
	focusedRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }),

	closeLanguageMenu();
	this.props.dispatch(closeLanguageMenu());

	focusedRef: PropTypes.object,
	focusedRef: PropTypes.shape({current: PropTypes.instanceOf(Element)}),

	// harcoded logic because of only two options
	// hardcoded logic because of only two options

[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!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

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!

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!

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 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!

kbangelov Dec 30, 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!

Choose a reason for hiding this comment

Uh oh!

kbangelov commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

kbangelov commented Dec 19, 2025 •

edited

Loading

kbangelov Dec 29, 2025 •

edited

Loading

kbangelov Dec 29, 2025 •

edited

Loading

kbangelov Dec 30, 2025 •

edited

Loading