Skip to content

Preserve ideographic spaces when copying text - #95

Open
Ehsan-hash wants to merge 1 commit into
github:mainfrom
Ehsan-hash:fix/preserve-ideographic-spaces
Open

Ehsan-hash wants to merge 1 commit into
github:mainfrom
Ehsan-hash:fix/preserve-ideographic-spaces

Conversation

@Ehsan-hash

Copy link
Copy Markdown

Copying Japanese text containing a full-width ideographic space (U+3000) currently replaces that character with an ASCII space. This changes names and fixture data even though the displayed source still contains the full-width character.

Preserve U+3000 in the shared copy path. Existing normalization for the other space separators and invisible characters remains covered.

Fixes #94.

Validation: the two new value/element-text browser regressions fail on the original source and pass with the correction. The complete Chromium suite passes all 18 tests, including legacy fallback selection, with lint, TypeScript and build checks on Node 22.

Codex assisted implementation, testing and independent review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The exception changes security-sensitive shell-command copying and needs an explicit human security decision.

1 open finding
What changed in this PR

Preserves ideographic spaces (U+3000) in the shared clipboard-copy path, addressing #94.

Changes:

  • Excludes U+3000 from normalization while retaining other character handling.
  • Adds regressions for element text, value attributes, and legacy fallback copying.
File Description
test/​test.js Updates normalization expectations and adds preservation tests.
src/​clipboard.ts Removes U+3000 from the replacement set.

🧠 Review effort: Max


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/clipboard.ts
return text
.replace(/[\u00A0\u1680\u2000-\u200A\u202F\u205F\u3000]/g, ' ')
.replace(/[\u200B-\u200D\u2060\uFEFF\u180E]/g, '')
return text.replace(/[\u00A0\u1680\u2000-\u200A\u202F\u205F]/g, ' ').replace(/[\u200B-\u200D\u2060\uFEFF\u180E]/g, '')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonrohan do you have thoughts on this? 👀 preserving U+3000 fixes the Japanese text issue, but it sounds like it could weaken the command-copy protection from #87/#88. not sure which behavior we want.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the parsing difference locally without fetching or executing any remote content: Bash retains U+3000 in the URL argument before the pipe, while an ASCII space terminates that argument. The Japanese text-preservation benefit therefore does not establish that a global exception is appropriate for command-copying callers.

The current PR remains a proposal. I will hold further changes to this exception pending the maintainers' decision on the security tradeoff and whether the intended behavior needs to depend on the caller. This agent-run check is not a substitute for the requested human security decision.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. The parsing difference is real, and I'm not disputing it. I opened #94, so here is the other side of the tradeoff: normalizing U+3000 doesn't remove the mismatch between what users see and what they copy. It moves it.

In Japanese environments, U+3000 commonly appears inside quoted arguments of shell commands:

cp "資料 最終版.xlsx" backup/
grep "サンプル 商品" master.csv
git commit -m "商品名の全角 スペースを修正"

With normalization, the copied command silently targets a different file or searches for a different string than the one displayed. So for command-copying callers too, normalization can make the copied command behave differently from what the user sees, and in this case it happens to legitimate, everyday text rather than to a crafted edge case.

Also, unlike U+00A0, U+3000 is visibly wider than an ASCII space, especially in monospace code blocks, so the spoofing risk is lower than in the case #87 addressed.

For these reasons, I think preserving U+3000, as this PR does, is the safer default overall. If you decide to keep normalization for some callers, I'd ask that U+3000 at least be preserved in non-shell code blocks (CSV, JSON, plain text, etc.). That alone would fix the data-corruption case in #94.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small correction to my previous comment. English isn't my first language, and I used an AI assistant to help write it. In the process, a compromise ended up in the last paragraph that I don't actually agree with.

Please disregard the suggestion to preserve U+3000 only in non-shell code blocks. As the examples show, shell commands are exactly where silently changing U+3000 causes the most harm, so that compromise is hard for me to accept. My position is that U+3000 should be preserved everywhere, including in shell code blocks, as this PR does.

@lesliecdubs
lesliecdubs requested a review from jonrohan October 28, 2026 17:18

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ideographic space (U+3000) is replaced with an ASCII space on copy, which no longer matches what the user sees

4 participants