[UEPR-516] Debug modal accessibility

[kbangelov]

Resolves

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

Proposed Changes

• Added keyboard navigation for the modal slides
• Made next slide and previous slide buttons focusable

Reason for Changes

Part of Accessibility initiative for Scratch.

[kbangelov]

Also, the close button's focus doesn't look the prettiest. Its behavior was actually not changed at all in this PR, but maybe it's still worth fixing in terms of styling?

[Copilot]

Pull request overview

This PR enhances keyboard accessibility for the debug modal component by adding arrow key navigation between slides and making navigation buttons keyboard-focusable. The changes support Scratch's accessibility initiative to ensure users can navigate the debugging modal using only the keyboard.

Changes:

• Added keyboard navigation with arrow keys to cycle through modal slides
• Made previous and next navigation buttons focusable and keyboard-activatable
• Added refs to slide items for programmatic focus management

Comments suppressed due to low confidence (7)

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:158

• The slide items with role="button" are missing keyboard event handlers. While arrow key navigation is handled globally, these elements should also respond to Enter and Space keys for activation when focused. Add onKeyDown handler to trigger handleTopicSelect when Enter or Space is pressed.

                            tabIndex={-1}
                            role="button"
                            ref={slideRefs[index]}

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:200

• Navigation buttons with role="button" should have aria-label attributes to provide accessible names for screen reader users. The alt attribute on img elements is not sufficient when the img is a child of an interactive element with role="button". Add aria-label="Previous" to the previous button.

                            tabIndex={0}
                            role="button"
                            onKeyDown={handleKeyDownPrevious}
                        />

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:94

• The global keydown event listener on window will intercept arrow key events from all elements, potentially interfering with other interactive elements within the modal (e.g., text inputs, scrollable areas). Consider checking event.target to ensure the event is not from an input field or other interactive element, or scope the event listener to a specific element within the modal.

    useEffect(() => {
        if (!isOpen) return;

        window.addEventListener('keydown', handleKeyDownSlides);
        return () => window.removeEventListener('keydown', handleKeyDownSlides);
    }, [isOpen, handleKeyDownSlides]);

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:100

• Elements with role="button" should respond to both Enter and Space keys for full keyboard accessibility. Currently, only Enter key is handled. Add handling for KEY.SPACE as well to match standard button behavior.

    const handleKeyDownPrevious = useCallback(e => {
        if (e.key === KEY.ENTER) {
            handlePrevious();
        }
    }, [handlePrevious]);

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:106

• Elements with role="button" should respond to both Enter and Space keys for full keyboard accessibility. Currently, only Enter key is handled. Add handling for KEY.SPACE as well to match standard button behavior.

    const handleKeyDownNext = useCallback(e => {
        if (e.key === KEY.ENTER) {
            handleNext();
        }
    }, [handleNext]);

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:211

• Navigation buttons with role="button" should have aria-label attributes to provide accessible names for screen reader users. The alt attribute on img elements is not sufficient when the img is a child of an interactive element with role="button". Add aria-label="Next" to the next button.

                            tabIndex={0}
                            role="button"
                            onKeyDown={handleKeyDownNext}
                        />

packages/scratch-gui/src/components/debug-modal/debug-modal.jsx:156

• Setting tabIndex={-1} on elements with role="button" removes them from the keyboard tab order, which defeats the purpose of adding keyboard accessibility. The slide items should be focusable via keyboard navigation. Consider using tabIndex={0} to make them keyboard accessible, or manage focus programmatically if using a roving tabindex pattern.

                            tabIndex={-1}

---

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

[KManolov3]

Do we really need to specify this explicitly? If we used a <button wouldn't ENTER and SPACE work out of the box?

[kbangelov]

Enter, yes. Space, no. So i'll just keep Space for now.

[KManolov3]

We should've removed these logs a while ago... as they are currently, they just add bloat to Google Analytics. It's fine to leave it as-is for now.

[KManolov3]

Shouldn't this eventListener also be removed when isOpen becomes false?

[kbangelov]

seems identical with the condition of unmounting to me?

[KManolov3]

In gui.jsx DebugModal is always being rendered - whether it's content is shown however is being controlled from outside by the isOpen flag. So we won't go into the unmounting logic which removes the event listener.
Feel free to test it locally, and see whether we end up with multiple registered event listeners.

[KManolov3]

Why is this needed?

[KManolov3]

In gui.jsx DebugModal is always being rendered - whether it's content is shown however is being controlled from outside by the isOpen flag. So we won't go into the unmounting logic which removes the event listener.
Feel free to test it locally, and see whether we end up with multiple registered event listeners.

[KManolov3]

Let's make this a button as well