KillerCodeMonkey / ngx-quill

Angular (>=2) components for the Quill Rich Text Editor
MIT License
1.77k stars 258 forks source link

Blur event is not triggered when we click on the checkbox after we focus in ngx-quill #1683

Closed pavitsrivatsan closed 1 year ago

pavitsrivatsan commented 1 year ago

The blur event doesn't get triggered when we check on a check box outside the editor.

Minimal reproduction : https://ngx-quill-angular-lxt4zn.stackblitz.io/

KillerCodeMonkey commented 1 year ago

The Blur event in ngx-quill is more a wrapper of the selection change event of quilljs. And there is the problem, that this event is often not triggered, when directly clicking other Inputs/ controls

You can validate that by listening in the onEditorChanged or Selection changed event. They should not be triggered then as well

Pavit Srivatsan @.***> schrieb am Fr., 28. Apr. 2023, 19:05:

The blur event doesn't get triggered when we check on a check box outside the editor.

Minimal reproduction : https://ngx-quill-angular-lxt4zn.stackblitz.io/

— Reply to this email directly, view it on GitHub https://github.com/KillerCodeMonkey/ngx-quill/issues/1683, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARI4YH6R4V4MHQKCH62QW3XDP2HFANCNFSM6AAAAAAXPPRMXM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ginobean commented 1 year ago

It looks to me like I can add a blur event listener directly to the scrollingContainer field of the QuillEditor:

onQuillEditorCreated(editorInstance: any) {
  editorInstance.scrollingContainer.addEventListener(
    'blur',
    (event: FocusEvent) => this.quillEditorBlur(event)
  );
}

quillEditorBlur(e: FocusEvent) {
  console.log('mark corresponding control as having been touched..');
}

Can you simply update the blur handling of the QuillEditor so that it does what my code above does?

Took a brief look at your blur handling code, in quill-editor.component.mjs, and it currently seems to be more complicated than it needs to be, without providing the basic functionality that we were expecting it to have.

KillerCodeMonkey commented 1 year ago

it depends if the quill editor is really losing the focus when this is happens. Only because blur is called for the scrollingContainer does not mean quilljs lost the focus (you are not able to type anymore). But seems like blur event is triggered even when pasting or using the toolbar

there are many open issues regarding this in react-quill, quilljs repo.

ginobean commented 1 year ago

The problem is that if I click inside the quill editor, then click on a different control, in the same form, your blur event is NOT getting called. In contrast, the blur event code example that I provided DOES get called. This is critical, if you need to know if a control has been touched at all, using blur events.

When I tried to use your blur event handling, to determine if the quill editor had been touched, was unable to do so reliably. Whereas, my code does seem to reliably handle it.

KillerCodeMonkey commented 1 year ago

Yep but only for controls to being touched. But as I said not 100% reliable. Because you Event gets triggered when quilljs is not "blurred" in the meaning that you lost the focus an can not type in the editor.

If I implement you solution I will get many issues about "blur gets triggered even the editor does not lose focus".

Since I want to stay as much as possible at the native quilljs behavior I decided to go that way.

But maybe I could implement both and let decide the developer which behavior first more their needs.

Gino @.***> schrieb am Mi., 20. Sept. 2023, 21:48:

The problem is that if I click inside the quill editor, then click on a different control, in the same form, your blur event is NOT getting called. In contrast, the blur event code example that I provided DOES get called. This is critical, if you need to know if a control has been touched at all, using blur events.

When I tried to use your blur event handling, to determine if the quill editor had been touched, was unable to do so reliably. Whereas, my code does seem to reliably handle it.

— Reply to this email directly, view it on GitHub https://github.com/KillerCodeMonkey/ngx-quill/issues/1683#issuecomment-1728333965, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARI4YGCATENLZLBWNDTHH3X3NCA5ANCNFSM6AAAAAAXPPRMXM . You are receiving this because you commented.Message ID: @.***>

ginobean commented 1 year ago

Implementing both sounds like reasonable approach. I generally prefer to use the API, and not delve into the guts, unless absolutely necessary!

KillerCodeMonkey commented 1 year ago

@ginobean i release 22.1.0-beta.2 i decided to go with 2 additional outputs (onNativeFocus, onNativeBlur). I added an additional fix that the native "blur" is not triggered when clicking something in the toolbar.

Could you try it if it fits your needs? Thanks

https://github.com/KillerCodeMonkey/ngx-quill/tree/v22.1.0-beta.2

ginobean commented 1 year ago

Yes. release 22.1.0-beta.2 resolves the blur issue, for me, via the onNativeBlur.

BTW, I don't know if this is due to a quirk of my local build environment or not, but I noticed that when I tried to build your v22.1.0-beta.2 ngx-quill locally, using 'npm run build', I encountered a couple of typescript errors,

Compiling with Angular sources in Ivy full compilation mode.
projects/ngx-quill/src/lib/quill-editor.component.ts:141:29 - error TS2769: No overload matches this call.
  Overload 1 of 4, '(token: ProviderToken<unknown>): unknown', gave the following error.
    Argument of type 'InjectionToken<Document>' is not assignable to parameter of type 'ProviderToken<unknown>'.
      Type 'InjectionToken<Document>' is not assignable to type 'InjectionToken<unknown>'.
        Property '_desc' is protected but type 'InjectionToken<T>' is not a class derived from 'InjectionToken<T>'.
  Overload 2 of 4, '(token: ProviderToken<unknown>, flags?: InjectFlags): unknown', gave the following error.
    Argument of type 'InjectionToken<Document>' is not assignable to parameter of type 'ProviderToken<unknown>'.

141   private document = inject(DOCUMENT)
                                ~~~~~~~~

projects/ngx-quill/src/lib/quill-editor.component.ts:267:90 - error TS2339: Property 'body' does not exist on type 'unknown'.

267         bounds = this.service.config.bounds ? this.service.config.bounds : this.document.body
                                                                                             ~~~~

I resolved both typescript errors, by adding // @ts-ignore above each of the two error causing lines, in the source code, and that allowed the build to succeed.

KillerCodeMonkey commented 1 year ago

maybe you have different tsconfig settings?

But i will release the beta version as a final release and will close this issue :)

Thanks

PS:

for any one else -> use onNativeFocus/Blur in Addition :)

ginobean commented 1 year ago

Sounds good. Thanks for the update! :).

skovalyov commented 7 months ago

@KillerCodeMonkey But when the component is used in a reactive form and { updateOn: 'blur' } is used for its form control, then it still relies on onBlur() not on onNativeBlur(). Would it make sense to make it configurable using, for example, some component property instead of two event handlers?

KillerCodeMonkey commented 7 months ago

In General I would like to keep the output, because maybe in some cases it make sense to keep track of both.

But I am up to improvements. Can I Do something to make your Integration easier?

skovalyov commented 7 months ago

@KillerCodeMonkey Let me explain my use case.

I have a reactive form with an ngx-quill editor as one of the controls. In controller it is declared as:

noteForm = this.fb.group({
  noteContent: this.fb.control<string>(null, { updateOn: 'blur' })
});

And in template it is declared as:

<quill-editor
  formControlName="noteContent"
  [modules]="quillModules">
</quill-editor>

When I subscribe to valueChanges, it does not emit when I click on another control outside the quill editor instance.

this.noteForm.get("noteContent").valueChanges.subscribe((value) => {
  /* Do something */
});

The underlying reason, as I understand, is the same as discussed in this issue, i.e. blur is not emitted.

What I have to do with the current API is to add onNativeBlur event handler and patch the reactive form manually, which is obviously not a good way to work with reactive forms.

What I think of is to have some property, e.g. blurEventHandler: 'quill' | 'native' that will determine what blur event should be used to notify the reactive form, if it is possible. Or some alternative solution?