Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#3564 closed New Feature (fixed)

Implement resize handle

Reported by: Martin Kou Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: 3.0RC Review+ Cc:

Description

A planned feature in CKEditor 3.0 is that we should have a resize handle at the bottom right corner of the editor.

Attachments (7)

3564.patch (6.4 KB) - added by Martin Kou 10 years ago.
resizer.gif (65 bytes) - added by Martin Kou 10 years ago.
3564_2.patch (7.4 KB) - added by Martin Kou 10 years ago.
resizer_rtl.gif (66 bytes) - added by Martin Kou 10 years ago.
3564_3.patch (9.7 KB) - added by Martin Kou 10 years ago.
3564_4.patch (9.5 KB) - added by Martin Kou 10 years ago.
3564_4_ref.patch (1.9 KB) - added by Tobiasz Cudnik 10 years ago.
Tested on FF3, IE8, IE8(6), Safari3.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by Martin Kou

Keywords: 3.0RC added
Status: newassigned

Changed 10 years ago by Martin Kou

Attachment: 3564.patch added

Changed 10 years ago by Martin Kou

Attachment: resizer.gif added

comment:2 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:3 Changed 10 years ago by Martin Kou

I'm not running langtool for the patch since it makes the patch unnecessarily large and hard to review (it may cause lots of conflits later).

Instead I'll just put a reminder here that if I were to commit the patch I need to run langtool first.

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • Just like the elementspath plugin, the resize plugin should listen for the "themeSpace" event (with a higher priority, like 100), appending the HTML for it. The appended HTML may set an id to the element, which gets later, in another event, used to initialize the element events. (there are performance benefits on it)
  • The plugin code is calling editor.getThemeSpace( 'contents' ).getAscendant( 'table' ), which is ok for now, but may be potentially broken for custom themes. We could instead introduce the CKEDITOR.editor.prototype.getResizable() function, which returns the proper object for that. This function is to be called by the resize function as well.
  • The elementpath styles changes, other than bad, should not be needed. You may try using a negative margin-top for the resize handler to make it move to the right place.
  • An RTL solution must also be available.
  • Let's try to have a standard for the configuration names, using an underscore after the prefix name. In this case we would have resize_minWidth, resize_minHeight, etc. (note the lowercase after the underscore).
  • Let's have the minWidth setting set to 750 by default. Lower values makes the editor ugly with the default toolbar.

Changed 10 years ago by Martin Kou

Attachment: 3564_2.patch added

Changed 10 years ago by Martin Kou

Attachment: resizer_rtl.gif added

comment:5 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

I've considered the changes in elementspath.css for a while, but they're actually correct.

The way the element path bar occupies 100% of the bottom space makes it extremely difficult to lay out other elements in the bottom bar correctly. I've done extensive tests on that:

  1. I can't use position: absolute in the bottom space for the other element, because position: absolute does not work in <td>.
  2. If I simply use float:right, most browsers would let the elements path to occupy one very thin line and the resizer to occupy a whole line - thus the bottom bar's height is modified.
  3. IE6, on the other hand, would interpret the element path to occupy a full line, and the resizer would strangely get some horizontal offset as well.
  4. IE6 again, in Quirks mode, would have the element path occupy a full line, but no horizontal offset for the resizer.
  5. So taking into point 2, 3, 4 into account - If I were to use negative margins to get the resizer to work, I would have to write different cases for i. FF/Safari/Opera, ii. IE standards mode; iii. IE quirks mode... probably more. So the negative margin fix is a unreliable hack.

So instead, I looked to how the positioning is handled at the editor toolbar. Individual toolbars only occupy enough space for themselves, and so more items can be easily added with CSS float and they'll be positioned correctly. And that's why I see the real problem in the bottom box is the 100% width of the element path box. If it only occupies enough space for itself, then I can have a very simple solution for the resizer that works for all browsers across quirks and standards modes.

comment:6 Changed 10 years ago by Martin Kou

The other items are fixed. Note that to test RTL resizing, the test document itself must be RTL as well (e.g. put a dir="rtl" at the <html> tag). Otherwise the resizing UI effect would look wrong.

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The code looks pretty good now. The elementspath.css changes are also good; much better than the first patch.

The only problem I've found is Safari (3 and 4 on Windows). The resizer is not only not working with that browser, but it's also totally breaking the editor, which is quite a serious issue.

Changed 10 years ago by Martin Kou

Attachment: 3564_3.patch added

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

Nasty browser bug there. Added a workaround.

comment:9 Changed 10 years ago by Martin Kou

Line 187 in theme.js is part of the Safari fix - if it's removed then the browser will crash, again.

comment:10 in reply to:  9 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The new patch avoid the resizer breaking Safari, but it also makes it work pretty bad in that browser. We may even make it pass for now, if you really can't find a better solution for it, opening a dedicated ticket for it.

I've found out that the culprit here is the outer.setStyle( 'width', width ) call. All the rest works. So, it looks like the display:hidden trick (if really needed) needs to be applied to that call only. It's not needed for the "height" change.

Line 187 in theme.js is part of the Safari fix - if it's removed then the browser will crash, again.

So please add a comment in the code about it, otherwise it looks really senseless.

Changed 10 years ago by Martin Kou

Attachment: 3564_4.patch added

comment:11 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

The line that actually kills performance is L188 in theme.js. But it is needed to force WebKit into rendering the page without the table or else the display hack would be ineffective.

I've tried to change the table layout algorithm by setting table-layout to fixed, but that didn't help.

So, proposing a simplified patch, which removes unnecessary setStyles() calls and also made L188 to be WebKit only so to eliminate the performance penalty in other browsers.

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Ok... so, when committing, please remember:

  • Run langtools (before committing).
  • Open a new ticket for the webkit performance issue (targeted for the 3.0 for now).
  • Possibly open a ticket at webkit about this blocking issue.

comment:13 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3534].

Click here for more info about our SVN system.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3564_4_ref.patch added

Tested on FF3, IE8, IE8(6), Safari3.

comment:14 Changed 10 years ago by Tobiasz Cudnik

I've attached a reference patch which fixes resize on new skin branch from #3503, which introduces some DOM structure changes. It should be applied only on this branch, it won't work on trunk.

Branch URL: https://svn.fckeditor.net/CKEditor/branches/features/kama

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