Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Using model selection for calculating caption visibility #136

Merged
merged 3 commits into from
Sep 6, 2017
Merged

Conversation

szymonkups
Copy link
Contributor

@szymonkups szymonkups commented Sep 4, 2017

Suggested merge commit message (convention)

Fix: Fixed a bug causing editor with ImageCaption plugin enabled threw an error after view got rendered. Closes ckeditor/ckeditor5#5107.


Additional information

View selection was used before it was properly refreshed. This caused errors when trying to access current selection position. It is safer to use model selection in this case.

@scofalik
Copy link
Contributor

scofalik commented Sep 4, 2017

I propose more informative name for changelog entry.

Instead of
Fix: Using model selection when calculating caption visibility before rendering. Closes #127.

I propose
Fix: Fixed a bug causing editor with ImageCaption plugin enabled threw an error after view got rendered. Closes #127.

I'd explain what caused the bug (1-2 sentences) and how it was fixed (1 sentence) in additional information. @Reinmar ?

@scofalik
Copy link
Contributor

scofalik commented Sep 4, 2017

I miss some kind of test. I know that writing this kinds of tests is a bit frustrating, but if you would be able to reproduce the bug in unit test and then wrap it in expect( ... ).not.to.throw() block, it would be perfect.

@szymonkups
Copy link
Contributor Author

Updated PR description by adding "Additional information", added manual test checking the issue.

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

View selection was used before it was properly refreshed. This caused errors when trying to access current selection position. It is safer to use model selection in this case

But why is the view selection outdated? And which part of it, actually?

@scofalik scofalik merged commit 6147fee into master Sep 6, 2017
@scofalik scofalik deleted the t/127 branch September 6, 2017 13:59
@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

scofalik merged commit 6147fee into master 26 seconds ago

I guess I'll never know :D

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2017

OK, @scofalik has explained it to me that some nodes are removed from the view but the selection is still anchored in them, so you can't get an editableElement at that moment.

This is surprising and pity. And something which we might need to address anyway. It may cause a lot of bugs that selection is temporarily invalid and some well known properties are nulls.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

updateCaptionVisibility throws when typing at the end of a link
3 participants