feat: the problem of starting to input the placeholder without disappearing in the context of Chinese input method#457
feat: the problem of starting to input the placeholder without disappearing in the context of Chinese input method#457wuyiping0628 wants to merge 1 commit intodevfrom
Conversation
…earing in the context of Chinese input method
WalkthroughA constructor is added to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/fluent-editor/src/modules/custom-clipboard.ts (1)
28-35: Reconsider the else branch logic and add cleanup for the event listener.Two concerns with this implementation:
Else branch redundancy: The else branch executes when
innerText === '\n'(empty) OR whenql-blankis already absent. Settingthis.quill.options.placeholderon every input event in these cases seems unnecessary - the placeholder text doesn't need to be reassigned repeatedly.Memory leak risk: The event listener is never removed. If the editor instance is destroyed, the listener remains attached to the DOM element.
♻️ Suggested improvement
constructor(public quill: FluentEditor) { super(quill) // 解决 quill 组件在中文输入法的情景下开始输入placeholder不消失的问题 - this.quill.root.addEventListener('input', () => { + const handleInput = () => { if (this.quill.root.innerText !== '\n' && this.quill.root.classList.contains('ql-blank')) { this.quill.root.classList.toggle('ql-blank', false) } - else { - this.quill.options.placeholder = this.quill.getLangText('editor-placeholder') - } - }) + } + this.quill.root.addEventListener('input', handleInput) + + // Cleanup on editor destruction + this.quill.on('editor-cleanup' as any, () => { + this.quill.root.removeEventListener('input', handleInput) + }) }Could you clarify the purpose of the else branch? If it's intended to reset the placeholder text after the editor becomes empty again, this may conflict with Quill's built-in placeholder handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluent-editor/src/modules/custom-clipboard.ts` around lines 28 - 35, The input handler attached via this.quill.root.addEventListener redundantly reassigns this.quill.options.placeholder on every input and is never removed; change the listener to a named function that tracks state transitions (only set this.quill.options.placeholder when the editor transitions from non-empty to empty, i.e., when this.quill.root.innerText becomes '\n' and ql-blank is absent) instead of on every event, and store that handler so you can call this.quill.root.removeEventListener(...) in the editor cleanup path (e.g., a dispose/destroy method on the CustomClipboard or where the Quill instance is torn down) to avoid the memory leak; reference the existing symbols this.quill.root, this.quill.options.placeholder, this.quill.getLangText, classList.contains/classList.toggle and implement the removal in the corresponding teardown method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fluent-editor/src/modules/custom-clipboard.ts`:
- Around line 23-26: The constructor for your custom Clipboard module is
incorrect: remove the redundant "declare quill: FluentEditor" and change the
constructor signature to accept both (quill: FluentEditor, options?:
Partial<ClipboardOptions>) and forward options to the parent by calling
super(quill, options) so Quill-provided matchers and options aren't dropped;
also capture the event listener handler you add (store the reference) and remove
it during cleanup/destroy (e.g., on module teardown or override a destroy
method) to avoid the memory leak.
---
Nitpick comments:
In `@packages/fluent-editor/src/modules/custom-clipboard.ts`:
- Around line 28-35: The input handler attached via
this.quill.root.addEventListener redundantly reassigns
this.quill.options.placeholder on every input and is never removed; change the
listener to a named function that tracks state transitions (only set
this.quill.options.placeholder when the editor transitions from non-empty to
empty, i.e., when this.quill.root.innerText becomes '\n' and ql-blank is absent)
instead of on every event, and store that handler so you can call
this.quill.root.removeEventListener(...) in the editor cleanup path (e.g., a
dispose/destroy method on the CustomClipboard or where the Quill instance is
torn down) to avoid the memory leak; reference the existing symbols
this.quill.root, this.quill.options.placeholder, this.quill.getLangText,
classList.contains/classList.toggle and implement the removal in the
corresponding teardown method.
🪄 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
Run ID: 1c74abfa-1865-4833-ac1a-9a0da5f863db
📒 Files selected for processing (1)
packages/fluent-editor/src/modules/custom-clipboard.ts
| declare quill: FluentEditor | ||
|
|
||
| constructor(public quill: FluentEditor) { | ||
| super(quill) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Quill clipboard module constructor signature options parameter
💡 Result:
In Quill v2, the Clipboard module constructor is:
constructor(quill: Quill, options: Partial<ClipboardOptions>)where:
interface ClipboardOptions {
matchers: [Selector, Matcher][];
}
type Selector = string | Node['TEXT_NODE'] | Node['ELEMENT_NODE'];
type Matcher = (node: Node, delta: Delta, scroll: ScrollBlot) => Delta;So the only documented/typed option is matchers, an array of [selectorOrNodeType, matcherFn] entries. These configured matchers are appended after Quill’s default clipboard matchers (e.g., via modules: { clipboard: { matchers: [...] } }). [1], [2]
🏁 Script executed:
cat -n packages/fluent-editor/src/modules/custom-clipboard.ts | head -50Repository: opentiny/tiny-editor
Length of output: 1996
Constructor signature mismatch with Quill module system.
Quill v2's Clipboard module constructor expects (quill: Quill, options: Partial<ClipboardOptions>) where options.matchers allows users to configure custom clipboard matchers. This constructor only accepts quill, which means:
- Any
optionspassed by Quill are silently dropped - User-configured clipboard matchers will be lost
- The
super(quill)call doesn't forward options to the parentClipboardclass
Additionally, the declare quill: FluentEditor on line 23 is redundant and should be removed since it's redeclared as public on line 25.
The event listener added on line 28 is never cleaned up, creating a potential memory leak if the editor instance is destroyed or recreated.
🔧 Proposed fix to accept and forward options
export class CustomClipboard extends Clipboard {
- declare quill: FluentEditor
- constructor(public quill: FluentEditor) {
- super(quill)
+ constructor(quill: FluentEditor, options?: Record<string, unknown>) {
+ super(quill, options)
+ this.quill = quillAlso consider storing the listener reference and removing it on cleanup to prevent memory leaks:
+ private inputListener = () => {
+ if (this.quill.root.innerText !== '\n' && this.quill.root.classList.contains('ql-blank')) {
+ this.quill.root.classList.toggle('ql-blank', false)
+ }
+ else {
+ this.quill.options.placeholder = this.quill.getLangText('editor-placeholder')
+ }
+ }
+
constructor(quill: FluentEditor, options?: Record<string, unknown>) {
super(quill, options)
this.quill = quill
- this.quill.root.addEventListener('input', () => { ... })
+ this.quill.root.addEventListener('input', this.inputListener)
+ }
+
+ destroy() {
+ this.quill.root.removeEventListener('input', this.inputListener)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare quill: FluentEditor | |
| constructor(public quill: FluentEditor) { | |
| super(quill) | |
| constructor(quill: FluentEditor, options?: Record<string, unknown>) { | |
| super(quill, options) | |
| this.quill = quill |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fluent-editor/src/modules/custom-clipboard.ts` around lines 23 - 26,
The constructor for your custom Clipboard module is incorrect: remove the
redundant "declare quill: FluentEditor" and change the constructor signature to
accept both (quill: FluentEditor, options?: Partial<ClipboardOptions>) and
forward options to the parent by calling super(quill, options) so Quill-provided
matchers and options aren't dropped; also capture the event listener handler you
add (store the reference) and remove it during cleanup/destroy (e.g., on module
teardown or override a destroy method) to avoid the memory leak.
1 participant
…earing in the context of Chinese input method
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit