Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31ddfd06c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
caio-pizzol
left a comment
There was a problem hiding this comment.
@palmer-cl makes sense to widen the scan past single-char runs, but removing the bound entirely can delete into a previous paragraph outside of TC mode. left two inline comments — main suggestion is using paragraph start as the bound instead of no bound.
| const findPreviousTextDeleteRange = (doc, cursorPos, minPos) => { | ||
| for (let pos = cursorPos - 1; pos >= minPos; pos -= 1) { | ||
| const findPreviousTextDeleteRange = (doc, cursorPos) => { | ||
| for (let pos = cursorPos - 1; pos > 0; pos -= 1) { |
There was a problem hiding this comment.
without a lower bound, this scan can walk past paragraph boundaries and delete a character from the wrong paragraph. that only gets caught by normalizeReplaceStepSingleCharDelete in track-changes mode — in normal editing it goes through silently.
instead of removing minPos entirely, scope it to the current paragraph:
const paragraphStart = $pos.start($pos.depth);
const deleteRange = findPreviousTextDeleteRange(state.doc, $pos.pos, paragraphStart);that still lets you cross sibling runs (fixing the single-char run issue) without leaving the paragraph.
There was a problem hiding this comment.
the reason $pos.start($pos.depth) didn't work is that $pos.depth points to different levels depending on cursor position:
- between runs (case 1:
$pos.nodeBeforeis a run) →$pos.depthis the paragraph depth →$pos.start($pos.depth)= paragraph start ✓ - at the start of a run (case 2:
$pos.pos === $pos.start()) →$pos.depthis the run's depth →$pos.start($pos.depth)= run content start ✗
so in case 2 the bound collapses back to the run, which is why backspace still couldn't cross into the previous run in your video.
we already solve this exact problem in backspaceAcrossRuns.js:32 — just need to compute paraDepth the same way:
const paragraphDepth = $pos.parent.type === runType ? $pos.depth - 1 : $pos.depth;
const paragraphStart = $pos.start(paragraphDepth);
const deleteRange = findPreviousTextDeleteRange(state.doc, $pos.pos, paragraphStart);that gives paragraph-scoped bounds for both cursor positions — crosses sibling runs but not paragraph boundaries.
check: #2225
| @@ -109,27 +109,4 @@ describe('backspaceNextToRun', () => { | |||
| }); | |||
There was a problem hiding this comment.
this test covers the cross-paragraph case — with a paragraph-scoped bound it should still pass. worth keeping.
|
Closing this PR this is no longer an issue in Firefox post merging main, it looks like |


This is related to: #2175