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

[kbangelov]

Aims to resolve UEPR-445

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

Proposed Changes

1. Added tabIndex=0 to the elements in nav bar so they can be focused with tab
2. 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.
3. Made a use-menu-navigation hook to be used by the menus. Handles common key pressing, opening, closing logic and more. Uses customized BFS logic for HTML components
4. 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 done additionally concerning nav bar accessibility - in other PRs?

• Choosing a language option does not focus on Settings after. Also not for the other options either, but that's because not closing after selecting an option has been their behavior so far. Perhaps another issue could be made that clears whether they should or not.
• I have written some aria-enabled and aria-disabled logic, however, it doesn't seem to work yet.
• Semantic improvements might be needed. I made some divs with role="button" into actual button elements but there are some img components left which have that role. And probably other things too.
• Focus is not too pretty on some menu items.
• Write tests for navigation bar

To be discussed additionally

• The behavior of using screen reader and clicking with cursor leads to improper reads, as opposed to just navigating with keyboard. But I don't know if that's a big issue.

Bugs - currently testing

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

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

[kbangelov]

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.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.

---

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

[KManolov3]

Really like how it's turning out with the hook. The accessibility behaviour logic looks quite encapsulated, which means we can test it and make global changes to the behavior easily!

[KManolov3]

The one thing that's responsible for a lot of boilerplate code in the components using this hook is the itemRefs param, especially in the places where the itemRefs need to be defined manually.

I wonder if we can reach a state where we don't need to pass them explicitly. One thing we could do is pass a data prop to the selectable menu items (e.g. pass data-menu-item=true as a prop) and then use it here to select all children items of the menu.

const MENU_ITEM_SELECTOR = '[data-menu-item="true"]';
...
const handleOnOpen = useCallback(() => {
        if (menuContext.isOpenMenu(menuRef)) return;

        menuContext.openInnerMenu(menuRef, depth);
        
        // Wait for the UI to be rendered before interacting with the DOM
        requestAnimationFrame(() => {
            // Select all the children of the menu marked as items
            const items = menuRef.current?.querySelectorAll(MENU_ITEM_SELECTOR);
            if (items && items[defaultIndexOnOpen]) {
                items[defaultIndexOnOpen].focus();
            }
        });
    }, [menuContext, menuRef, depth, defaultIndexOnOpen]);

const handleMove = useCallback(direction => {
        if (!menuRef.current) return;

        const items = Array.from(menuRef.current.querySelectorAll(MENU_ITEM_SELECTOR));
        if (items.length === 0) return;

        // Maybe we can use the document's active element to check what's the currently active element,
        // instead of keeping it in state with `focusedIndex` ?
        const currentIndex = items.indexOf(document.activeElement);
        
        const nextIndex = (currentIndex + direction + items.length) % items.length;
        items[nextIndex].focus();
    }, [menuRef]);
...etc

The submenus will still need their own refs, but they could be created in the components themselves (e.g. fileMenu can create its own menuRef to use for useMenuNavigation, instead of it being passed from outside.

I'm not completely sure how this will work in practice (e.g. if we'll end up with some race conditions due to the DOM rendering), but it sounds workable to me in theory.

What do you think?

cc: @adzhindzhi, I'd like to hear your thoughts as well

[adzhindzhi]

I definitely agree that reducing the boilerplate around itemRefs would be nice, but I’m not entirely sure querying the DOM is the best way to do it. It feels a bit hacky/fragile and depends on things already being rendered. That said, we could try it out and see if any issues come up in practice.

Another direction I was thinking of was keeping the hook ref-based, but letting MenuItem create its own ref and register it through the hook (by exposing a register method or something similar), instead of passing refs around from the outside. That might let us get rid of the manual ref arrays without relying on DOM queries. This is more of a thought than a strong opinion, though, and I'm not sure if something like that would actually be applicable, considering various edge cases, etc., so I'm definitely fine with trying out the data-prop approach instead.

[KManolov3]

Another direction I was thinking of was keeping the hook ref-based, but letting MenuItem create its own ref and register it through the hook

That was a direction I was also thinking in, but I'm not sure whether that guarantees us the correct order of items, especially in cases where one of the non-last items depends on a (potentially async) condition

[kbangelov]

