fix(a11y): keep invalid editor save focusable with aria-disabled - #398
fix(a11y): keep invalid editor save focusable with aria-disabled#398seonghobae wants to merge 17 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough편집기 저장 버튼이 Changes편집기 저장 접근성 처리
MSP XML 파싱
Node.js 및 패키지 버전 고정
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
seonghobae
changed the title
|
@jules Please make the submit guard flush the debounced draft validation before reading |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.jules/palette.md:
- Around line 118-120: Update .jules/palette.md lines 118-120 to remove the
claim that title is reliably announced by screen readers and require explicit
accessible descriptions such as aria-describedby; in app.js lines 1056-1061, set
aria-describedby="editor-errors" on the save button when validation errors exist
and remove it when they do not, while retaining title only as supplemental
guidance; in tests/e2e/editor-save-aria-disabled.spec.js lines 13-17, add an
assertion that the save button references editor-errors or exposes the
corresponding accessible description.
In `@app.js`:
- Around line 412-418: Update the submit handler around renderDraftValidation
and saveEditor so renderDraftValidation.flush() runs before checking the submit
button’s aria-disabled state or state.editor.errors; return without calling
saveEditor when the refreshed validation reports the draft is invalid. Add a
regression test covering deletion of a required value while validation rendering
is pending.
In `@cloud-sync.js`:
- Around line 744-766: Update the tag and collectBlocks helpers to recognize
XML-valid whitespace before the closing angle bracket in both opening and
closing tags, such as Task, UID, and their corresponding end tags, while
preserving exact element-name matching and incomplete-input handling. Add
regression coverage for these whitespace variants in task blocks and fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 18e6db79-f5a0-4484-ba9f-a6dea23c6132
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/fuzz.yml.github/workflows/server-tests.yml.jules/palette.mdapp.jscloud-sync.jspackage.jsontests/e2e/editor-save-aria-disabled.spec.jstests/unit/editor-submit-accessibility.test.mjstests/unit/msproject.test.mjs
| ## 2026-08-03 - Replace native disabled with aria-disabled for form submit buttons | ||
| **Learning:** Using the native `disabled` attribute on form submission buttons prevents them from receiving focus. This completely hides the button's `title` tooltip (which often explains *why* the form cannot be submitted) from keyboard-only and screen-reader users, leading to a confusing UX when validation fails. | ||
| **Action:** Use `aria-disabled="true"` instead of `disabled` for submit buttons when form validation fails. This ensures the button remains in the tab order so the `title` tooltip can be read. Since the browser no longer blocks the form submission natively, ensure the form's `submit` event listener explicitly checks `getAttribute('aria-disabled') === 'true'` and aborts if necessary. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
필수 오류 설명을 title만으로 제공하지 마세요.
title은 키보드 및 보조 기술에서 일관되게 노출되지 않습니다. 필수 안내는 명시적 접근성 설명으로 연결해야 합니다. (developer.mozilla.org)
.jules/palette.md#L118-L120:title이 스크린 리더에서 읽힌다는 설명을 제거하고, 필수 안내에는aria-describedby같은 명시적 연결을 사용하도록 수정하세요.app.js#L1056-L1061: 오류가 있으면 저장 버튼에aria-describedby="editor-errors"를 설정하세요. 오류가 없으면 이 속성을 제거하세요.title은 보조 안내로만 유지하세요.tests/e2e/editor-save-aria-disabled.spec.js#L13-L17:editor-errors에 대한aria-describedby또는 접근성 설명을 검증하세요.
📍 Affects 3 files
.jules/palette.md#L118-L120(this comment)app.js#L1056-L1061tests/e2e/editor-save-aria-disabled.spec.js#L13-L17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.jules/palette.md around lines 118 - 120, Update .jules/palette.md lines
118-120 to remove the claim that title is reliably announced by screen readers
and require explicit accessible descriptions such as aria-describedby; in app.js
lines 1056-1061, set aria-describedby="editor-errors" on the save button when
validation errors exist and remove it when they do not, while retaining title
only as supplemental guidance; in tests/e2e/editor-save-aria-disabled.spec.js
lines 13-17, add an assertion that the save button references editor-errors or
exposes the corresponding accessible description.
| const saveButton = form.querySelector('button[type="submit"]'); | ||
| if (saveButton && saveButton.getAttribute('aria-disabled') === 'true') { | ||
| return; | ||
| } | ||
|
|
||
| renderDraftValidation.flush(); | ||
| saveEditor(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
유효성 상태를 갱신한 후 제출을 차단하세요.
입력 이벤트의 유효성 렌더링이 대기 중이면 이 코드는 이전 aria-disabled 값을 검사합니다. 유효한 초안에서 필수 값을 지운 직후 제출하면 saveEditor()가 호출될 수 있습니다.
renderDraftValidation.flush()를 먼저 호출하세요. 그 후 갱신된 aria-disabled 또는 state.editor.errors를 검사하세요. 이 순서를 검증하는 회귀 테스트도 추가하세요.
수정 예시
event.preventDefault();
+ renderDraftValidation.flush();
const saveButton = form.querySelector('button[type="submit"]');
if (saveButton && saveButton.getAttribute('aria-disabled') === 'true') {
return;
}
- renderDraftValidation.flush();
saveEditor();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.js` around lines 412 - 418, Update the submit handler around
renderDraftValidation and saveEditor so renderDraftValidation.flush() runs
before checking the submit button’s aria-disabled state or state.editor.errors;
return without calling saveEditor when the refreshed validation reports the
draft is invalid. Add a regression test covering deletion of a required value
while validation rendering is pending.
| const tag = (block, name) => { | ||
| const m = block.match(new RegExp(`<${name}>([^<]*)</${name}>`)); | ||
| return m ? m[1].trim() : ''; | ||
| const openingTag = `<${name}>`; | ||
| const closingTag = `</${name}>`; | ||
| const valueStart = block.indexOf(openingTag); | ||
| if (valueStart === -1) return ''; | ||
| const contentStart = valueStart + openingTag.length; | ||
| const valueEnd = block.indexOf(closingTag, contentStart); | ||
| return valueEnd === -1 ? '' : block.slice(contentStart, valueEnd).trim(); | ||
| }; | ||
| const collectBlocks = (source, openTag, closeTag) => { | ||
| const out = []; | ||
| let from = 0; | ||
| for (;;) { | ||
| const start = source.indexOf(openTag, from); | ||
| if (start === -1) break; | ||
| const contentStart = start + openTag.length; | ||
| const end = source.indexOf(closeTag, contentStart); | ||
| // Incomplete open tag: stop linearly (do not rescan the remainder). | ||
| if (end === -1) break; | ||
| out.push(source.slice(start, end + closeTag.length)); | ||
| from = end + closeTag.length; | ||
| } | ||
| return out; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
유효한 XML 태그 공백을 처리하세요.
tag와 collectBlocks는 <Task> 및 </Task>처럼 정확히 일치하는 문자열만 찾습니다. XML은 <Task >, <UID >, </Task >와 같은 공백 표현을 허용합니다. 이 형식의 MSP XML은 작업 블록 또는 필드를 찾지 못해서 가져오기에 실패합니다.
XML 구조를 처리하는 파서로 변경하거나, 시작 및 종료 태그를 XML 구문에 맞게 토큰화하세요. 이 태그 공백 변형의 회귀 테스트도 추가하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-sync.js` around lines 744 - 766, Update the tag and collectBlocks
helpers to recognize XML-valid whitespace before the closing angle bracket in
both opening and closing tags, such as Task, UID, and their corresponding end
tags, while preserving exact element-name matching and incomplete-input
handling. Add regression coverage for these whitespace variants in task blocks
and fields.
|
@jules Please address all current actionable review findings on this branch as one focused accessibility fix:
Run targeted unit/E2E tests and the full repository checks. |
|
Closing in favor of #411. The direction—keep invalid Save focusable with |
1 participant
Summary
disabledon the editor save button witharia-disabled="true"when validation fails, keeping the explanatory tooltip reachable by keyboard and assistive technology.aria-disabledonce the draft is valid and restores the normal save tooltip.Verification
tests/e2e/editor-save-aria-disabled.spec.jsOriginally created by Jules for accessibility hardening.
Summary by CodeRabbit
접근성 개선
버그 수정
테스트
개발 환경