#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)
Change History (21)
comment:1 Changed 16 years ago by
| Keywords: | 3.0RC added |
|---|---|
| Status: | new → assigned |
Changed 16 years ago by
| Attachment: | 3564.patch added |
|---|
Changed 16 years ago by
| Attachment: | resizer.gif added |
|---|
comment:2 Changed 16 years ago by
| Keywords: | Review? added |
|---|
comment:3 Changed 16 years ago by
comment:4 Changed 16 years ago by
| 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 16 years ago by
| Attachment: | 3564_2.patch added |
|---|
Changed 16 years ago by
| Attachment: | resizer_rtl.gif added |
|---|
comment:5 Changed 16 years ago by
| 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:
- I can't use position: absolute in the bottom space for the other element, because position: absolute does not work in <td>.
- 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.
- 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.
- IE6 again, in Quirks mode, would have the element path occupy a full line, but no horizontal offset for the resizer.
- 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 16 years ago by
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 16 years ago by
| 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 16 years ago by
| Attachment: | 3564_3.patch added |
|---|
comment:8 Changed 16 years ago by
| Keywords: | Review? added; Review- removed |
|---|
Nasty browser bug there. Added a workaround.
comment:9 follow-up: 10 Changed 16 years ago by
Line 187 in theme.js is part of the Safari fix - if it's removed then the browser will crash, again.
comment:10 Changed 16 years ago by
| 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 16 years ago by
| Attachment: | 3564_4.patch added |
|---|
comment:11 Changed 16 years ago by
| 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 16 years ago by
| 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 16 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Fixed with [3534].
Click here for more info about our SVN system.
comment:14 Changed 16 years ago by
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
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.