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

Link toolbar does not show for inline widgets with no defined toolbar #9607

Closed
ma2ciek opened this issue Apr 30, 2021 · 5 comments · Fixed by #9236
Closed

Link toolbar does not show for inline widgets with no defined toolbar #9607

ma2ciek opened this issue Apr 30, 2021 · 5 comments · Fixed by #9236
Assignees
Labels
package:image package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@ma2ciek
Copy link
Contributor

ma2ciek commented Apr 30, 2021

A follow-up to #2052. Branch i/2052-inline-images 

I've been testing inline widgets (https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html) on the current branch of inline images (0b674a4) and when a link is added on an inline widget, the balloon can not be later opened by clicking on the widget.

cc @oleq.

@ma2ciek ma2ciek added type:bug This issue reports a buggy (incorrect) behavior. squad:core Issue to be handled by the Core team. labels Apr 30, 2021
@pomek
Copy link
Member

pomek commented May 4, 2021

It happens because we block the LinkUI in the wrong way:

if ( view.createRangeIn( startLink ).getTrimmed().isEqual( range ) ) {
// The link element is not fully selected when, for instance, there's an inline image widget directly inside.
// This should be interpreted as a selected inline image and the image UI should be displayed instead
// (LinkUI should not interact).
if ( startLink.childCount === 1 && isWidget( startLink.getChild( 0 ) ) ) {
return null;
}
return startLink;
} else {

The code assumes that the selected widget must be an inline image. However, integrators can allow inserting a widget into a link.

We need to figure out some other way for blocking the LinkUI when the linked inline image is selected.

@pomek
Copy link
Member

pomek commented May 4, 2021

After applying some fixes, it works:

diff --git a/packages/ckeditor5-link/src/linkimageui.js b/packages/ckeditor5-link/src/linkimageui.js
index 1dc232a04a..93302e1d1f 100644
--- a/packages/ckeditor5-link/src/linkimageui.js
+++ b/packages/ckeditor5-link/src/linkimageui.js
@@ -49,10 +49,16 @@ export default class LinkImageUI extends Plugin {
 
 		// Prevent browser navigation when clicking a linked image.
 		this.listenTo( viewDocument, 'click', ( evt, data ) => {
+			// TODO: Should we block the `link` command or the `LinkUI` plugin?
+			// As for now, it blocks the command, as we do here `packages/ckeditor5-link/src/linkui.js:209`.
 			if ( this._isSelectedLinkedImage( editor.model.document.selection ) ) {
+				editor.commands.get( 'link' ).forceDisabled( 'LinkImageUI:link' );
+
 				data.preventDefault();
+			} else {
+				editor.commands.get( 'link' ).clearForceDisabled( 'LinkImageUI:link' );
 			}
-		} );
+		}, { priority: 'high' } );
 
 		this._createToolbarLinkImageButton();
 	}
diff --git a/packages/ckeditor5-link/src/linkui.js b/packages/ckeditor5-link/src/linkui.js
index c665c1cac3..4986cfc4b5 100644
--- a/packages/ckeditor5-link/src/linkui.js
+++ b/packages/ckeditor5-link/src/linkui.js
@@ -240,12 +240,17 @@ export default class LinkUI extends Plugin {
 	 */
 	_enableUserBalloonInteractions() {
 		const viewDocument = this.editor.editing.view.document;
+		const linkCommand = this.editor.commands.get( 'link' );
 
 		// Handle click on view document and show panel when selection is placed inside the link element.
 		// Keep panel open until selection will be inside the same link element.
 		this.listenTo( viewDocument, 'click', () => {
 			const parentLink = this._getSelectedLinkElement();
 
+			if ( !linkCommand.isEnabled ) {
+				return;
+			}
+
 			if ( parentLink ) {
 				// Then show panel but keep focus inside editor editable.
 				this._showUI();
@@ -621,7 +626,7 @@ export default class LinkUI extends Plugin {
 	 * selected and the **only** element within the selection boundaries.
 	 *
 	 * @private
	 * @returns {module:engine/view/attributeelement~AttributeElement|null}
 	 */
 	_getSelectedLinkElement() {
 		const view = this.editor.editing.view;
@@ -642,13 +647,6 @@ export default class LinkUI extends Plugin {
 
 			// Check if the link element is fully selected.
 			if ( view.createRangeIn( startLink ).getTrimmed().isEqual( range ) ) {
-				// The link element is not fully selected when, for instance, there's an inline image widget directly inside.
-				// This should be interpreted as a selected inline image and the image UI should be displayed instead
-				// (LinkUI should not interact).
-				if ( startLink.childCount === 1 && isWidget( startLink.getChild( 0 ) ) ) {
-					return null;
-				}
-
 				return startLink;
 			} else {
 				return null;

@pomek
Copy link
Member

pomek commented May 4, 2021

☝️ When the inline image is selected, I cannot open the UI using Ctrl+K. So blocking the command is not working. I'll check how it will work when disabling the LinkUI plugin.

@pomek
Copy link
Member

pomek commented May 4, 2021

It works. The latest changes:

diff --git a/packages/ckeditor5-link/src/linkimageui.js b/packages/ckeditor5-link/src/linkimageui.js
index 1dc232a04a..6a7e056b7f 100644
--- a/packages/ckeditor5-link/src/linkimageui.js
+++ b/packages/ckeditor5-link/src/linkimageui.js
@@ -46,13 +46,19 @@ export default class LinkImageUI extends Plugin {
 	init() {
 		const editor = this.editor;
 		const viewDocument = editor.editing.view.document;
+		const linkUI = editor.plugins.get( 'LinkUI' );
 
 		// Prevent browser navigation when clicking a linked image.
 		this.listenTo( viewDocument, 'click', ( evt, data ) => {
+			// Block the `LinkUI` plugin when an image was clicked. In such a case, we'd like to display the image toolbar.
+			// See: #9607.
 			if ( this._isSelectedLinkedImage( editor.model.document.selection ) ) {
+				linkUI.forceDisabled( 'LinkImageUI:link' );
 				data.preventDefault();
+			} else {
+				linkUI.clearForceDisabled( 'LinkImageUI:link' );
 			}
-		} );
+		}, { priority: 'high' } );
 
 		this._createToolbarLinkImageButton();
 	}
diff --git a/packages/ckeditor5-link/src/linkui.js b/packages/ckeditor5-link/src/linkui.js
index c665c1cac3..60323c42a7 100644
--- a/packages/ckeditor5-link/src/linkui.js
+++ b/packages/ckeditor5-link/src/linkui.js
@@ -246,6 +246,10 @@ export default class LinkUI extends Plugin {
 		this.listenTo( viewDocument, 'click', () => {
 			const parentLink = this._getSelectedLinkElement();
 
+			if ( !this.isEnabled ) {
+				return;
+			}
+
 			if ( parentLink ) {
 				// Then show panel but keep focus inside editor editable.
 				this._showUI();
@@ -642,13 +646,6 @@ export default class LinkUI extends Plugin {
 
 			// Check if the link element is fully selected.
 			if ( view.createRangeIn( startLink ).getTrimmed().isEqual( range ) ) {
-				// The link element is not fully selected when, for instance, there's an inline image widget directly inside.
-				// This should be interpreted as a selected inline image and the image UI should be displayed instead
-				// (LinkUI should not interact).
-				if ( startLink.childCount === 1 && isWidget( startLink.getChild( 0 ) ) ) {
-					return null;
-				}
-
 				return startLink;
 			} else {
 				return null;

niegowski added a commit that referenced this issue May 4, 2021
Fix (link): The link UI should be shown when clicking a linked inline widget. Closes #9607.
@pomek
Copy link
Member

pomek commented May 5, 2021

Fixed by #9617.

@pomek pomek closed this as completed May 5, 2021
oleq added a commit that referenced this issue Jun 1, 2021
Feature (image): Introduced support for inline images in editor content. Available out–of–the–box in all ready–to–use editor builds inline images can be uploaded, styled, resized, and linked and complement the already supported block images. See the [image feature overview guide](https://ckeditor.com/docs/ckeditor5/latest/features/images/images-overview.html) to see inline images in action. For more information about breaking changes, migration path, and tips, check out the [migration guide](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html).

Feature (image): It should be possible to define the dropdown menu as an object in the `config.image.toolbar` configuration. Closes #9340.

Other (image): Turned the image utils module into an editor plugin to allow sharing utils outside the package. See #8871.

Fix (image): The side-aligned images should always have some `min-width` property to not take the whole editor width. Closes #9342.

Fix (image): The floating block images, except the `side` image, should be displayed side by side by default. Closes #9183.

Fix (image): An image should never overflow the widget boundaries while changing its size. Closes #9166.

Fix (image): The size label should be displayed above the image if it doesn't fit inside. See #9166.

Fix (image): An Image caption placeholder text should not wrap or overflow. Closes #9162.

Other (image): The image toolbar should be visible if the selection is placed inside an image caption. Closes #9136.

Other (image): The image caption should be controlled using the toolbar button and a command for better integration with image styles. Closes #8907.

Other (build-decoupled-document): The editor document build should include the `ImageResize` plugin. Closes #9507.

Feature (link): Integrated the link feature with inline images. Closes #8871. Closes #9017. Closes #9167.

Fix (link): The link UI should be shown when clicking a linked inline widget. Closes #9607.

Other (widget): Safeguarded the way the `Widget` plugin sets the fake selection. Closes #9580.

Fix (widget): Selected inline widgets wrapped in an attribute in the view should create a fake selection. Closes #9524. Closes #9521.

Other (widget): Replaced the `findOptimalInsertionPosition()` helper with `findOptimalInsertionRange()` that will now attempt to replace selected blocks when inserting new widgets. Closes #9102.

Fix (clipboard): Hide all toolbars when the user starts dragging the widget and show them when the drag ends. Closes #9566.

Fix (code-block): The code block feature should not allow for inserting inline widgets as its content. Closes #9567.

Other (easy-image): Removed the `Image` plugin dependency from the `EasyImage` plugin. Closes #9399.

Other (table): Added the `.ck-table-bogus-paragraph` CSS class to the in-cell pseudo-paragraph used for data tables for easier and safer styling.

Other (core): Added several icons for new image styles (see #8909).

Other (ui): Added the `class` property to the `SplitButtonView` UI component. See #8909.

Other (image, table, html-embed, media-embed, page-break, horizontal-line): New widgets will replace the selected block instead of being added next to it while being inserted (see #9102).

BREAKING CHANGE (image): The `Image` plugin works as a glue for both `ImageBlock` and `ImageInline` features now (previously only block images). If you do not want inline images to be allowed, consider replacing the `Image` plugin with `ImageBlock` in your editor configuration. Otherwise, all images without the `<figure>` wrapper will be loaded into the editor content as inline images, which in some cases may affect content semantics and styling.

BREAKING CHANGE (image): The `ImageEditing` plugin is no longer standalone as the majority of its logic was extracted to `ImageBlockEditing` and `ImageInlineEditing` plugins. The logic remaining in the `ImageEditing` is common for both `ImageBlockEditing` and `ImageInlineEditing` plugins.

BREAKING CHANGE (image): The image caption is no longer displayed automatically the user selects a block image. Instead, its presence is controlled using the `'toggleImageCaption'` toolbar button and a [`ToggleImageCaptionCommand`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagecaption_toggleimagecaptioncommand-ToggleImageCaptionCommand.html) for better integration with [revamped image styles system](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html#image-styles). [Learn more](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html#image-caption).

BREAKING CHANGE (image): Some helpers from the image utils module (`@ckeditor/ckeditor5-image/src/image/utils.js`) have moved to the [`ImageUtils` plugin](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imageutils-ImageUtils.html). The helpers are still accessible via `editor.plugins.get( 'ImageUtils' )` namespace, for instance, `editor.plugins.get( 'ImageUtils' ).insertImage( ... )`.

BREAKING CHANGE (image): The API of the [`insertImage()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imageutils-ImageUtils.html#function-insertImage) helper has changed, please make sure to update your integrations.

BREAKING CHANGE (image): The `getSelectedImageWidget()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `getViewImgFromWidget()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `isImageAllowed()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `isImage()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `isImageWidget()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `toImageWidget()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `captionElementCreator()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `isCaption()` helper is no longer part of the public API.

BREAKING CHANGE (image): The `getCaptionFromImage()` helper is now available as [`getCaptionFromImageModelElement()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagecaption_utils.html#function-getCaptionFromImageModelElement).

BREAKING CHANGE (image): The `matchImageCaption()` helper is now available as [`matchImageCaptionViewElement()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagecaption_utils.html#function-matchImageCaptionViewElement).

BREAKING CHANGE (image): The `defaultIcons` are now available as [`DEFAULT_ICONS`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagestyle_utils.html#constant-DEFAULT_ICONS).

BREAKING CHANGE (image): The `defaultStyles` are now available as [`DEFAULT_OPTIONS`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagestyle_utils.html#constant-DEFAULT_OPTIONS).

BREAKING CHANGE (image): The linked image indicator (icon) rendered as a `<span>` with the `.ck-link-image_icon` CSS class has been removed. To alter the look of the indicator (including the icon), please use `figure.image > a::after` (for linked block images) and `a span.image-inline::after` (for linked inline images) CSS selectors instead.

BREAKING CHANGE (image): The `isImageWidget()` helper is no longer exposed by the [`Image`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image-Image.html) plugin.

BREAKING CHANGE (image): The API of common [image converters](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image_converters.html) has changed. Please refer to the documentation to learn more.

BREAKING CHANGE (image): The API of various [image caption utils](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imagecaption_utils.html) has changed. Please refer to the documentation to learn more.

BREAKING CHANGE (image): The conversion helpers [`srcsetAttributeConverter()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image_converters.html#function-srcsetAttributeConverter) and [`modelToViewAttributeConverter()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image_converters.html#function-modelToViewAttributeConverter) now require the `imageType` parameter.

BREAKING CHANGE (easy-image): The [`EasyImage`](https://ckeditor.com/docs/ckeditor5/latest/api/module_easy-image_easyimage-EasyImage.html) plugin is no longer automatically importing the [`Image`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image-Image.html) plugin as a dependency. [Learn more](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html#easyimage-plugin).

BREAKING CHANGE (ckfinder): The [`CKFinder`](https://ckeditor.com/docs/ckeditor5/latest/api/module_ckfinder_ckfinder-CKFinder.html) plugin is no longer automatically importing the [`Image`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_image-Image.html) plugin as a dependency. [Learn more](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html).

BREAKING CHANGE (link): The `isImageAllowed()` helper is now available as [`isLinkableElement()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_link_utils.html#function-isLinkableElement).

BREAKING CHANGE (media-embed): The API of the [`insertMedia()`](https://ckeditor.com/docs/ckeditor5/latest/api/module_media-embed_utils.html#function-insertMedia) helper has changed. See the documentation to learn more.

BREAKING CHANGE (build-decoupled-document): The official [decoupled document](https://www.npmjs.com/package/@ckeditor/ckeditor5-build-decoupled-document) build now ships with the [`ImageResize`](https://ckeditor.com/docs/ckeditor5/latest/api/module_image_imageresize-ImageResize.html) plugin enabled by default. [Learn more](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/migration/migration-to-28.html).

BREAKING CHANGE (table): The in-cell pseudo-paragraph used for data tables is no longer styled using the inline `style` attribute but a `.ck-table-bogus-paragraph` CSS class instead.

BREAKING CHANGE (widget): The `findOptimalInsertionPosition()` helper is now `findOptimalInsertionRange()` and returns a [model range](https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_model_range-Range.html). Also, instead of searching for a position next to the currently selected block, it will now attempt to replace that block (see #9102).

BREAKING CHANGE (widget): The `checkSelectionOnObject()` helper is no longer part of the public API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants