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

Hide ImageTextAlternative balloon when image element has been removed #138

Merged
merged 7 commits into from
Sep 13, 2017

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented Sep 8, 2017

Suggested merge commit message (convention)

Fix: Hide ImageTextAlternative balloon when image element has been removed by external changes. Closes ckeditor/ckeditor5#5115.

@oskarwrobel oskarwrobel requested a review from oleq September 8, 2017 18:12
@oleq
Copy link
Member

oleq commented Sep 11, 2017

Shouldn't this issue be included in http://localhost:8125/ckeditor5-ui/tests/manual/contextualballoon/externalchanges.html or provide a new MT?

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Minor changes needed in tests.

it( 'should return false when widget element is not the only element in the selection', () => {
const notWidgetizedElement = new ViewElement( 'p' );

frag = new ViewDocumentFragment( [ notWidgetizedElement, notWidgetizedElement ] );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be new ViewDocumentFragment( [ element, notWidgetizedElement ] ) instead?

@oskarwrobel
Copy link
Contributor Author

Shouldn't this issue be included in http://localhost:8125/ckeditor5-ui/tests/manual/contextualballoon/externalchanges.html or provide a new MT?

I was thinking about it but this is something that we are able to test by our unit tests. I think MT is useful to test repositioning because this is something we need to stub in unit tests.

But I can add MT if you think is needed for this case.

@oleq
Copy link
Member

oleq commented Sep 12, 2017

For one thing, I was surprised that the MT is in ckeditor5-ui instead of ckeditor5-link. The code which handles re-positioning and stuff on #render isn't in the ContextualBalloon. It's a pure link stuff.

Then I thought that we should check the same thing in the image and I finally found out there's a corresponding test in the image too ckeditor5-image/tests/manual/tickets/106/1.html. It doesn't test this kind of issue because what is "remotely" removed is the paragraph before the image instead of the image widget itself.

So maybe, let's just extend this MT and include this issue's case?

@oskarwrobel
Copy link
Contributor Author

I was using 106 MT to prepare the GIF for this issue. OK, I'll extend this test.

@oleq
Copy link
Member

oleq commented Sep 12, 2017

The MT is OK. But it revealed that the editor loses focus when the image which text alternative is being edited gets removed. This, I'm afraid, must be addressed (it works in the link feature because it explicitly calls focus).

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.

Image balloon is not removed together with image on external changes
2 participants