Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
kbangelov wants to merge 62 commits into
base: develop
Choose a base branch
from
Open

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

kbangelov wants to merge 62 commits into from

Conversation

@kbangelov
Copy link
Contributor

@kbangelov kbangelov commented Dec 19, 2025 •
edited
Loading

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. related BUG: screen reader sometimes fails to pick up latest version of some elements immediately (can be observed on menus that are collapsed (says expanded), can also be observed on "turn on/off turbo mode", as well as on about menu in case we mock it (with an array) so that it closes upon clicking a menu item
  • Login Menu doesn't navigate properly (close on tab for instance) because it's written in www
  • 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. BUG: Language Menu seems to cause a button inside button warning, not too obvious why that is.
  • 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. But that might be too much of an edge case for practical purposes.

@kbangelov kbangelov requested a review from a team as a code owner December 19, 2025 14:15
@KManolov3 KManolov3 requested a review from cwillisf December 19, 2025 14:26
@KManolov3 KManolov3 marked this pull request as draft December 19, 2025 14:26
@KManolov3 KManolov3 requested a review from Copilot December 19, 2025 14:27
Copilot AI reviewed Dec 19, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
kbangelov
kbangelov commented Dec 29, 2025
View reviewed changes
@kbangelov kbangelov marked this pull request as ready for review December 29, 2025 13:16
kbangelov
kbangelov commented Dec 30, 2025
View reviewed changes
KManolov3
KManolov3 reviewed Jan 20, 2026
View reviewed changes
KManolov3
KManolov3 reviewed Jan 20, 2026
View reviewed changes
adzhindzhi
adzhindzhi reviewed Jan 20, 2026
View reviewed changes
@KManolov3
Copy link
Contributor

KManolov3 commented Jan 21, 2026

image

Can we fix only the inner part of the menu item highlighting in the case of Settings' menu items? We'd want a behaviour similar to what happens on-hover in e.g. the editor version on staging:

image

KManolov3
KManolov3 reviewed Jan 21, 2026
View reviewed changes
Copilot AI reviewed Jan 23, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 6 comments.

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

packages/scratch-gui/src/components/menu/menu.jsx
tabIndex={-1}
aria-label={ariaLabel}
aria-selected={isSelected}
aria-disabled={isDisabled}
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

The aria-disabled attribute should be a string ('true' or 'false'), not a boolean value. Change to aria-disabled={isDisabled ? 'true' : 'false'} or use the disabled attribute directly on interactive elements.

Suggested change
aria-disabled={isDisabled}
aria-disabled={isDisabled ? 'true' : 'false'}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@kbangelov kbangelov Jan 23, 2026

Choose a reason for hiding this comment

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

I am not sure this is right? I think it converts to string by itself

adzhindzhi
adzhindzhi reviewed Jan 26, 2026
View reviewed changes
Copy link
Contributor

@adzhindzhi adzhindzhi left a comment

Choose a reason for hiding this comment

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

I left a few minor comments, but it looks great overall!

packages/scratch-gui/src/components/menu-bar/settings-menu.jsx Outdated
Comment on lines 82 to 83
data-menu-item
data-menu-item-wrapper
Copy link
Contributor

@adzhindzhi adzhindzhi Jan 23, 2026

Choose a reason for hiding this comment

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

Can you remind me why is it both a menu-item and a wrapper?

Copy link
Contributor Author

@kbangelov kbangelov Jan 27, 2026

Choose a reason for hiding this comment

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

My bad. Top level menus don't need to be either of those. Must have been from some former logic. It literally does not make a difference. I might have to consider keeping the menu-item in case they stop being top level menus?

@kbangelov kbangelov requested a review from adzhindzhi January 27, 2026 10:13
@kbangelov kbangelov force-pushed the task/uepr-445-ensure-navigable-toolbar branch from 811a72d to 5c6d9ff Compare January 27, 2026 10:15
@kbangelov kbangelov force-pushed the task/uepr-445-ensure-navigable-toolbar branch from 5c6d9ff to d393d77 Compare January 27, 2026 10:17
adzhindzhi
adzhindzhi approved these changes Feb 2, 2026
View reviewed changes
Copy link
Contributor

@adzhindzhi adzhindzhi left a comment

Choose a reason for hiding this comment

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

I think it looks good! Thank you! 🙌

packages/scratch-gui/src/hooks/use-menu-navigation.jsx
Comment on lines +64 to +65
const isRtlFromStore = useSelector(state => state.locales.isRtl);
const isRtl = isRtlProp ?? isRtlFromStore;
Copy link
Contributor

@adzhindzhi adzhindzhi Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like we're currently always passing isRtl as a prop, so the hook never actually falls back to the store value. I don't mind keeping this as a fallback, but I wonder if it would make more sense for components to be responsible for reading from the store and passing the value explicitly.

Copy link
Contributor Author

@kbangelov kbangelov Feb 2, 2026

Choose a reason for hiding this comment

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

Yes currently we have a double insurance and they do already read from state as a fallback, besides getting passed that value as a prop.

KManolov3
KManolov3 reviewed Feb 2, 2026
View reviewed changes
packages/scratch-gui/src/components/menu-bar/settings-menu.jsx
selectedItemKey={activeTheme}
isRtl={isRtl}
depth={2}
depth={depth + 1}
Copy link
Contributor

@KManolov3 KManolov3 Feb 2, 2026

Choose a reason for hiding this comment

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

What would happen if depth isn't passed as a prop? We should probably add a default 0 in the component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KManolov3 KManolov3 KManolov3 left review comments

Copilot code review Copilot Copilot left review comments

@adzhindzhi adzhindzhi adzhindzhi approved these changes

@cwillisf cwillisf Awaiting requested review from cwillisf

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kbangelov @adzhindzhi @KManolov3