Opened 6 years ago

Closed 5 years ago

Last modified 21 months ago

#7430 closed New Feature (fixed)

Align button should work on selected objects

Reported by: fredck 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 fredck 5 years ago.
7430_3.patch (5.1 KB) - added by garry.yao 5 years ago.
7430_4.patch (5.4 KB) - added by fredck 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

  • Cc satya_minnekanti@… added
  • Keywords ibm added

Changed 6 years ago by garry.yao

comment:2 Changed 6 years ago by garry.yao

  • Component changed from General to Core : Styles
  • Owner set to garry.yao
  • Status changed from new to review

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 fredck

  • Keywords IBM added; ibm removed
  • Status changed from review to assigned

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

Changed 5 years ago by fredck

comment:4 Changed 5 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from assigned to review

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

comment:5 Changed 5 years ago by garry.yao

  • Owner changed from fredck 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 fredck

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 fredck

  • Status changed from review to review_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 fredck

Changed 5 years ago by garry.yao

comment:9 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

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 fredck

  • Milestone set to CKEditor 3.6.3
  • Status changed from review to review_passed

comment:11 Changed 5 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7422].

comment:12 follow-up: Changed 5 years ago by 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?

comment:13 in reply to: ↑ 12 Changed 5 years ago by fredck

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 follow-up: Changed 5 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by fredck

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 follow-up: Changed 5 years ago by 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.

comment:17 in reply to: ↑ 16 ; follow-up: 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 fredck

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 21 months ago by satya

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 21 months ago by j.swiderski

@satya please see #13102.

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