Hparsons accessibility - #1473
Conversation
f1b4b00 to
b625c02
Compare
|
Ooops, was based off another branch. Just rebased off of main. |
There was a problem hiding this comment.
🟡 Changes recommended
The new keyboard movement mode can leave _activeBlock null (and aria-activedescendant stale) while still in role="application", causing runtime errors and incorrect accessibility state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves accessibility and keyboard navigation for the HParsons (micro-parsons) interactive and fixes an image-related drag-and-drop edge case, with accompanying tests.
Changes:
- Adds persistent screen-reader feedback announcements (live region) and refreshes per-block ARIA state after grading/clearing/reset.
- Reworks ParsonsInput to use a single Tab stop with an “activation” movement mode (Enter/Space) and updated ARIA labeling/selection semantics.
- Ensures MathJax-rendered content does not introduce tab stops by disabling tabindex and observing late DOM mutations; prevents images inside draggable premises from becoming the drag target.
File summaries
| File | Description |
|---|---|
| bases/rsptx/interactives/runestone/hparsons/test/hparsons.test.js | Adds tests for MathJax tab-stop removal, live region feedback announcements, and keyboard movement mode behavior. |
| bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts | Introduces unified keyboard movement mode, active-block tracking, and richer per-block ARIA updates. |
| bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/micro-parsons.ts | Exposes refreshBlockAria() on the custom element API. |
| bases/rsptx/interactives/runestone/hparsons/js/hparsons.js | Adds MathJax tabindex cleanup + MutationObserver and makes math rendering return a Promise. |
| bases/rsptx/interactives/runestone/hparsons/js/BlockFeedback.js | Adds a persistent live region for grading/reset announcements and refreshes block ARIA after feedback changes. |
| bases/rsptx/interactives/runestone/dragndrop/test/dragndrop.test.js | Adds a regression test ensuring dragging an image uses the enclosing premise. |
| bases/rsptx/interactives/runestone/dragndrop/js/dragndrop.js | Disables image draggability within premises and uses currentTarget for correct drag/drop element IDs. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( | ||
| this.el.getAttribute("role") === "application" && | ||
| this._activeBlock | ||
| ) { | ||
| this.el.setAttribute("aria-activedescendant", this._activeBlock.id); | ||
| } |
| const activeBlock = this._activeBlock as HTMLDivElement; | ||
| const currentArea = | ||
| activeBlock.parentElement === this._dragArea | ||
| ? this._dragArea | ||
| : this._dropArea; |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed runtime/behavioral issues (stale live-region announcements via uncanceled timeouts, misleading Promise semantics in renderMathInBlocks(), and a potential null dereference in keyboard movement mode) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:556
- In keyboard movement mode, the keydown handler assumes
_activeBlockis always non-null (as HTMLDivElement). But_updateBlockAriacan set_activeBlock = nullif the active block is removed/re-rendered while role is stillapplication, which would cause a runtime error on the next key press when accessingactiveBlock.parentElement. Add a null/containment guard and exit movement mode (or pick a fallback block) when_activeBlockis missing.
const activeBlock = this._activeBlock as HTMLDivElement;
const currentArea =
activeBlock.parentElement === this._dragArea
? this._dragArea
: this._dropArea;
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| announceFeedback(message = this.messageDiv.textContent.trim()) { | ||
| this.feedbackLiveRegion.textContent = ""; | ||
| setTimeout(() => { | ||
| this.feedbackLiveRegion.textContent = message; | ||
| }, 10); |
There was a problem hiding this comment.
Addressed in followup commit
| setTimeout(() => { | ||
| const blocks = this.hparsonsInput.querySelectorAll( | ||
| ".parsons-block", | ||
| ); | ||
| blocks.forEach((block) => { | ||
| block.innerHTML = this.decodeHTMLEntities(block.innerHTML); | ||
| if (block.innerHTML.indexOf("process-math") !== -1) { | ||
| block.innerHTML = block.innerHTML.replace( | ||
| /<span class="process-math">|<\/span>/g, | ||
| "", | ||
| ); | ||
| } | ||
| this.queueMathJax(block); | ||
| }); | ||
| disableMathJaxTabStops(this.hparsonsInput, [".parsons-block"]); | ||
| resolve(); | ||
| }, 10); | ||
| }); |
There was a problem hiding this comment.
Addressed in followup commit
There was a problem hiding this comment.
🟡 Changes recommended
The new keyboard movement mode has confirmed correctness issues (duplicate IDs from clones, potential runtime crash when _activeBlock is null, and focus/ARIA inconsistencies) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:430
- Blocks can be cloned (reusable mode / Sortable clone) and will carry over an existing
id, which can create duplicate IDs in the DOM. That breaksaria-activedescendanttargeting and violates the uniqueness requirement for element IDs. Update the ID assignment to also regenerate IDs whendocument.getElementById(block.id)refers to a different node (duplicate case).
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:466 aria-activedescendantis being maintained onthis.el, but_setActiveBlock()moves DOM focus onto the block itself (block.focus()). Thearia-activedescendantpattern is only honored when focus remains on the owning element, so screen readers may ignore the active-descendant updates. Consider either (a) keeping focus onthis.eland relying onaria-activedescendant, or (b) droppingaria-activedescendantand relying on the focused block.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:547- While in keyboard movement mode, pressing Escape exits the mode but leaves focus on the last focused block (tabindex=-1). That undermines the “single Tab stop” goal because focus can remain on a non-tabbable descendant. After handling Escape, move focus back to the keyboard surface element (
this.el).
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:556
this._activeBlockcan becomenull(e.g., if blocks are reset/removed while movement mode is active). The keydown handler then casts it toHTMLDivElementand immediately dereferencesparentElement, which will throw at runtime. Guard against a missing active block before using it.
const activeBlock = this._activeBlock as HTMLDivElement;
const currentArea =
activeBlock.parentElement === this._dragArea
? this._dragArea
: this._dropArea;
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:449
- In movement mode,
aria-activedescendantis only set when_activeBlockis truthy, but when_activeBlockbecomesnull(e.g., if the active block is removed), the previousaria-activedescendantvalue is left behind and becomes stale. Cleararia-activedescendantwhen there is no active block.
if (
this.el.getAttribute("role") === "application" &&
this._activeBlock
) {
this.el.setAttribute("aria-activedescendant", this._activeBlock.id);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed keyboard-mode robustness and activation issues in ParsonsInput (null active block handling, stale aria-activedescendant, and unreliable click activation) that can cause runtime errors or broken accessibility behavior.
Review details
Suppressed comments (6)
Previously missed (5) — in code that hasn't changed since the last review.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:435
aria-labelfor each block is built from_getTextFromBlock(...), which won’t pick up MathJax’s rendered speech text (e.g.,mjx-container[aria-label]). For math blocks this can result in empty/incorrect labels even though MathJax provides an accessible label. Prefer MathJax’saria-labelwhen present, with_getTextFromBlockas a fallback.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:450_updateBlockAriasetsaria-activedescendantwhen_activeBlockis present, but never clears it when_activeBlockbecomes null while the surface remains inrole="application"(e.g., after a reset removes the active block). This can leave a stalearia-activedescendantpointing at a non-existent element.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:531- The click-to-activate handler only triggers when
ev.target === this.el. Since the container has multiple child elements (tips/areas), clicks will usually target a child node and never activate keyboard movement, even though the surface hasrole="button". Consider activating on any click that’s not on a.parsons-blockinstead of requiring the click target to be the root element.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:556 - In keyboard movement mode, the keydown handler dereferences
_activeBlockvia a type cast without a null check. If_activeBlockis cleared (e.g., reset removes the focused block), arrow/enter handling will throw when readingactiveBlock.parentElement. Add a guard to re-establish an active block (or exit movement mode) when_activeBlockis missing.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:86 - The SR-only instructions say "Press Enter" but the key handler also allows Space (
ev.key === " ") to activate keyboard movement. Update the instructions to mention Space so screen reader users get accurate guidance.
This issue also appears on line 495 of the same file.
bases/rsptx/interactives/runestone/hparsons/js/micro-parsons/ParsonsInput.ts:496
- The SR-only instructions are reset on exit to "Press Enter...", but Space also activates movement mode. Keep the exit-state instructions consistent with the actual supported keys.
this._keyboardInstructions.textContent =
"Press Enter to move blocks with the keyboard.";
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Screenreader/keyboard navigation overhaul for hparsons
This pull request introduces significant accessibility improvements and keyboard navigation enhancements to the Parsons block interactive and drag-and-drop components. The main themes are improved ARIA support for screen readers, a new unified keyboard movement mode for arranging blocks, and fixes for image drag behavior. These changes make the interactive components more accessible and user-friendly, especially for users relying on keyboard navigation or assistive technology.
Accessibility and ARIA Improvements:
feedbackLiveRegion) toBlockFeedbackfor polite screen reader feedback, and ensured feedback is announced or cleared at appropriate times. [1] [2] [3] [4]ParsonsInput, including per-blockaria-labels,aria-selected, andaria-activedescendantmanagement, and live keyboard instructions. [1] [2]HParsonsto ensure MathJax elements do not disrupt tab order, and improved math block rendering accessibility. [1] [2]Keyboard Navigation Enhancements:
Supporting Methods and API:
refreshBlockAriamethod on the Parsons input and element to allow parent components to trigger ARIA updates as needed. [1] [2]These changes collectively make the Parsons and drag-and-drop interactives more robust, accessible, and maintainable.