[UEPR-462] Ensure "Stage" and "Target" editor regions are navigable through tab navigation

[kbangelov]

Resolves

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

Proposed Changes

Adds some better keyboard accessibility

• Improved aria-labels and such for most focusable components
• Restricts user from focusing on other elements outside the modal when in full screen mode via modal-focus-context.jsx
• Makes the popover on direction input disappear on unfocus via custom hook useFocusTrap.js

Does not include

• Popover navigation with arrows for direction input
• "Choose a Sprite/Backdrop" menu navigation with keyboard.

Reason for Changes

Part of accessibility initiative for Scratch.

[kbangelov]

I am actually leaning more towards replacing this with inputRef or more generically openingElementRef that we apply to the input or whatever element activates the popover instead of the container including them both.

[redspacecat]

keep in mind that it seems some of the recent changes relating to navigation are causing issues

[adzhindzhi]

Hmm, I like the idea of trapping focus inside the stage when it's in full screen, but making every single element have tabIndex=-1 feels a bit hacky to me... I've been thinking about other ways to achieve the same behavior (I am basically thinking out loud here, so sorry if my thoughts are a bit messy).

The main issue is that this isn't a real modal, but we want to treat it like one. I don't see using an actual dialog as a viable option though. But maybe instead of manually changing the tabIndex, we could make some sort of a trapFocus/restrictFocus helper that targets a specific container, listens to keydown events, and:

• Collects a list of focusable items within the container (we could potentially reuse your existing data-focusable prop)
• If focus is outside that list, move it back to the first focusable element
• If the last focusable element is focused and Tab is pressed, loop focus back to the first
• If the first element is focused and Shift + Tab is pressed, move focus to the last

I'm fine with using ModalFocusProvider for now and handling this logic here, but generally it might make sense to extract it on its own at some point, as I can see this potentially being used in other places as well, which are not necessarily modals. The benefits I see in this approach are that if you have multiple containers or areas which can trap focus, you can "specify" the target region, in which the focus is trapped. Right now, if any other element outside the Stage has data-focusable as a prop, we'd still be able to navigate it even when in fullScreen.

What do you think? I'd like to here @KManolov3's thoughts as well. There's something similar (although not entirely) implemented here, which I think could be helpful.

[kbangelov]

I agree about the flaw of having other elements with the same prop. I missed that. Besides that my code seems to match most of your ideas, except the way we make them unfocusable by tab via tabIndex=-1 which does feel just a little bit hacky. I agree with that too.

My suggestion for the first issue is to add a param to data-focusable so different sections don't get mixed up and call the context with that param. Making this into a hook, instead of context should be considered, then using the container of the items to look for all the direct focusables that should remain. I'm even leaning more in that direction right now.

The tabIndex logic itself doesn't feel terribly wrong to me since that's the prop is responsible for the focus of elements. I am struggling to picture something different right now. But I'll also wait to hear @KManolov3 on this one.

[KManolov3]

A general thought I'm having with accessibility related logic is to try to keep it as simple as possible due to both maintenance and testing concerns.

With that in mind, I think a trapFocus approach, where you have a hook to which you pass a given parent element and a data prop name makes sense to me. The hook can return a trapFocus() and releaseFocus() methods. trapFocus() would then add event listeners which listen for Tab events and iterate over the children of the parent element which have the specified data prop name.

That should be fairly concise in terms of implementation and it'd allow us to avoid setting a prop for all elements.

What do you think?

[kbangelov]

I'll give it a shot.

[KManolov3]

A general thought I'm having with accessibility related logic is to try to keep it as simple as possible due to both maintenance and testing concerns.

With that in mind, I think a trapFocus approach, where you have a hook to which you pass a given parent element and a data prop name makes sense to me. The hook can return a trapFocus() and releaseFocus() methods. trapFocus() would then add event listeners which listen for Tab events and iterate over the children of the parent element which have the specified data prop name.

That should be fairly concise in terms of implementation and it'd allow us to avoid setting a prop for all elements.

What do you think?

[adzhindzhi]

Instead of doing this in a useEffect, can we call trapFocus/releaseFocus when the full screen button gets clicked? I think it's a bit more explicit, and we don't risk triggering it more times than we actually need to?

[kbangelov]

I mean clicking button changes the state and activates this just once, I think. I think it's safe to keep it this way. It seems more tied to the state rather than a click.

[adzhindzhi]

Well, not exactly. For example, when you are in embed mode, scratch-www renders the editor with isFullScreen=true, which will trap the focus, and never release it. However, we wouldn't want to trap the focus when we're in embed mode, because we'd prevent any other elements in the document from being navigable.

[Copilot]

Pull request overview

This PR improves keyboard accessibility in Scratch GUI, primarily around fullscreen stage controls and the direction picker popover, by adding focus management and clearer accessible names for key controls.

Changes:

• Introduces a useFocusTrap hook and applies it to the fullscreen stage header to keep tab focus within the fullscreen overlay.
• Adds localized aria-labels and fullscreen focus markers (data-focusable) to key controls (green flag, stop, fullscreen toggle).
• Updates Label to support refs (via forwardRef) and adds a focus-out behavior to close the direction picker popover.

Reviewed changes

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

Show a summary per file

File	Description
packages/scratch-gui/src/hooks/useFocusTrap.js	Adds a reusable focus-trap hook for tab navigation containment.
packages/scratch-gui/src/components/stage-header/stage-header.jsx	Applies focus trapping in fullscreen and improves aria-labels / focusability markings.
packages/scratch-gui/src/containers/controls.jsx	Plumbs isFullScreen down into the controls UI.
packages/scratch-gui/src/components/controls/controls.jsx	Passes isFullScreen to Green Flag / Stop All so they can participate in focus trapping.
packages/scratch-gui/src/components/green-flag/green-flag.jsx	Adds localized aria-label and fullscreen focus marker behavior.
packages/scratch-gui/src/components/stop-all/stop-all.jsx	Adds localized aria-label, fullscreen focus marker behavior, and disables when inactive.
packages/scratch-gui/src/components/forms/label.jsx	Converts Label to forwardRef to support focus containment logic.
packages/scratch-gui/src/components/direction-picker/direction-picker.jsx	Adds a document focus listener to close the popover on focus leaving the picker/popover.
packages/scratch-gui/src/components/box/box.jsx	Contains a small whitespace-only change.

---

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

[adzhindzhi]

When will offsetParent be null?

[kbangelov]

It's more of an edge case but if an element is invisible due to stylings, such as display : none, it should be filtered out by this check. I wonder whether to leave it, to be honest.

[adzhindzhi]

If an element is invisible, it probably shouldn't have the data-focusable attribute. As far as I can see, offsetParent can also be null when the element has position: fixed (see MDN).

I think it's reasonable to leave handling of hidden or fixed elements to the consumer of the hook. They may want those elements to remain focusable, or they may not. That doesn't feel like a general concern this hook needs to address... 🤔

[adzhindzhi]

Looks great! 🙌 I really like how this turned out!