Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-root editor: cannot focus root which has only block widget inside #16806

Closed
FilipTokarski opened this issue Jul 26, 2024 · 7 comments · Fixed by #16892
Closed

Multi-root editor: cannot focus root which has only block widget inside #16806

FilipTokarski opened this issue Jul 26, 2024 · 7 comments · Fixed by #16892
Assignees
Labels
package:editor-multi-root support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@FilipTokarski
Copy link
Member

📝 Provide detailed reproduction steps (if any)

  1. Go to multi-root editor demo
  2. Remove all content from one of the roots and add only a table
  3. Focus other root and then try focusing the root with the table (but do not click on the table)

✔️ Expected result

Focus should stay in the root, similarly to a "standard" editor:

Screen.Recording.2024-07-26.at.12.55.51.mov

❌ Actual result

Focus jumps back to the other root, it's not possible to focus a root which has only block widget:

Screen.Recording.2024-07-26.at.12.54.42.mov

❓ Possible solution

If you have ideas, you can list them here. Otherwise, you can delete this section.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: …
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@FilipTokarski FilipTokarski added type:bug This issue reports a buggy (incorrect) behavior. support:2 An issue reported by a commercially licensed client. package:editor-multi-root labels Jul 26, 2024
@pszczesniak
Copy link
Contributor

It does not need to ba a table, it can be anything that selected got a toolbar/tooltip; for example an image:

Screen.Recording.2024-07-26.at.13.20.09.mov

@Mati365
Copy link
Member

Mati365 commented Aug 8, 2024

@FilipTokarski / @pszczesniak I checked it on Firefox, on latest docs, and I'm not able to reproduce this issue. Can you still reproduce this?

@pszczesniak
Copy link
Contributor

@Mati365 yes i can (Chrome).

@pszczesniak
Copy link
Contributor

On Firefox it works as expected.

@Mati365 Mati365 self-assigned this Aug 8, 2024
@Mati365
Copy link
Member

Mati365 commented Aug 8, 2024

Summary

Clicking on element with contenteditable="false" containing only one element with contenteditable="true" does not trigger focus (and selection change) on such contenteditable="true" child. It's reproducible in this test.zip, click in white area bordered with purple border and compare results between Chrome and Firefox. In the proposed change we trigger focus by the hand on such element before handling selection in the editor.

Debug notes

  1. It's not recent regression. It's possible to reproduce it on 39 version.
  2. Increasing timeout in this function shows that clicking on editable does not focus nested block element:

https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/view/observer/focusobserver.ts#L106-L110

obraz

so it's performing fallback to previously focused node

  1. It's reproducible on 1 editable. It's possible to focus table by clicking outside editable (while having it focused) 🤯
  2. Looks like Chrome treats focusing nested contenteditable differently than Firefox. Maybe it's causing the issue? Example: test.zip. Click on editable element and check where the editing cursor really is on chrome and on Firefox.
  3. Focus tracker assumes that selectionchange event is fired after focusing element (in our case, it's editable). Check the code. After a small amount of time, it's flushing interface and then fallbacks to previous selected element because there is no selection (or that selection is incorrect) in the focused area. It works on Firefox, however. It's directly related to test.zip.

@f1ames
Copy link
Contributor

f1ames commented Aug 9, 2024

I stumbled upon this same issue today. I prepared demo for rerepo and then found this issue... Anyway, I'll share it, maybe it could be helpful - https://stackblitz.com/edit/1haefu?file=main.js.

Screen.Recording.2024-08-09.at.16.22.33.mov

What' interesting here is the other part of this issue (however, I'm not 100% sure if it's the same cause) is how document selection updates when you focus and then deselect different roots.

AFAIU selecting the root (and then deselecting it) should result in document selection (editor.editing.view.document.selection) being placed inside this focused root. However, with block widget inside middle root something happens and selection gets back to middle root, as if it's gets stolen by it:

Screen.Recording.2024-08-09.at.16.35.05.mov

@Mati365 maybe it could be worth checking, but if it's something different I can extract to a separate issue.

@Mati365
Copy link
Member

Mati365 commented Aug 9, 2024

@f1ames That's the same issue. I suspect that Chrome is not firing the selection change event properly after focusing editable that contains block element. So we have focus but there is no selection, and the editor is trying to fall back to the last known focused element because editable does not contain any cursor. The funny thing is that forcing chrome to focus first block or contenteditable element works.

https://github.com/ckeditor/ckeditor5/pull/16892/files#diff-5dd5f41942a4096889e59688767ae58a1755591bf98f1e52c8c8e9baba0609d0R254-R282

I extracted the issue to bare minimum HTML here: https://stackblitz.com/edit/stackblitz-starters-lf3smh?file=index.html

So on Firefox it used to work ok:

stackblitz-selection-2024-08-09_16.54.24.mp4

but on Chrome:

stackblitz-selection-chrome-2024-08-09_16.55.51.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:editor-multi-root support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants