Opened 9 years ago

Last modified 9 years ago

#12618 confirmed Bug

Block alignment buttons during upload

Reported by: Piotr Jasiun Owned by:
Priority: Normal Milestone:
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

Until upload is done image alignment of the images should not be possible because it ends up with the wrong content if user user image2.

Change History (17)

comment:1 Changed 9 years ago by Piotr Jasiun

Status: newconfirmed

comment:2 Changed 9 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:3 Changed 9 years ago by Piotr Jasiun

Status: assignedreview

Done. Changes in t/12618.

I had to use selectionChange event, because when I tried with widget.focus is was changed anyway later by updateCommandsContext called from selectionChanged.

The whole enabling/disabling commands mechanism works pretty bad. They can be, disable because of many reasons (context, mode, readonly) and there is no single mechanism which control the state of the command. There should be an event witch is called every time there is possibility that state changes and every listener can disable command.

comment:4 Changed 9 years ago by Piotr Jasiun

I created ticket about this mechanism #12929.

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

Status: reviewreview_failed

The code will bleed when there are no justify commands.

There should be an event witch is called every time there is possibility that state changes and every listener can disable command.

There's a command#state event which you can cancel and change the state back to disabled. I find this much more straightforward solution - you listen on these 4 command's state events and block state changes while the widget is focused.

Finally, a manual tc (tags: 4.5.0, tc) is missing.

Last edited 9 years ago by Piotrek Koszuliński (previous) (diff)

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

PS. If what I wrote in comment:5 will allow to solve this issue, then I think we can close #12929.

comment:7 Changed 9 years ago by Piotr Jasiun

Status: review_failedreview

I pushed changes and rebased this branch onto #12805 so I can create a manual test. Changes in t/12618.

As we discussed using state event is not an universal solution for the general problem. I changed description of the #12929 to point some ideas.

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

Rebase needed due to conflicts.

comment:9 Changed 9 years ago by Piotr Jasiun

Done.

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

Status: reviewassigned

While review we noticed two things:

  1. This change should touch only uploadimage.
  2. We need a nicer way to control available alignment options or commands states when element/widget is selected.

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

Owner: Piotr Jasiun deleted
Status: assignedconfirmed

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

Milestone: CKEditor 4.5.0 BetaCKEditor 4.5.0

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

Version: 4.5.0 Beta

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

Milestone: CKEditor 4.5.0

comment:15 Changed 9 years ago by Piotr Jasiun

We decided that solution should be more generic, because this is a problem we have with every widget: some commands does not make sense when widget is focused. Instead of having hardcoded list of disabled commands, this logic should be moved to the widget plugin and list of commands should be in a widget definition property.

I am not sure on what level we should disable commands: this should we a list of commands, toobarGroups or toolbars and if it should be a white- or black-list and I do not remember what we decided. We can not easily decide which command makes sense in each context: about command should be active always, justify* should be disabled for upload widget, but *list (which also works on the paragraph) could be available.

I think we should focus on solving current case which is conflict between upload widget and justify in image2 and do not make too generic solutions. So the widget property should simply have the list of commands which should be disabled when widget is focused:

    disabledCommands: [ 'justifyleft', 'justifycenter', 'justifyright', 'justifyblock' ]

Then we could have a more generic solution when we fix #12929 one day. With the single entry point user will be able to easily implement black/whitelisting for commands or toolbarGroups, so whatever he need. One day.

Last edited 9 years ago by Piotr Jasiun (previous) (diff)

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

I remember that we were thinking about whitelisting commands that could be enabled. It would be cool, because you more know about stuff that will work with your widget rather than about stuff that won't. At least it seemed so... I realised that you would need to whitelist stuff like "source", "undo", "redo", etc. Otherwise, the editor would disable them all. Sad.

I agree with you @pjasiun on the fact the we should now do the minimum. This won't become a super cool feature anyway, because our commands are not grouped/tagged in any reasonable way. Using toolbar groups is not an option, because commands are not buttons. There's a relation most of the time, but that would be ugly :D.

So, let's do the minimum.

comment:17 in reply to:  16 Changed 9 years ago by Piotr Jasiun

Replying to Reinmar:

I remember that we were thinking about whitelisting commands that could be enabled. It would be cool, because you more know about stuff that will work with your widget rather than about stuff that won't. At least it seemed so... I realised that you would need to whitelist stuff like "source", "undo", "redo", etc. Otherwise, the editor would disable them all. Sad.

I agree, we were talking about white-listing. But also it is true that all commands like "source", "undo", "redo", "about", "find", all insertion commands, etc. should be enabled. This is why I believe black-listing is the simpler solution, and what we need in this case.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy