I/9213 enabling caption toolbar #9459

magda-chrzescian · 2021-04-13T08:31:31Z

Suggested merge commit message (convention)

Fix (image): The buttons in the image toolbar should be enabled when the selection is in a caption. Closes #9213.
Fix (image): Dropdowns in the image toolbar should be disabled when all of the nested buttons are disabled. See #9213.

We actually don't need the getSelectedImageWidget function anymore. Whenever we are looking for an image on which to execute a command, we are also looking for the image ancestor of the selected caption. So I replaced the getSelectedImageWidget with getSelectedOrAncestorImageWidget.

It was a public function, so I'm not sure if it's ok to change its name and behavior. But because of the changes in managing the toolbar state, our integrator probably also should start using the getSelectedOrAncestorImageWidget function.

BTW, should such renaming be considered a breaking change?

…enabled.

oleq

Looks good 👍

My biggest concern, though, is why was the text alternative button left disabled when the selection is the caption? Is there any reason for that?

oleq · 2021-04-15T12:54:45Z

packages/ckeditor5-image/src/image/utils.js

+export function getImageModelElementAncestor( selection ) {
+	const selectedElement = selection.getSelectedElement();
+
+	return isImage( selectedElement ) ? selectedElement : selection.getFirstPosition().findAncestor( 'image' );


Suggested change

return isImage( selectedElement ) ? selectedElement : selection.getFirstPosition().findAncestor( 'image' );

return isImage( selectedElement ) ? selectedElement : selection.getFirstPosition().findAncestor( isBlockImage );

It seems to me that the model element findAncestor method accepts the element name only: https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_model_element-Element.html#function-findAncestor. Unless I'm missing something.

You're right. I must've mistaken it with some other helper. Sorry for the confusion.

oleq · 2021-04-15T12:59:08Z

packages/ckeditor5-image/tests/image/imagetypecommand.js

 			it( 'should be false when the selection is on other object', () => {
 				model.schema.register( 'object', { isObject: true, allowIn: '$root' } );
 				editor.conversion.for( 'downcast' ).elementToElement( { model: 'object', view: 'object' } );
 				setModelData( model, '[<object></object>]' );

 				expect( inlineCommand.isEnabled ).to.be.false;
 			} );
+
+			it( 'should be true when the selection is in a caption', () => {


Suggested change

it( 'should be true when the selection is in a caption', () => {

it( 'should be true when the selection is in a block image caption', () => {

oleq · 2021-04-15T12:59:39Z

packages/ckeditor5-image/tests/image/imagetypecommand.js

@@ -271,6 +266,18 @@ describe( 'ImageTypeCommand', () => {
 					'</paragraph>'
 				);
 			} );
+
+			it( 'should convert and set selection on the new image if the selection is in a caption', () => {


Suggested change

it( 'should convert and set selection on the new image if the selection is in a caption', () => {

it( 'should convert and set selection on the new image if the selection is in a block image caption', () => {

oleq · 2021-04-15T13:03:06Z

packages/ckeditor5-image/tests/image/utils.js

+			expect( image ).to.be.instanceOf( ModelElement );
+			expect( image.name ).to.equal( 'imageInline' );


expect( image.is( 'element', 'imageInline' ) ).to.be.true is enough to check this. The same applies to all tests that follow 👇

oleq · 2021-04-15T13:06:24Z

packages/ckeditor5-image/src/imagestyle/imagestyleui.js

@@ -150,6 +150,9 @@ export default class ImageStyleUI extends Plugin {
 				}
 			} );

+			dropdownView.bind( 'isEnabled' )
+				.toMany( buttonViews, 'isEnabled', ( ...areEnabled ) => !!areEnabled.find( isEnabled => isEnabled ) );


Suggested change

.toMany( buttonViews, 'isEnabled', ( ...areEnabled ) => !!areEnabled.find( isEnabled => isEnabled ) );

.toMany( buttonViews, 'isEnabled', ( ...areEnabled ) => areEnabled.some( isEnabled => isEnabled ) );

oleq · 2021-04-15T13:18:10Z

packages/ckeditor5-image/tests/imagestyle/imagestyleui.js

+			groups = groups.map( group => {
+				const items = group.view.toolbarView.items;
+
+				items.map( item => {
+					item.isEnabled = false;
+
+					return item;
+				} );
+
+				const activeButton = items.first;
+
+				activeButton.isEnabled = true;
+
+				return { ...group, activeButton };
+			} );
+
+			for ( const { buttonView } of groups ) {
+				expect( buttonView.isEnabled ).to.be.true;
+			}


This could be way simpler (why map() and return activeButton?):

Suggested change

groups = groups.map( group => {

const items = group.view.toolbarView.items;

items.map( item => {

item.isEnabled = false;

return item;

} );

const activeButton = items.first;

activeButton.isEnabled = true;

return { ...group, activeButton };

} );

for ( const { buttonView } of groups ) {

expect( buttonView.isEnabled ).to.be.true;

}

for ( const group of groups ) {

const groupItems = group.view.toolbarView.items;

for ( const item of groupItems ) {

item.isEnabled = false;

}

groupItems.first.isEnabled = true;

}

for ( const { buttonView, config } of groups ) {

expect( buttonView.isEnabled, `Failing group name: "${ config.name }"` ).to.be.true;

}

I didn't want to mutate the groups and mess up the following tests. I missed the fact that each of these tests has its own beforeEach and thus separate groups array. Thanks for the catch :)

oleq · 2021-04-15T13:19:09Z

packages/ckeditor5-image/tests/imagestyle/imagestyleui.js

+			groups = groups.map( group => {
+				const items = group.view.toolbarView.items;
+
+				items.map( item => {
+					item.isEnabled = false;
+
+					return item;
+				} );
+
+				return group;
+			} );
+
+			for ( const { buttonView } of groups ) {
+				expect( buttonView.isEnabled ).to.be.false;
+			}


Suggested change

groups = groups.map( group => {

const items = group.view.toolbarView.items;

items.map( item => {

item.isEnabled = false;

return item;

} );

return group;

} );

for ( const { buttonView } of groups ) {

expect( buttonView.isEnabled ).to.be.false;

}

for ( const group of groups ) {

const groupItems = group.view.toolbarView.items;

for ( const item of groupItems ) {

item.isEnabled = false;

}

}

for ( const { buttonView, config } of groups ) {

expect( buttonView.isEnabled, `Failing group name: "${ config.name }"` ).to.be.false;

}

magda-chrzescian · 2021-04-16T09:49:56Z

My biggest concern, though, is why was the text alternative button left disabled when the selection is the caption? Is there any reason for that?

Yes, my scatterbraining. I fixed it for the text alternative and image resize buttons.
I kept the imageUpload button the way it was (disabled). Theoretically, it can be placed in the image toolbar, but having it enabled with the caret inside the caption would suggest that it is possible to insert the image in there.

oleq · 2021-04-19T10:08:59Z

packages/ckeditor5-image/src/image/utils.js

@@ -65,7 +65,7 @@ export function getSelectedImageWidgetOrAncestor( selection ) {
 * @param {module:engine/view/selection~Selection|module:engine/view/documentselection~DocumentSelection} selection
 * @returns {module:engine/view/element~Element|null}
 */
-export function getSelectedImageWidgetAncestor( selection ) {
+export function getSelectionAncestorImageWidget( selection ) {


Looks like this helper is only used internally by getSelectedOrAncestorImageWidget(). I think we should merge them.

oleq · 2021-04-19T10:09:23Z

packages/ckeditor5-image/src/image/utils.js

@@ -86,7 +86,7 @@ export function getSelectedImageWidgetAncestor( selection ) {
 * @returns {module:engine/model/element~Element|null}
 */

-export function getSelectedImageElementOrAncestor( selection ) {
+export function getSelectedOrAncestorImageElement( selection ) {


What about getClosestSelectedImageElement()?

I'm not sure, the image element returned by this function doesn't have to be formally selected (<image><caption>[Foo]</caption></image>), so wouldn't this name be misleading?
However, I don't like the name above either. I was thinking about something like getCorrelatedImageElement - a bit enigmatic, but still understandable IMO.

Maybe then simply getClosestImageElement() (getClosestImageWidget())?

I liked the previous version better :) Changed to getClosestSelectedImageElement.

oleq · 2021-04-19T10:09:45Z

packages/ckeditor5-image/src/image/utils.js

@@ -49,14 +49,14 @@ export function isImageWidget( viewElement ) {
 * @param {module:engine/view/selection~Selection|module:engine/view/documentselection~DocumentSelection} selection
 * @returns {module:engine/view/element~Element|null}
 */
-export function getSelectedImageWidgetOrAncestor( selection ) {
+export function getSelectedOrAncestorImageWidget( selection ) {


What about getClosestSelectedImageWidget()?

magda-chrzescian · 2021-04-19T12:15:52Z

@oleq could you also refer to the following?

We actually don't need the getSelectedImageWidget function anymore. Whenever we are looking for an image on which to execute a command, we are also looking for the image ancestor of the selected caption. So I replaced the getSelectedImageWidget with getSelectedOrAncestorImageWidget.

It was a public function, so I'm not sure if it's ok to change its name and behavior. But because of the changes in managing the toolbar state, our integrator probably also should start using the getSelectedOrAncestorImageWidget function.

BTW, should such renaming be considered a breaking change?

oleq · 2021-04-19T12:57:47Z

We actually don't need the getSelectedImageWidget function anymore. Whenever we are looking for an image on which to execute a command, we are also looking for the image ancestor of the selected caption. So I replaced the getSelectedImageWidget with getSelectedOrAncestorImageWidget.

It was a public function, so I'm not sure if it's ok to change its name and behavior. But because of the changes in managing the toolbar state, our integrator probably also should start using the getSelectedOrAncestorImageWidget function.

BTW, should such renaming be considered a breaking change?

Since we're doing tons of breaking changes in this feature branch, I see nothing wrong with removing unused helpers and simplifying the names of the confusing ones.

Magdalena Chrześcian added 5 commits April 12, 2021 14:42

Disabling image toolbar dropdown if none of the items in toolbar are …

e898de7

…enabled.

getCorrelatedImage function and tests.

83bd112

Switching image type from the caption.

ffa6a77

Switching image style from the caption.

4f3fe19

Renaming, cleaning.

615c034

magda-chrzescian linked an issue Apr 13, 2021 that may be closed by this pull request

Disabled buttons in the image toolbar. #9213

Closed

Removed unnecessary space.

4436e36

magda-chrzescian requested a review from oleq April 13, 2021 10:17

Merge branch 'i/2052-inline-images' into i/9213-enabling-caption-toolbar

1accae2

oleq requested changes Apr 15, 2021

View reviewed changes

Magdalena Chrześcian added 4 commits April 16, 2021 12:44

Fixes after the review.

dc859b3

Enabling image resize and text alternative buttons + some renaming.

a4a0063

Tests for the ImageTextAternative, utils and ResizeImageCommand.

e79e7b6

Renaming and refactoring.

c4e7bbe

magda-chrzescian requested a review from oleq April 18, 2021 10:01

oleq reviewed Apr 19, 2021

View reviewed changes

Renaming and refactoring.

fbc420b

oleq merged commit b6bb0c8 into i/2052-inline-images Apr 20, 2021

oleq deleted the i/9213-enabling-caption-toolbar branch April 20, 2021 07:24

This was referenced Apr 20, 2021

Disabled buttons in the image toolbar. #9213

Closed

#8871: Integrate inline images with the link feature #9438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I/9213 enabling caption toolbar #9459

I/9213 enabling caption toolbar #9459

magda-chrzescian commented Apr 13, 2021 •

edited

Loading

oleq left a comment

oleq Apr 15, 2021

magda-chrzescian Apr 16, 2021

oleq Apr 19, 2021

oleq Apr 15, 2021

oleq Apr 15, 2021

oleq Apr 15, 2021

oleq Apr 15, 2021

oleq Apr 15, 2021

magda-chrzescian Apr 16, 2021

oleq Apr 15, 2021

magda-chrzescian commented Apr 16, 2021 •

edited

Loading

oleq Apr 19, 2021

oleq Apr 19, 2021

magda-chrzescian Apr 19, 2021

oleq Apr 19, 2021

magda-chrzescian Apr 20, 2021

oleq Apr 19, 2021

magda-chrzescian commented Apr 19, 2021

oleq commented Apr 19, 2021

	return isImage( selectedElement ) ? selectedElement : selection.getFirstPosition().findAncestor( 'image' );
	return isImage( selectedElement ) ? selectedElement : selection.getFirstPosition().findAncestor( isBlockImage );

	it( 'should be true when the selection is in a caption', () => {
	it( 'should be true when the selection is in a block image caption', () => {

	it( 'should convert and set selection on the new image if the selection is in a caption', () => {
	it( 'should convert and set selection on the new image if the selection is in a block image caption', () => {

		expect( image ).to.be.instanceOf( ModelElement );
		expect( image.name ).to.equal( 'imageInline' );

	.toMany( buttonViews, 'isEnabled', ( ...areEnabled ) => !!areEnabled.find( isEnabled => isEnabled ) );
	.toMany( buttonViews, 'isEnabled', ( ...areEnabled ) => areEnabled.some( isEnabled => isEnabled ) );

I/9213 enabling caption toolbar #9459

I/9213 enabling caption toolbar #9459

Conversation

magda-chrzescian commented Apr 13, 2021 • edited Loading

Suggested merge commit message (convention)

oleq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magda-chrzescian commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magda-chrzescian commented Apr 19, 2021

oleq commented Apr 19, 2021

magda-chrzescian commented Apr 13, 2021 •

edited

Loading

magda-chrzescian commented Apr 16, 2021 •

edited

Loading