@adzhindzhi if menu items were to create their own ref, I'm assuming you want to pass down the register method that the hook returns from the above menu? If so, I think I like that approach better since it's more React-esque and would solve the manual itemRefs declaring (which also has some repeated logic for conditional rendering which a maintainer might miss to alter)

cc: @KManolov3 would you agree it's better than the query selector approach?

[adzhindzhi]

I was more so thinking about using the hook in MenuItem itself and avoiding passing the function, if possible. But Kalo’s right that we may run into issues with the order of items, so we’d need to be careful there as well.

[KManolov3]

I think the correctness of the item order is what worries me w/ the MenuItem "registration" approach. Feel free to experiment locally with it.

Otherwise, I think I'd give the data-attribute approach a try; it may not be the prettiest, but, unless there are async issues with waiting for the DOM to be rendered, which can't be reliably addressed with something like requestAnimationFrame, it should get the job done and reduce boilerplate.

If either of those don't work, or end up revealing unexpected complexity, I'm okay with logging a task to address this in a later PR.

[adzhindzhi]

I definitely agree that reducing the boilerplate around itemRefs would be nice, but I’m not entirely sure querying the DOM is the best way to do it. It feels a bit hacky/fragile and depends on things already being rendered. That said, we could try it out and see if any issues come up in practice.

Another direction I was thinking of was keeping the hook ref-based, but letting MenuItem create its own ref and register it through the hook (by exposing a register method or something similar), instead of passing refs around from the outside. That might let us get rid of the manual ref arrays without relying on DOM queries. This is more of a thought than a strong opinion, though, and I'm not sure if something like that would actually be applicable, considering various edge cases, etc., so I'm definitely fine with trying out the data-prop approach instead.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.

---

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

[KManolov3]

FYI, there are some conflicts which we need to resolve before merging.

[adzhindzhi]

Hm, that's not an interactive element, do we want it to be focusable? Screen readers should be able to read it even without the tabIndex. Also I am not entirely sure we need the aria-label, screen readers would read the rendered text, which is projectTitle + username anyway?

[kbangelov]

Screen readers should be able to read it even without the tabIndex

Yes. But what I hadn't noticed was that it was a landmark. I don't think it should be. I changed the role to presentation which should not be read by good screen readers. You are welcome to suggest a better one.

As for the aria-label, I don't see harm in customizing the way that is read - it goes from "ProjectNameby username" to "Project ProjectName by username".

[adzhindzhi]

Judging by the contentinfo role description, I don’t think it’s a good fit here. It’s intended for page-level footer content, so it probably makes sense to remove the role altogether (as I don't see anything else that could be a fit).

Regarding tabIndex, it’s not related to what screen readers announce. It only controls whether an element is focusable. Since this element isn’t interactive, it shouldn’t be focusable.

As for aria-label, I don’t think it’s a good practice to use it on non-interactive elements when the content is already readable by screen readers. While I understand the intent to improve how it’s announced, it seems more appropriate to fix the rendered text itself (for example by adding {' '}), rather than overriding it with an aria-label when it’s not necessary. I’m not strongly opposed to leaving it as-is, but I believe it’s unnecessary.

[Copilot]

Pull request overview

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

---

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

[Copilot]

An img element with role=\"button\", tabIndex, and aria-label should be replaced with an actual button element containing the image for better semantic HTML and consistent keyboard interaction patterns.

[Copilot]

Inconsistent style import usage. The file imports both styles (from account-menu.css) and stylesMenuBar (from menu-bar.css), but dropdownCaretIcon is referenced from stylesMenuBar while other nearby elements use styles. Consider consolidating styles or documenting why this specific class comes from a different stylesheet.

Suggested change

	<div className={styles.dropdownCaretPosition}>
	<div className={styles.dropdownCaretPosition}>
	{/* Use shared menu bar styling for the caret icon to keep its appearance consistent
	* with other menu bar components. The surrounding layout uses account-menu styles.
	*/}

[kbangelov]

The most I'll go is rename styles to stylesAccountMenu. Comments seem like overkill

[Copilot]

The element has tabIndex={0} making it focusable, but role=\"presentation\" indicates it should be ignored by assistive technologies. These attributes conflict. If the element should be focusable and announced by screen readers (which the aria-label suggests), use a semantic element like button or a more appropriate role like region or group.

Suggested change

	role="presentation"
	role="group"

[kbangelov]

perhaps it's onto something, I was wondering what role i should give it.