Skip to content

fix: P1 issue 対応 (#392 PNG ダウンロード失敗の UI 通知 / #386 QrCode a11y) - #434

Merged
fumtas1k merged 4 commits into
developfrom
claude/fix-p1-issues-ep1N1
May 14, 2026
Merged

fix: P1 issue 対応 (#392 PNG ダウンロード失敗の UI 通知 / #386 QrCode a11y)#434
fumtas1k merged 4 commits into
developfrom
claude/fix-p1-issues-ep1N1

Conversation

@fumtas1k

Copy link
Copy Markdown
Owner

サマリー

P1 タグ open issue 7 件のうち、独立かつ単一 PR で完結できる以下 2 件をまとめて対応。

変更内容

#392: downloadPngFromSvgElement Promise 化

  • src/utils/download.ts — 戻り型 voidPromise<void>img.onerrorreject(new Error('PNG への変換に失敗しました'))img.onload の末尾で resolve()。並列実装 svgContentToPngBlob と同じ pattern。
  • src/components/tools/JanCode.tsxGs1Databar.downloadPng pattern を踏襲し downloadError state + await/try-catch + <ErrorMessage variant="block"> を配置。SVG ダウンロードも try/catch で包む。

#386: QrCode a11y

  • src/components/tools/QrCode.tsx — プレビューコンテナに role="status" aria-live="polite"generateQrSvg で生成 SVG に <title>QRコード: <text></title>role="img" / aria-label="QRコード" を注入。<title> 内テキストは XML エスケープして XSS 二次防衛線も兼ねる。

test-gates 陽性対照

両 issue ともガード/検知系として、旧実装に当てれば fail する設計のテストを併設:

対象 陽性対照
#392 unit Image.onerror 強制発火で Promise.reject('PNG への変換に失敗しました') を assert (src/utils/__tests__/download.test.ts)。旧 void 実装には await できず必ず fail。
#392 E2E page.evaluatewindow.Image を必ず onerror 発火する stub に差し替え、role="alert"ErrorMessage 表示を assert (tests/e2e/jan-code.spec.ts)。旧実装は silent failure で alert 出ず。
#386 E2E (title) <script>alert(1)</script> を入力 → containerHtml&lt;script&gt; が含まれ生 <script> タグが DOM に無いことを assert (tests/e2e/qr-code.spec.ts)。escape を外せば fail。

検証

  • node_modules/.bin/astro check → 0 errors / 0 warnings / 0 hints
  • npm run test → 1246 件 pass (build artifact 依存 meta テストのため事前に npm run build 実施)
  • npm run test:e2e -- jan-code.spec.ts qr-code.spec.ts a11y-live-region.spec.ts → 22 件 pass
  • npm run format:check → All files use Prettier code style

Test plan

  • CI green
  • JAN-13 ページで PNG ダウンロード正常動作(手動)
  • QR コードページで生成 SVG の <title> を DevTools で確認(手動)

Closes #392
Closes #386


Generated by Claude Code

claude added 2 commits May 14, 2026 04:20
旧実装は戻り型 void で img.onerror 時に何もせず silent failure。Promise
化して reject を `JanCode.downloadPng` の catch から ErrorMessage 表示に
接続する。並列実装 svgContentToPngBlob / downloadPngFromSvgContent と同じ
pattern (Gs1Databar の downloadPng 参考) で API 内部の不一致を解消する。

unit / E2E の陽性対照 (img.onerror 強制発火で reject / ErrorMessage 表示)
を追加。旧 void 実装に当てると await 不可 / ErrorMessage 非表示で fail
する設計とした (test-gates 鉄則: 検知能力ゼロで green を防ぐ)。

Closes #392
QR コードは dangerouslySetInnerHTML で SVG 挿入されるが、コンテナの
aria-live も SVG <title> も無く、スクリーンリーダーが生成完了・図の意味を
読み取れなかった。

- プレビューコンテナに role="status" aria-live="polite" を付与
  (Gs1Databar / JanCode の status 領域と同形)
- generateQrSvg に <title> 注入 + role="img" / aria-label を追加。
  text は XML エスケープして XSS 二次防衛線も兼ねる
- E2E 2 ケース追加: <title> テキスト assert と <script> 注入で実体参照化
  されることの陽性対照 (旧実装に当てれば fail する)

Closes #386
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

🖼️ Visual Regression Test 結果

  • Status: ✅ 全 40 件 pass
  • Workflow run: 25842241932
  • Artifact (diff 画像 / playwright-report): 上記 workflow run の Artifacts セクションから download

diff が 意図的な visual 変更の場合: Update Visual Regression Baseline workflow を本 PR ブランチで workflow_dispatch trigger して baseline を更新。
diff が 意図しない regression の場合: 該当変更を fix。
本 check は required ではないため fail のままでも merge は可能(reviewer 判断)。

@fumtas1k fumtas1k self-assigned this May 14, 2026

Copy link
Copy Markdown
Owner Author

レビュー (#434)

P1 issue 2 件 (#392 silent failure / #386 a11y) を 1 PR に同梱した修正。陽性対照を意識した unit / E2E が両方追加されていて、CLAUDE.md「ガード / バリデータには陽性対照必須」ルールにきれいに沿っている点が良い。Gs1Databar の既存 pattern との一貫性も保たれている。下記は主に a11y と Promise 経路の取りこぼしに関する指摘。


🔴 Should fix — <title> が SR に届かない可能性

src/components/tools/QrCode.tsx:40 で生成される SVG は

<svg ... role="img" aria-label="QRコード"><title>QRコード: https://example.com</title>...

になっている。ARIA 仕様上 aria-label が指定されると accessible name の計算で <title> は無視される (Accessible Name and Description Computation 4.3.1)。
つまり SR ユーザーには <title> 内の URL/テキストは読み上げられず「QRコード」だけになる。E2E では getByRole('img', { name: 'QRコード' }) が pass するため検知できていないが、issue #386 の本来の意図 (SR で内容が読み取れる) は半分しか満たせていない。

修正案 2 択:

// 案 A: aria-label を外して <title> を唯一の accessible name 源にする
return svg.replace(/<svg([^>]*)>/, `<svg$1 role="img">${title}`);

// 案 B: 内容を aria-label に直接入れて <title> は装飾扱いにする
const label = `QRコード: ${text}`; // escapeXml は不要 (属性値は React/DOM 側で処理)
return svg.replace(/<svg([^>]*)>/, `<svg$1 role="img" aria-label="${escapeAttr(label)}">${title}`);

E2E にも「accessible name に URL の一部が含まれること」の assert を追加すると陽性対照になります。


🟡 Consider — aria-live="polite" の発火頻度

QrCode.tsx:117role="status" aria-live="polite" を付けた親 div はテキスト入力 1 文字ごとに中の SVG が再生成され、その都度 live region が変化と判定されて SR がアナウンスを試みる (polite なのでキュー)。長い URL を打つと「QRコード QRコード QRコード…」と連呼される懸念がある。

useEffect 内で生成を 300〜500ms debounce するか、live region を role="status" のラッパでなく「生成完了直後だけ一時的に出す sr-only テキスト」に変える方がノイズが少ない (本 PR スコープ外なら issue 化推奨)。


🟡 Consider — img.onload 内例外は Promise reject に伝わらない

src/utils/download.ts:104-110:

img.onload = () => {
  ctx.drawImage(img, 0, 0);       // canvas tainted 時に SecurityError
  URL.revokeObjectURL(url);
  const a = document.createElement('a');
  a.href = canvas.toDataURL('image/png'); // tainted canvas で throw
  ...
  resolve();
};

drawImage / toDataURL がここで throw すると Promise には reject されず、ブラウザの unhandled error として出るだけで JanCodecatch に届かない (= silent failure 経路が一部残る)。JanCode.tsx:64 のコメント「canvas エラーで reject する」も現状の挙動とは齟齬がある。

img.onload = () => {
  try {
    ctx.drawImage(img, 0, 0);
    URL.revokeObjectURL(url);
    const a = document.createElement('a');
    a.href = canvas.toDataURL('image/png');
    a.download = filename;
    a.click();
    resolve();
  } catch (e) {
    URL.revokeObjectURL(url);
    reject(e instanceof Error ? e : new Error('PNG への変換に失敗しました'));
  }
};

これで JanCode 側コメントの記述とも整合し、陽性対照を toDataURL 失敗系でも書ける。


🟢 Nits

  • src/utils/__tests__/download.test.ts:67 の外側 imgInstance = { onload: null, onerror: null, src: '' } の初期化は Image の setter で即上書きされる dead code (動作には影響なし)。コメントで意図を残すか削除を検討。
  • JanCode.tsxdownloadSvg の try/catch は downloadBlobURL.createObjectURL で throw する稀ケースへの defense-in-depth として妥当。
  • escapeXml& を最初に処理する順序が正しく書かれており OK。

✅ 良かった点

  • 陰性 / 陽性対照を it() レベルで分離 (download.test.ts)。test-gates 鉄則 1 を満たすコメントも丁寧。
  • E2E で FailingImageaddInitScript ではなく page.evaluate 内で差し替える理由 (withProductionCspgoto 後に fn を呼ぶため) が説明されており、将来の retry/migrate でハマらない。
  • <title> 内に XSS 二次防衛として escapeXml を入れた点 (dangerouslySetInnerHTML の sink 前に escape) は本来の text が attacker-controlled ではないとはいえ defense-in-depth として正しい。

Should fix (aria-label 上書き) が解消されれば approve できます。


Generated by Claude Code

claude added 2 commits May 14, 2026 04:46
PR #434 レビュー指摘対応 (Should fix):
aria-label="QRコード" を併用していると ARIA Accessible Name and Description
Computation 4.3.1 で <title> が name 計算から除外され、URL 等の本文が
SR で読み上げられない。

- generateQrSvg から aria-label 注入を撤去し、role="img" のみ付与
- E2E を強化: aria-label 属性が存在しないこと / <title> が first child で
  あること / textContent に URL 本文が含まれることを 3 段 assert
  (旧 aria-label="QRコード" 実装に当てると属性存在チェックで fail する陽性対照)
… に伝播

PR #434 レビュー指摘対応:
img.onload 内で drawImage / toDataURL が throw すると (canvas tainted で
SecurityError 等) Promise には reject されず unhandled error として漏れ、
caller の catch に届かない silent failure 経路が残っていた。

- try/catch で onload 全体を包み、catch で reject、finally で
  URL.revokeObjectURL を呼ぶことでリーク回避も両立
- 陽性対照 unit を追加: canvas.toDataURL を throw する stub で
  Promise が reject されることを assert (try/catch なし実装には
  unhandled error として漏れて fail する設計)
- imgInstance のデフォルト初期化を dead code として削除し、コメントで意図を残す

Copy link
Copy Markdown
Owner Author

レビューありがとうございます。Should fix と Consider をそれぞれ対応しました。

🔴 #1 <title> が SR に届かない → 対応 (commit 60fe880)

案 A を採用し generateQrSvg から aria-label 注入を撤去、role="img" のみ付与。<title> を唯一の accessible name 源にしました。
E2E は陽性対照を 3 段に強化:

  • aria-label 属性が存在しないこと (not.toHaveAttribute('aria-label', /.*/))
  • <title> が SVG の first child であること
  • textContent に URL 本文が含まれること

aria-label="QRコード" 実装に当てると 1 段目で fail する設計です。

🟡 #3 img.onload 内例外が reject に届かない → 対応 (commit 439893f)

ご指摘の通り drawImage / toDataURL の throw が silent に漏れる経路が残っていました。onload 全体を try/catch で包み、catch で reject、URL.revokeObjectURL は finally に移動してリーク回避も両立しました。
unit に canvas.toDataURL を throw する stub で reject を assert する陽性対照を追加。

🟡 #2 aria-live 連呼問題 → スコープ外として issue 化

#435 で起票しました。debounce で 300〜500ms 待つ案 A と sr-only テキスト分離の案 B を併記しています。

🟢 #4 imgInstance の dead code → 対応 (commit 439893f)

デフォルト初期化を削除し、「Image の src setter で確定される」旨のコメントを残しました。

検証

  • node_modules/.bin/astro check → 0 errors
  • npm run test → 1247 件 pass
  • npm run test:e2e -- qr-code.spec.ts jan-code.spec.ts a11y-live-region.spec.ts → 22 件 pass
  • npm run format:check → All files use Prettier code style

Generated by Claude Code

Copy link
Copy Markdown
Owner Author

再レビュー (#434, sha 439893f)

前回指摘の 🔴 / 🟡 主要 3 件すべて、修正方針も含めて適切に解消されています。Approve 可 という所感です。


✅ 解消確認

🔴 → ✅ aria-label<title> を上書きする問題

  • src/components/tools/QrCode.tsx:40 aria-label="QRコード" を削除し role="img" + first-child <title> のみで accessible name を提供する形に変更。
  • ソースコメントで「ARIA AccName 4.3.1 で aria-label があると <title> が name 計算から除外される」と根拠を明記。将来 a11y を「改善」したつもりで aria-label を復活させる regression に対するガードとして機能する。
  • E2E (tests/e2e/qr-code.spec.ts:90-93) に下記 3 つの陽性対照 assert が追加されており、retrieve に強い:
    • await expect(svgImg).not.toHaveAttribute('aria-label', /.*/) ← aria-label 復活で fail
    • firstElementChild?.tagName === 'title'<title> の位置が変わると fail
    • titleTexthttps://example.com を含む ← 内容が読まれない構造に変えたら fail

特に「aria-label が無いこと」を陽性対照化している点が良い。test-gates skill の鉄則 (検知能力ゼロで green の状態を作らない) に沿っている。

🟡 → ✅ img.onload 内例外が reject に伝播しない問題

src/utils/download.ts:94-110try / catch / finally 化:

img.onload = () => {
  try {
    ctx.drawImage(...);
    ...
    resolve();
  } catch (e) {
    reject(e instanceof Error ? e : new Error('PNG への変換に失敗しました'));
  } finally {
    URL.revokeObjectURL(url);
  }
};
  • finally で revoke することで drawImage throw 時の URL リークも閉じている。
  • instanceof Error 分岐で文字列 throw に対するメッセージ縮退も担保。
  • unit test (src/utils/__tests__/download.test.ts:151-175) に canvas.toDataURL が throw する陽性対照 it() を追加。旧 (try/catch なし) 実装に当てれば Promise reject されず timeout で必ず fail する設計で、検知能力が保証されている。

🟡 → ✅ unit test の dead code 初期化

imgInstance = { onload: null, ... } の初期化を削除し、コメントで「src setter で確定される」と意図を明記。残ったコードに一読で理解できる注釈が付いている。


🟢 残課題 (本 PR 外で OK)

  • role="status" aria-live="polite" の入力 keystroke 毎発火問題は本 PR では未対応。本来の issue [P1] fix(a11y): QrCode 生成結果に aria-live と SVG title を追加 #386 のスコープ外であることは妥当な判断ですが、debounce 化の独立 issue を切っておくと将来 a11y QA で再発見されたとき trace 可能になります。1 行 issue でも構いません。
  • docs/decisions.md への記録 (XSS 二次防衛 / <title> 上書き問題回避) は任意。今回のソースコメントが十分に詳しいので、必須ではないと考えます。

サマリ

観点 前回 今回
accessible name に URL が読まれる 🔴
Promise reject 経路の網羅 🟡
陽性対照テストの規約準拠 ✅ (さらに強化)
XSS 二次防衛
live region 発火頻度 🟡 🟢 (issue 化推奨、本 PR 外)

LGTM 🎉


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants