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

Rendering view document after focusing #3968

Closed
szymonkups opened this issue Jan 31, 2017 · 3 comments · Fixed by ckeditor/ckeditor5-engine#864
Closed

Rendering view document after focusing #3968

szymonkups opened this issue Jan 31, 2017 · 3 comments · Fixed by ckeditor/ckeditor5-engine#864
Assignees
Milestone

Comments

@szymonkups
Copy link
Contributor

View document is rendered after each blur event but not after each focus. This means that if selection is placed in the exact same place (without selection change event) document will not be re-rendered:

output
Nested ediable updates its CSS class after being focused but it is not re-rendered so the blue outline is not showing when selection is placed in the exact same place.

I've tried to test if adding viewDocument.render() to FocusObserver on focus event helps with this but unfortunately it is not working as expected. I think following things are happening when focusing nested editable:

  • DOM element is focused, FocusObserver sets viewDocument.isFocused to true,
  • render() method is called, view elements are rendered,
  • old selection from view is rendered into DOM
  • there is no selection change event with selection placed inside nested editable.

It looks like rendering on each focus event (which is fired when nested editable is focused) will not allow to put selection inside nested editable (because it will be rendered back to its position from view selection).

Any ideas how to solve it?

@szymonkups szymonkups self-assigned this Jan 31, 2017
@pjasiun
Copy link

pjasiun commented Jan 31, 2017

That's because for some strange reason there is no render() call in the SelectionObserver anymore. It's observers role to re-render DOM if DOM changes and there is no change event from the model.

In this case, it would be perfect to call render on focus. It's focus, what changes, so it is FocusObserver who should re-render DOM.

Unfortunately, the selection change event is fired asynchronously. It means that if we re-render DOM synchronously after the focus change we will overwrite the DOM selection with the View selection, before the native DOM selection change event is fired, so before we update the view selection.

In my opinion, there are 2 things to do.

  1. Selection Observer has to call render if there is no change event. It was most probably removed because of the infinite loop protection, but this should be handled in a different way.

  2. Since, in this case, selection does not change, the Selection Observer may see no good reason to render. As I said it's focus observer role to ensure that render is called, in this case. It should wait for the next DOM selection change event, to prevent premature rendering and call render in the proper moment (does not matter if it was called by selection observer or not).

Note that it's renderer who should compare DOM and view and do nothing if there is no good reason to render anything. Observers should be free to call render whenever the feel that something may need the update.

@Reinmar
Copy link
Member

Reinmar commented Jan 31, 2017

Small change after F2F talk with @pjasiun:

  1. Focus observer should somehow trigger selection observer. See how selection observer triggers the mutation observer. Selection observer could have the same kind of method (i.e. flush()) which would update model selection (do _handleSelectionChange()).
  2. Both observers should call render().

Regarding 1 – we can consider change the flush() to better name.

Regarding 2 – I wouldn't touch the sel obs now because it's a separate issue (not needed for this issue) and it may be really tricky. @pjasiun is reporting a separate ticket for that.

@pjasiun
Copy link

pjasiun commented Jan 31, 2017

pjasiun referenced this issue in ckeditor/ckeditor5-engine Mar 22, 2017
Fix: View document is now re-rendered after focusing. Closes #795.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants