Opened 4 years ago

Closed 3 years ago

#13197 closed Bug (fixed)

Image2: Linked inline image's alignment class is not transferred to widget wrapper

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.5.0
Component: General Version: 4.4.0
Keywords: Cc:

Description

Source:

<p>
	<a href="http://en.wikipedia.org/wiki/File:Comet_67P_on_19_September_2014_NavCam_mosaic.jpg"><img src="img/comet-67p.jpg" alt="Comet 67P" width="1131" height="900" class="image30 align-right" /></a>
</p>

Settings:

	image2_alignClasses: [ 'align-left', 'align-center', 'align-right' ],
	image2_disableResizer: true,

	stylesSet: [
		{ name: 'Image 30%', type: 'widget', widget: 'image', attributes: { 'class': 'image30' } }
	]

Change History (13)

comment:1 Changed 4 years ago by Piotrek Koszuliński

Status: newconfirmed

Compare this with a not linked image case. With link the class is left on the image, without link it's properly transferred to the wrapper.

comment:2 Changed 3 years ago by Piotrek Koszuliński

Priority: NormalHigh

comment:3 Changed 3 years ago by Piotrek Koszuliński

Version: 4.4.0

comment:4 Changed 3 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:5 Changed 3 years ago by Artur Delura

I found curious relationship: If we add some text content (even one space) between closing img and a tag then everything is fine. There are two diffrences. Anchor is moved out of the widget wrapper and wrapper itself contains desirable class.

The question is whether anchor tag should remain in widget wrapper or it might be kicked out from it? Think it's good to have link out of the image wrapper. I don't see a reson to keep it in.

comment:6 Changed 3 years ago by Piotrek Koszuliński

That behaviour is not a surprise. Image2 accepts links only as it (itself) would create them - without any spaces. That's totally correct and you can ignore links with spaces in this ticket. The question that can be asked is - why do we keep links inside widgets. I remember that there were important reasons, but @anowodzinski could definitely write more.

comment:7 Changed 3 years ago by Artur Delura

Moving back to issue:

When widget is created there is set a data object on it. Data object has propety align which is set to none. Aling setup is here. It look for specified classes in this.element which in our case is an anchor, which obviously doesnt' have such classes, so finally align is set to none.

Here comes another question whether property element has proper value - which is an anchor. Shouldn't it be an image? I think the answer is: This is okay.

I assume that this is okay. In code listed above, we can look through not only this.element but it's child as well. Which in our case is an image which have our class which we are looking for.

Last edited 3 years ago by Artur Delura (previous) (diff)

comment:8 Changed 3 years ago by Artur Delura

Status: assignedreview

Changes and tests in branch:t/13197.

comment:9 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I was close to giving R+, but I don't like one thing - blindly checking if any element of the widget has an alignment class.

if ( this.element.hasClass( alignClasses[ 0 ] ) || image.hasClass( alignClasses[ 0 ] ) )
    data.align = 'left';
else if ( this.element.hasClass( alignClasses[ 2 ] ) || image.hasClass( alignClasses[ 2 ] ) )
    data.align = 'right';

I would rather see there a check - if it's a captioned image, then look for class on this.element. If it's not a captioned image, always look on image.

comment:10 Changed 3 years ago by Artur Delura

Status: review_failedassigned

comment:11 Changed 3 years ago by Artur Delura

Changes and tests in branch:t/13197.

Last edited 3 years ago by Artur Delura (previous) (diff)

comment:12 Changed 3 years ago by Artur Delura

Status: assignedreview

comment:13 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

I removed the 2nd case from the manual test because it wasn't necessary. If anything, it should be tested by automated tests. Besides that - the fact that alignment class work only on image depends only on configuration. If you changed ACF and contents.css everything could be fine.

Fixed on major with git:ce8ccc2.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy