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

Reposition visible toolbar when it is in a not visible stack of rotator. #105

Merged
merged 2 commits into from
Aug 26, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Aug 23, 2019

Suggested merge commit message (convention)

Fix: Reposition visible toolbar when it is in a not visible stack of rotator. Closes ckeditor/ckeditor5#1957.


Additional information

The previous fix was causing problems in some edge cases - ie when toolbar was removed on focus lost and the whole logic was re-applied twice instead of just repositioning the toolbar. See ckeditor/ckeditor5#1957.
@jodator
Copy link
Contributor Author

jodator commented Aug 23, 2019

@Mgsy Could you test this solution? I think that this fixes the toolbar on table with image bug but it could re-introduce: https://github.com/ckeditor/ckeditor5-widget/issues/92.

cc @pjasiun & @oskarwrobel - I'm pinging you also with this as you better knew what happened in https://github.com/ckeditor/ckeditor5-widget/issues/92.

@coveralls
Copy link

coveralls commented Aug 23, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling dee7608 on t/1957 into cfd41c1 on master.

@jodator jodator marked this pull request as ready for review August 23, 2019 11:33
@jodator
Copy link
Contributor Author

jodator commented Aug 23, 2019

I didn't add any new tests as I feel that this fix looks like a better way of fixing the previous one.

Please do not merge until we decide if we're adding a test or not (I'll try to come up with a test-case anyway).

@mlewand mlewand requested a review from Mgsy August 23, 2019 11:49
@jodator jodator requested a review from Mgsy August 23, 2019 11:49
Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jodator
Copy link
Contributor Author

jodator commented Aug 23, 2019

As for now I'm not able to reproduce the bug with automated test. I'll try tomorrow probably.

@Reinmar
Copy link
Member

Reinmar commented Aug 23, 2019

Heh, I know that we know this, but this API needs to be seriously rethought...

When a setter doesn't do anything, you know that it's bad.

image

@Reinmar Reinmar merged commit a438c8b into master Aug 26, 2019
@Reinmar Reinmar deleted the t/1957 branch August 26, 2019 07:43
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.

Editor crashes after moving selection outside a table while an image is selected
4 participants