Ticket #4973 (closed Bug: fixed)

Opened 5 years ago

Last modified 4 years ago

Div dialog 'Style' select field in vain

Reported by: garry.yao Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.2
Component: Core : Styles Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc:

Description

The 'elementStyle' field in div dialog have no effect right now, it will never be populated, we should have the functionality properly align with v2.

Attachments

4973.patch (6.5 KB) - added by garry.yao 5 years ago.
default.js (2.7 KB) - added by garry.yao 5 years ago.
Modified Default stylesSet for Testing Div Dialog
4973_2.patch (7.8 KB) - added by garry.yao 5 years ago.
4973_3.patch (9.3 KB) - added by garry.yao 4 years ago.
4973_4.patch (10.1 KB) - added by garry.yao 4 years ago.

Change History

comment:1 follow-up: ↓ 3 Changed 5 years ago by garry.yao

In V2 we've been using "any customized styleset that apply to div' for it, while we don't have global styleset anymore, is it a good idea to just allow all block styles here so the 'div' element could even get chance to be changed to other blocks?

comment:2 Changed 5 years ago by wwalc

We shouldn't take styles for all block elements, for example "div" should not become "ul" automatically if someone have defined style for the "ul" element. It is also uncommon to use headers as containers.

If we're going to provide styles for other elements than div and then change div into other element, the 'div' dialog should be renamed to 'container', a more generic name, otherwise the behaviour would be a little bit counterintuitive (after all it's a "div" dialog).

When no styles are available, we could disable that select field, to not confuse users and also we could provide some default style just to show users that such functionality exists (otherwise it will be disabled by default).

comment:3 in reply to: ↑ 1 Changed 5 years ago by fredck

Replying to garry.yao:

In V2 we've been using "any customized styleset that apply to div' for it, while we don't have global styleset anymore, is it a good idea to just allow all block styles here so the 'div' element could even get chance to be changed to other blocks?

No, never. The <div> here is not a normal block, like a <p>. It's a container for other block elements. We must just replicated the V2 behavior.

Changed 5 years ago by garry.yao

Changed 5 years ago by garry.yao

Modified Default stylesSet for Testing Div Dialog

comment:4 Changed 5 years ago by garry.yao

  • Keywords Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

Alignment with the v2 feature by globalize the custom style definitions.
The attached 'default' stylesSet could be used for testing.

Changed 5 years ago by garry.yao

comment:5 Changed 5 years ago by garry.yao

The new patch handles the possible conflicts of same attributes/styles between the div styles and default dialog fields, please note that it's a bit counterintuitive that by setting 'Div Style' to empty on an existed stylish div will not actually remove that style.

comment:6 Changed 5 years ago by alfonsoml

I've created #5023 to improve the default styleSet sample.

comment:7 Changed 5 years ago by alfonsoml

  • Keywords Review- added; Review? removed

The change in resourcemanager.js is breaking my test page where I load a plugin from a custom folder.

	CKEDITOR.plugins.addExternal( 'easyupload', '/CKEplugins/easyupload/' );

	config.extraPlugins='easyupload';

now tries to load

    '/CKEplugins/easyupload/undefined?t=1263666437818'

After fixing that, what about changing

CKEDITOR.stylesSet.addExternal( styleSetName,
		externalPath ? stylesSet.slice( 1 ).join( ':' ) : pluginPath + 'styles/',
		externalPath ? '' : styleSetName + '.js' );

to

CKEDITOR.stylesSet.addExternal( styleSetName,
		externalPath ? stylesSet.slice( 1 ).join( ':' ) : pluginPath + 'styles/' + styleSetName + '.js', 
		'' );

I would move all that initialization (lines 202-209) to the top of the init function of the plugin (line 18)

Please, read #5025 with regards to the removal of two API functions.

In the dialog, line 453 doesn't need the assignment to the 'style' var, and so that var can be removed also from line 443

    style = styles[ styleName ] = new CKEDITOR.style( styleDefinition ); 

The rest seems to work, although I haven't tried any kind of extensive check.

Changed 4 years ago by garry.yao

comment:8 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

New patch targeting all above comments.

comment:9 Changed 4 years ago by alfonsoml

  • Keywords Review- added; Review? removed

This patch creates a bug in IE:

1 Load replacebyclass in IE8 2 Select the contents (or part of it) and create a DIV container. No need to specify any attribute

  1. Edit the container using the context menu. Don't change anything and click OK
  2. Switch to source and back to design
  3. Try to edit again the container like in step 3. It gives a "permission denied" at line 97 in a call from commitInternally

This one can be filed as another bug as it also happens in 3.1: trying to set the content direction to LTR initially fails, it's set only after setting first RTL.

Changed 4 years ago by garry.yao

comment:10 Changed 4 years ago by garry.yao

  • Keywords Review? added; Review- removed

New patch targeting the following issues:

  1. 'CKEDITOR.stylesSet' should defined by styles plugin instead of stylesCombo plugin;
  2. Div dialog should disable empty styles field instead of hiding it;
  3. IE script error when open Div dialog for editing.

comment:11 Changed 4 years ago by alfonsoml

  • Keywords Review+ added; Review? removed

comment:12 Changed 4 years ago by garry.yao

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

I'm committing this as a new feature since it's actually an alignment with v2.
Fixed with [5050] on 3.2.x branch.

comment:13 Changed 4 years ago by garry.yao

  • Component changed from UI : Dialogs to Core : Styles
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy