Opened 6 years ago

Closed 5 years ago

Last modified 2 years ago

#7430 closed New Feature (fixed)

Align button should work on selected objects

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Styles Version:
Keywords: IBM Cc: satya_minnekanti@…

Description

If an object is selected (an image or a table), the alignment buttons should act on that specific object, not the paragraph. At least the align left and right buttons.

These buttons should simply apply/remove the proper float style to the object.

Attachments (5)

7430.patch (4.2 KB) - added by Garry Yao 6 years ago.
7430_2.patch (2.7 KB) - added by Frederico Caldeira Knabben 5 years ago.
7430_3.patch (5.1 KB) - added by Garry Yao 5 years ago.
7430_4.patch (5.4 KB) - added by Frederico Caldeira Knabben 5 years ago.
7430_5.patch (5.6 KB) - added by Garry Yao 5 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added
Keywords: ibm added

Changed 6 years ago by Garry Yao

Attachment: 7430.patch added

comment:2 Changed 6 years ago by Garry Yao

Component: GeneralCore : Styles
Owner: set to Garry Yao
Status: newreview

Proposed patch override justify commands for elements image, table and flash, further elements requested are to be included.

comment:3 Changed 6 years ago by Frederico Caldeira Knabben

Keywords: IBM added; ibm removed
Status: reviewassigned

I think we need to first come to a conclusion on #7207, to finally define our approach here.

Changed 5 years ago by Frederico Caldeira Knabben

Attachment: 7430_2.patch added

comment:4 Changed 5 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: assignedreview

This new patch takes a different approach for it.

It introduces a simple implementation into the justify plugin, which makes this feature event/delegation based. In this way, each interested plugin can implement it in the most adequate way, without having to force us to come with a single solution that should work for everybody.

The patch includes the feature for images only, which I believe is the most urgent case for this. If this approach is accepted, we can have it on other plugins through dedicated tickets.

Changed 5 years ago by Garry Yao

Attachment: 7430_3.patch added

comment:5 Changed 5 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao

It looks like the two plugin specific events introduced are here just to work around this particular issue, while such a requirement here could make us considering it a bit generic for the future flexibility.

In the new patch the "beforeExecCommand" event is used to override the default alignment behavior, and a "commandState" event is introduced, to allow the command state to be determinate in not only one space (plugin).

comment:7 in reply to:  6 Changed 5 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

http://ckeditor.t/tt/7430/1.html

I think this test hasn't been pushed.

comment:8 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The proposed patch is appending a new method called queryCommandState. The thing that sounds strange is that the patch never uses that method to "query the command state", but just to "update the command state". Sounds counter-intuitive, in fact.

Additionally, by using beforeCommandExec and the new commandState events, we run those listeners for every single command. Considering that we have access to the commands, we could have something better.

I'm coming with yet another patch, which are under the same lines of the current one, but taking the above comments in consideration.

Changed 5 years ago by Frederico Caldeira Knabben

Attachment: 7430_4.patch added

Changed 5 years ago by Garry Yao

Attachment: 7430_5.patch added

comment:9 Changed 5 years ago by Garry Yao

Status: review_failedreview

The new patch fixes the following minor issues with TC updates.

  1. Handle "align" attribute (deprecated) on image;
  2. No justification status when cursor is directly inside of body (enter br).

comment:10 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.3
Status: reviewreview_passed

comment:11 Changed 5 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7422].

comment:12 Changed 5 years ago by David

With the release of 3.6.3, does this mean that images will no longer allow center alignment? Is that feature on the roadmap?

comment:13 in reply to:  12 Changed 5 years ago by Frederico Caldeira Knabben

Replying to dbrownITR:

With the release of 3.6.3, does this mean that images will no longer allow center alignment? Is that feature on the roadmap?

With the 3.6.2, it is not possible to align images to center as well. Am I wrong?

comment:14 Changed 5 years ago by David

In 3.6.2 we were able to:

  1. Insert an image on its own line (therefore wrapped in P tags)
  2. Click on the image to select it
  3. Click the "Center text" button which applies the "text-align: center" style to the wrapping P tag

In 3.6.3, steps 1 and 2 work the same but step 3 is no longer possible as the "Center" button is completely disabled while an image is selected.

comment:15 in reply to:  14 Changed 5 years ago by Frederico Caldeira Knabben

Replying to dbrownITR:

In 3.6.2 we were able to:

  1. Insert an image on its own line (therefore wrapped in P tags)
  2. Click on the image to select it
  3. Click the "Center text" button which applies the "text-align: center" style to the wrapping P tag

In 3.6.3, steps 1 and 2 work the same but step 3 is no longer possible as the "Center" button is completely disabled while an image is selected.

I see... well, you're not really centering the image, but the paragraph. In fact this was a cause for misunderstanding because users expected the image to be centered, not the paragraph.

It is still possible to achieve your needs though... it is enough to put the caret after/before the image and the alignment buttons will get enabled.

comment:16 Changed 5 years ago by David

Thanks for the info. I discovered that alternative method just a few minutes ago as well. With this workaround, we can adjust our user's understand of how the tool works.

comment:17 in reply to:  16 ; Changed 5 years ago by zeeterminata

Replying to dbrownITR:

Thanks for the info. I discovered that alternative method just a few minutes ago as well. With this workaround, we can adjust our user's understand of how the tool works.

As a client, I'm really not very happy about this and basically saying, "Deal with it," is a bit unprofessional. The fact that I don't even have a center align option in the image properties pane either; at least with the developer that I am working with.

On my own personal blog, I use Wordpress and at work (where I'm working with right now) we use Drupal. And this is how I'm going to have to align my images to the center?

comment:18 in reply to:  17 Changed 5 years ago by Frederico Caldeira Knabben

Replying to zeeterminata:

As a client, I'm really not very happy about this...

I understand your disappointment. The fact is that HTML doesn't provide a way to center inline elements. This can only be achieved by aligning the entire block element or by CSS tricks that may not work perfectly.

I've just opened #8938 as a possible solution for that. Feel free to add comments there if you have anything to add.

comment:19 Changed 2 years ago by Satya Minnekanti

Please re-open this ticket as it's not properly fixed. If i select a Table and click on Align Right button it's applying Alignment to Table content instead of Table. This works for images. We have a customer logged ticket for this issue

comment:20 Changed 2 years ago by Jakub Ś

@satya please see #13102.

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