Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12163 closed Bug (invalid)

Maximize (and resize) plugin and shared spaces incompatibility

Reported by: Wiktor Walc Owned by: Artur Delura
Priority: Normal Milestone:
Component: General Version: 4.0
Keywords: Cc:

Description

It looks like the maximize plugin correctly turned off itself when inline mode is detected in init() callback:

// Maximize plugin isn't available in inline mode yet.
if ( editor.elementMode == CKEDITOR.ELEMENT_MODE_INLINE )
	return;

Similarly, Maximize button is disabled on iOS:

// Disabled on iOS (#8307).
modes: { wysiwyg: !CKEDITOR.env.iOS, source: !CKEDITOR.env.iOS },

We know that maximize plugin is useless when shared spaces are used, but instead of switching it off automatically just like with two previous cases, we rely on users knowledge:

removePlugins : 'maximize'

Perhaps we could do this automatically for maximize and resize plugins?

Change History (13)

comment:1 Changed 5 years ago by Jakub Ś

Status: newconfirmed
Version: 4.0

Perhaps this should be considered bug and not feature request?

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

Type: New FeatureBug

Yes, this is more a bug than a feature.

comment:3 Changed 5 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:4 Changed 5 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/12163 but here add new responsibility to plugin. It have to manage disabling self when some condition is met. Maybe we could handle it somewhere else, perhaps here: https://github.com/ckeditor/ckeditor-dev/blob/master/core/editor.js#L490-L493

And other issue is that we didn't removed sharedspace plugin but instead just prevent initialize it. Just see at editor instancce plugins field - maximize plugin is present.

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

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

Status: reviewreview_failed

You should check whether sharedspaces is enabled in the specific editor instance, not whether it's available globally in CKEDITOR. Use editor.plugins.sharedspace.

comment:6 Changed 5 years ago by Artur Delura

Status: review_failedreview

Right. I amended changes to branch:t/12163.

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

Milestone: CKEditor 4.4.5
Resolution: fixed
Status: reviewclosed

Fixed on master with git:665e3b9.

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

Milestone: CKEditor 4.4.5
Resolution: fixed
Status: closedreopened

I reverted this change in git:df1293de.

I realised that it doesn't make sense, because enabling the sharedspace plugin will disable the maximize and resize plugins even though the sharedspaces feature may not be truly enabled (it needs config.sharedSpaces). That may break websites which included the sharedspace but don't use it in some cases.

Additionally, I was reluctant to make this change from the beginning and now I'm strongly against it. One plugin cannot disable itself based on another's plugin configuration and existence. This will make too strong relations between our plugins and may be a cause of bugs in the future. Additionally, there are more incompatible plugins, like the bbcode, but we don't check in dozen of other plugins whether it's enabled. Instead, we properly configure the list of plugins which are enabled. This is the right way to resolve conflicts.

Of course I understand that we'll force developers to think about it, but the fact is that we are not able to do this right on our side. Not with all possible plugins, including 3rd party ones.

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

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

Status: reopenedpending

comment:10 Changed 5 years ago by Wiktor Walc

Altough I'm a fan of smart applications that solve known and common issues for you, I agree with Reinmar that we should solve this case in a different way.

The most important place to deal with this is the documentation: https://github.com/ckeditor/ckeditor-docs/issues/32

comment:11 Changed 5 years ago by Jakub Ś

Status: pendingconfirmed

I actually like to say a little bit of extra work hasn't killed anyone. I agree with @Reinmar as well.

comment:12 Changed 5 years ago by Jakub Ś

Resolution: invalid
Status: confirmedclosed

This will make too strong relations between our plugins and may be a cause of bugs in the future. Additionally, there are more incompatible plugins, like the bbcode, but we don't check in dozen of other plugins whether it's enabled. Instead, we properly configure the list of plugins which are enabled. This is the right way to resolve conflicts.


The most important place to deal with this is the documentation: ​https://github.com/ckeditor/ckeditor-docs/issues/32


Taking two above comments into account I'm closing this ticket as invalid.

comment:13 Changed 5 years ago by Anna Tomanek

This is also now properly documented as suggested in the GitHub issue.

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