[UEPR-516] Debug modal accessibility by kbangelov · Pull Request #450 · scratchfoundation/scratch-editor

kbangelov · 2026-02-19T11:08:18Z

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?

kbangelov · 2026-02-19T11:09:49Z

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

+
+            let nextIndex;
+            if (e.key === KEY.ARROW_LEFT || e.key === KEY.ARROW_UP) {
+                nextIndex = selectedTopicIndex > 0 ? selectedTopicIndex - 1 : sections.length - 1;


It is a bit 50/50 to me whether we should add the loop logic. On one hand, we could. On the other, the buttons so far don't suggest looping. But that's also a thing we can change if we wanted to.

Good question, I guess we can run it by Tereza from a UX perspective. I also wonder if ARROW_LEFT/ARROW_RIGHT should change the selection. Probably yes, it seems convenient?

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.

Copilot · 2026-02-19T11:15:31Z

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

+            setSelectedTopicIndex(nextIndex);
+            logTopicChange(nextIndex);
+        }
+    }, [slideRefs]);


The dependency array for handleKeyDownSlides is missing selectedTopicIndex. This will cause the callback to use stale values of selectedTopicIndex, leading to incorrect navigation behavior. Add selectedTopicIndex to the dependency array.

Suggested change

}, [slideRefs]);

}, [slideRefs, selectedTopicIndex, setSelectedTopicIndex]);

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

adzhindzhi · 2026-02-20T10:08:42Z

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

+
+            let nextIndex;
+            if (e.key === KEY.ARROW_LEFT || e.key === KEY.ARROW_UP) {
+                nextIndex = selectedTopicIndex > 0 ? selectedTopicIndex - 1 : sections.length - 1;


Good question, I guess we can run it by Tereza from a UX perspective. I also wonder if ARROW_LEFT/ARROW_RIGHT should change the selection. Probably yes, it seems convenient?

adzhindzhi · 2026-02-20T10:09:06Z

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

+            setSelectedTopicIndex(nextIndex);
+            logTopicChange(nextIndex);
+        }
+    }, [selectedTopicIndex]);


Should setSelectedTopicIndex also be part of the deps array?

adzhindzhi · 2026-02-20T10:27:01Z

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

+    useEffect(() => {
+        if (!isOpen) return;
+
+        window.addEventListener('keydown', handleKeyDownSlides);
+        return () => window.removeEventListener('keydown', handleKeyDownSlides);
+    }, [isOpen, handleKeyDownSlides]);


What I find slightly unfortunate about the current implementation is that the event listener will be re-attached on every topic change, rather than attaching it once when the dialog opens. And the reason that would happen is because in handleKeyDownSlides we rely on the selectedTopicIndex. Maybe we could do something like:

setSelectedTopicIndex(prev => { let nextIndex; if (e.key === KEY.ARROW_LEFT || e.key === KEY.ARROW_UP) { // logic here based on prev logTopicChange(nextIndex); });

for handleKeyDownSlides, to avoid the selectedTopicIndex dependency and avoid re-attaching the event listener?

adzhindzhi · 2026-02-20T10:32:34Z

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

                            })}
                            // eslint-disable-next-line react/jsx-no-bind
                            onClick={() => handleTopicSelect(index)}
+                            tabIndex={-1}


Do you need this? This is un-focusable through Tab navigation by default, isn't it?

adzhindzhi · 2026-02-20T10:42:57Z

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

+    useEffect(() => {
+        if (!isOpen) return;
+
+        window.addEventListener('keydown', handleKeyDownSlides);


nitpick: I wonder if we should attach the event listener to the modal element (or at least document) to make it more DOM-scoped.

feat: added keyboard movement logic inside debug modal

13d9220

kbangelov requested a review from a team as a code owner February 19, 2026 11:08

kbangelov commented Feb 19, 2026

View reviewed changes

kbangelov requested review from adzhindzhi and Copilot February 19, 2026 11:11

Copilot started reviewing on behalf of kbangelov February 19, 2026 11:12 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

chore: copilot comment fix

45be8d1

adzhindzhi reviewed Feb 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[UEPR-516] Debug modal accessibility#450

[UEPR-516] Debug modal accessibility#450
kbangelov wants to merge 2 commits intoscratchfoundation:release/UEPR-297-accessibility-improvementsfrom
kbangelov:task/uepr-516-debug-modal-accessibility

kbangelov commented Feb 19, 2026

Uh oh!

kbangelov left a comment

Uh oh!

kbangelov Feb 19, 2026

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

adzhindzhi Feb 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	}, [slideRefs]);
	}, [slideRefs, selectedTopicIndex, setSelectedTopicIndex]);

Comments

Conversation

kbangelov commented Feb 19, 2026

Resolves

Proposed Changes

Reason for Changes

Uh oh!

kbangelov left a comment

Choose a reason for hiding this comment

Uh oh!

kbangelov Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

adzhindzhi Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants