入力欄・contenteditable要素でのキーボードテキスト選択時にポップアップメニューを起動する#313
Conversation
Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
|
@copilot ベースブランチをマージしてください。コンフリクトがあったら解消してください。 |
…nto copilot/add-popup-menu-on-text-selection
マージ完了しました（57a0c98）。コンフリクトはなく、637件のテストすべてパスしています。 |
| // Since Range.getBoundingClientRect() is not available for form controls, | ||
| // position the anchor at the bottom-right of the input/textarea element. | ||
| const rect = active.getBoundingClientRect() | ||
| setAnchor({ x: rect.right, y: rect.bottom }) |
There was a problem hiding this comment.
@copilot
activeElementを使用すると入力欄全体が対象となり、ポップアップされる位置がキャレット位置から離れるので使用性が低下します。
以下のコードを参考に、選択されているテキストの位置を取得して、それを元にsetAnchorを行うようにしてください。
There was a problem hiding this comment.
修正しました（8ee6f4b）。
dom/index.ts に getInputSelectionEndPoint() 関数を追加し、参照コードと同様のミラーdiv技法を使って selectionEnd の実際の視覚的座標を取得するようにしました。SelectAnchor.tsx の onKeyUp ではこの関数を使い、要素全体の右下ではなく選択終了文字のすぐ下にアンカーを設置します。
There was a problem hiding this comment.
contenteditable な要素のケースにも対応してください。
There was a problem hiding this comment.
対応しました（849988f4）。
dom/index.ts に getEditableSelectionEndPoint() を追加しました。window.getSelection() でRangeを取得し、選択終了位置に collapse(false) でcollapseして getBoundingClientRect() で座標を取得します。矩形が空の場合は一時的なzero-width spaceスパンで計測するフォールバックも実装しています。
SelectAnchor.tsx の onKeyUp も isEditable ケースを追加して対応しました。input/textarea と contenteditable の共通ロジックはまとめて重複も削減しています。
Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
マージ完了しました（849988f4）。コンフリクトはなく、637件のテストすべてパスしています。 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 20.89% 21.71% +0.81%
==========================================
Files 318 319 +1
Lines 29785 30141 +356
Branches 1439 1523 +84
==========================================
+ Hits 6224 6544 +320
- Misses 23561 23597 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
コードレビュー入力欄・contenteditable要素でのキーボードテキスト選択対応の実装を確認しました。全体的に良い実装ですが、いくつかの問題点と改善提案があります。 🐛 バグ・潜在的問題1. 逆方向選択（Shift+左矢印）でポップアップが逆端に表示される
const caretPos = element.selectionEnd // 常に大きい方の位置を使用
const caretPos =
element.selectionDirection === "backward"
? element.selectionStart ?? 0
: element.selectionEnd ?? 0
const endRange = range.cloneRange()
endRange.collapse(false) // 常にRangeの「後端」へ同様に、contenteditable で Shift+左矢印による逆方向選択では const endRange = document.createRange()
endRange.setStart(selection.focusNode!, selection.focusOffset)
endRange.setEnd(selection.focusNode!, selection.focusOffset)2. ライブDOM変更によるサイドエフェクトのリスク
const span = document.createElement("span")
span.textContent = "\u200B"
endRange.insertNode(span) // ← ライブDOMを変更
const spanRect = span.getBoundingClientRect()
span.remove()
3.
|
PR レビュー: キーボードテキスト選択によるポップアップ対応 (Input/contenteditable)概要このPRは
潜在的なバグ🔴 [High]
|
| 重大度 | 問題 | 場所 |
|---|---|---|
| 🔴 High | focusNode non-null アサーション — クラッシュリスク |
dom/index.ts:491-492 |
| 🟡 Medium | selectionEndPoint が null 時に古いポップアップが残留 |
SelectAnchor.tsx:196-200 |
| 🟡 Medium | findContentEditableRoot が contenteditable="inherit" を見落とし |
dom/index.ts:387-390 |
| 🟡 Medium | RTL 入力で座標計算が誤る (direction スタイル未コピー) |
dom/index.ts:316-330 |
| 🟢 Low | マジックナンバー 8 を定数化すべき |
SelectAnchor.tsx:197 |
| 🟢 Low | keyup ごとにリフロー発生 | dom/index.ts:312, 354-374 |
| 🟢 Low | findEquivalentNode の O(n²) パス探索 |
dom/index.ts:401-423 |
| ℹ️ Info | 新規コードのテストがほぼゼロ (約108行未テスト) | — |
全体として、機能の実装アプローチは適切で、ミラーdivテクニックやRange APIの活用は妥当な手法です。上記のHighおよびMediumの問題を修正すれば、安定した動作が期待できます。
コードレビューPR の目的（input/textarea・contenteditable 要素でのキーボードテキスト選択時にポップアップを起動）は明確で、実装方針も適切です。テストも豊富に追加されており、ほぼ良い状態です。以下に気になった点を挙げます。 🔴 潜在的なバグ1. cloneRange.setStart(cloneNode, focusOffset)
cloneRange.setEnd(cloneNode, focusOffset)
2. * @returns {Point | null} The bottom-left coordinates of the selection end position.実際に返す値は 🟡 設計・ロジックの懸念3. const stylesToCopy = [
...
"white-space", // ここでコピーされるが...
...
]
// 後で必ず上書きされる
if (element instanceof HTMLTextAreaElement) {
mirror.style.whiteSpace = "pre-wrap" // 上書き
} else {
mirror.style.whiteSpace = "nowrap" // 上書き
}
4. const mayChangeSelection =
e.shiftKey ||
(e.key === "a" && (e.ctrlKey || e.metaKey)) ||
e.key === "Meta" || // ← Ctrl/Cmd を単体で押して離しただけでも発火する
e.key === "Control"コメントに「macOS で Cmd+A の keyup が 'a' ではなく Meta のみ発火する場合がある」とありますが、Meta/Control を単体で押して離した場合も 5. マジックナンバー selectionEndPoint.y -= 8 // Adjust to match the mouse cursor selection.
6. current.getAttribute("contenteditable") === "true" ||
current.getAttribute("contenteditable") === ""
🟢 テスト品質7. フォールバック（ if (!selectionEndPoint) {
const rect = active.getBoundingClientRect()
selectionEndPoint = { x: rect.right, y: rect.bottom }
}
8. expect(mockGetSelectionText).toHaveBeenCalled()
expect(mockGetInputSelectionEndPoint).toHaveBeenCalled()関数が「呼ばれたこと」のみ検証しており、実際に ✅ 良い点
|
dom.ts→dom/index.ts:getSelectionText()を input/textarea に対応SelectAnchor.tsx:isInputOrTextareaをインポートし、キーボード選択時にアンカー位置を設定getInputSelectionEndPoint()を追加：ミラーdiv技法でselectionEnd位置の正確な座標を取得SelectAnchor.tsxのonKeyUpでgetInputSelectionEndPoint()を使い、選択終了文字近傍にアンカーを設定getEditableSelectionEndPoint()を追加：Range API で contenteditable 要素の選択終了位置を取得SelectAnchor.tsxのonKeyUpを contenteditable 要素にも対応するよう拡張Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.