Opened 10 years ago

Closed 10 years ago

#2799 closed Bug (duplicate)

V3: The skin file make specific dialog stuff by using the global CKEDITOR.dialog

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review- Cc:

Description (last modified by Garry Yao)

Into the default skin.js file, we can find references to CKEDITOR.dialog.setMargins and CKEDITOR.dialog.on( 'resize' ). This is bad, because it assumes that all editor instances are using a single skin, which is false.

All settings must go inside each single editor instance. Also, events like "resize" should be fired by each instance. The event could be named "dialogResize", for example.

Attachments (2)

2799.patch (12.4 KB) - added by Garry Yao 10 years ago.
2799_2.patch (17.3 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 10 years ago by Garry Yao

Description: modified (diff)

Depends on: #2802.

Changed 10 years ago by Garry Yao

Attachment: 2799.patch added

comment:3 Changed 10 years ago by Garry Yao

Keywords: Review? added

comment:4 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Review- because the patch doesn't work.

The margin values are there for the mouseMoveHandler()s in the dialog drag/drop and resize logic. It is used in calculating the bounds for the dialog's position and sizes, relative to the browser window.

Specifically, the margin values represent areas that logically belong to the dialog, but visually shouldn't be considered part of the dialog (e.g. dialog shadows). Those areas should be excluded from calculations of the dialog's bounding area.

Now, when we look at the patch, the margin values are saved inside the dialog object, which are under the editor object. That part is correct. However, the mouseMoveHandler()s in drag/drop and resize part of the code are still using the margin values from CKEDITOR.dialog._ - which no longer exists. The net result is the dialog bounding area calculations are wrong after the patch. e.g. If you drag a dialog around, you'll notice the bounding area now includes the shadows.

Some other minor problems with the patch:

  1. _source/plugins/dialog/plugin.js, line 938: This line shouldn't exist.
  2. _source/skins/default/skin.js, line 42, 44, 50: Wrong bracket style.
  3. _source/skins/default/skin.js, line 44-110: Wrong indent level (should be 1 tab instead of 2).

Changed 10 years ago by Garry Yao

Attachment: 2799_2.patch added

comment:5 Changed 10 years ago by Garry Yao

Revised patch with the following fixes:

  • Now each skin are sensitive to it's belonged editor instance.
  • A init[Partname]Part method is added to skin definition to support for apply specific behaviors only to skin parts.
  • Adding dialogBeforeShow and dialogBeforeResize events to editor.
  • Introduce a new method: CKEDITOR.tools.capitalize.

comment:6 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Resolution: duplicate
Status: assignedclosed

This one will be fixed at #3191, with a much simpler solution possibly.

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