Opened 11 years ago

Closed 10 years ago

#2862 closed New Feature (fixed)

plugin:maximize porting from v2

Reported by: Garry Yao Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: Confirmed Oracle HasPatch Review+ Cc: Senthil

Description

Related v2 tickets:

#174
"Maximize the editor size" button can't handle relative DIVs
#762
Outside Flash goes over the maximized editor
#1211
Opera & Safari: Editor page can be scrolled even if editor size is maximized
#1538
Maximize command not working in IE
#2002
Maximize editor button is broken
#2233
Maximize editor screen don't show properly
#2474
An editor contained in a Div with overflow:auto can't be maximized.
#2825
When 'Maximize/Minimize' is clicked after maximizing, it does not restore to original size
#2862
plugin:maximize porting from v2
#3433
Restoring from maximized does not work across mode switches in IE8 Standards mode.
#3645
[IE] Editor size change after maximize
#3654
Resize handle should not be useable in maximized mode.
#3916
Maximize does not enlarge editor width when width is set.
#3931
Maximize maximizes to width setting, not screenwidth, when option is given with CKEDITOR.replace
#4023
[Opera] Maximize plugin
#4024
Maximize button is not translated
#4028
Maximize control's tool tip is wrong once it is maximized
#4214
Maximize bugs
#4258
Editor area chopped off on maximize for Arabic language in IE7 and IE8
#4459
CKEditor maximized appears below select element in IE6
#4509
Adding config for maximize on startup
#4519
[IE]Unable to maximize without editor focus
#4541
Maximize adds extra space on non-Kama skins
#4602
IE8 XP SP3 - maximize on long pages cuts off toolbar
#4835
Maximized editor wider than view port
#4854
[FF3] Maximize layout broken in RTL quirks
#4918
FF: Switching to source view when editor maximized
#4923
Maximize layout is broken when main page scrolled
#4958
Combos texts show text cursor when maximized
#5034
Maximize button does not work properly
#5058
Pressing tab when editor is maximized
#5059
Pressing tab when editor is maximized
#5149
Cursor dissapears after maximize in Firefox 3.6
#5178
[Nightly 3.2 Ajax Demo] Maximize button is not rendering the editor correctly
#5265
v2 skin: Editor size would not restore back correctly after Maximize
#5329
maximize will error if button is not displayed
#5579
Rich combos are not re-positioned when going to maximize mode
#5580
Maximize does not work properly in the Office 2003 and V2 skins
#5724
[Firefox] Maximize one editor instance make other instances uneditable
#5740
editor does not retain position in maximize mode
#5752
Maximize with multiple editors breaks UI in Firefox 3.6
#6057
Error when clicking beneath the body of a maximized editor
#6284
Caret placement impossible after maximize and then minimize
#6339
Plugin autogrow and Maximize
#6388
sharedSpaces don't disable maximize
#6467
setState(CKEDITOR.TRISTATE_DISABLED) on 'mode' impossible for maximize plugin
#6535
Combo box list stays after editor is maximized.
#6559
Maximize plugin @ Opera/Mac
#6695
[safari] Popup a dialog at maximize mode and then minimize cause weird styling
#6747
Maximize Issue with Toolbar in Firefox 3.x and IE 8
#6895
Pasting plain text in full-screen (maximized) view does not work
#6904
Safari: Styles disappear before editor goes to maximize mode
#6927
Pasting into the URL field does not work in Google Chrome with CKEditor in 'maximized' mode
#6947
CKEditor maximize firefox bug
#7038
Possibility to automatically switch toolbars on a maximize/minimize of ckeditor
#7253
Maximize functionality in Firefox fails without doctype tag
#7284
Maximize control collapses the editor
#7771
'Maximize' in container with Opacity makes container disappear (FF)
#7855
Clicking 'maximize' shows blank screen in Firefox when the editor is opened inside jQuery UI Dialog
#8271
CKEditor toolbar becomes invisible when using Tab key and Maximize toolbar button
#8307
[iOS] Maximize is broken
#8587
IE7 maximize long delay
#8930
Maximize does not work
#9065
Maximize to percent
#9253
Resize feature should be disabled for maximized editor
#9269
Maximize plugin test crashes IE7
#9311
Vertical scroll bar not appearing in maximized editor with autogtrow enabled
#9319
FF: Editor not expanding to Maximum size, when we click Maximize button after focusing in editor body
#9320
Editor does not autogrow after entering content in Maximize mode
#9321
FF: Vertical scroll bar & toolbar missing in maximized editor with autogtrow enabled
#9374
Opera: Editor displays off-screen in autogrow sample in maximize mode
#9465
Misplaced panels when editor is being maximized
#9680
The "Maximize" feature should not be a toolbar button
#9883
[FF] Maximized and minimized divarea leaks
#9886
[IE8-10] No scrollbar in maximized editor with autogrow enabled
#10084
[Android] Problems with Maximize plugin in zoomed-in page
#10326
Maximize button removes input[name=style] from form
#11020
Same config, different result between minifief and maximized version
#11169
Maximize not work
#11410
[FF] jQuery sample, maximize + minimize framed editor allows to edit whole page
#11436
Maximize plugin with shared space doesen't work
#11745
Maximize should use position:fixed instead of changing entire page styling
#12163
Maximize (and resize) plugin and shared spaces incompatibility
#12357
[IE8] Call maximize command fire resize event twice
#12398
Maximize-Button doesn't work in instances without a title
#12747
IE: Dropdowns become disabled when in maximize mode.
#13033
Maximize pluging would not work with floating tool plugin in firefox mozilla
#13046
Dropdown not working in maximize mode with RTL body
#13190
Maximize problems
#13403
Maximize tool issue - after maximizing, if browser back button used, page layout is messed up
#13481
Nested dialog hides parent dialog on Maximized editor.
#14695
CKEditor maximize plugin issue
#16778
Maximize hides the toolbar, can't minimze.
#16856
Maximize not working on iphone
#16983
Maximize does not work s expected when classic and inline editors are mixed on the same page

Attachments (15)

2862.patch (16.2 KB) - added by Garry Yao 11 years ago.
2862_2.patch (15.8 KB) - added by Garry Yao 11 years ago.
2862_3.patch (17.7 KB) - added by Garry Yao 11 years ago.
2862_4.patch (13.9 KB) - added by Garry Yao 11 years ago.
2862_5.patch (13.4 KB) - added by Garry Yao 11 years ago.
2862_6.patch (12.4 KB) - added by Garry Yao 10 years ago.
2862_7.patch (12.4 KB) - added by Garry Yao 10 years ago.
2862_8.patch (13.7 KB) - added by Garry Yao 10 years ago.
2862_9.patch (13.3 KB) - added by Garry Yao 10 years ago.
2862_10.patch (14.8 KB) - added by Garry Yao 10 years ago.
2862_11.patch (14.3 KB) - added by Garry Yao 10 years ago.
2862_12.patch (13.9 KB) - added by Martin Kou 10 years ago.
2862_13.patch (14.0 KB) - added by Martin Kou 10 years ago.
2862_14.patch (14.2 KB) - added by Martin Kou 10 years ago.
2862_15.patch (14.9 KB) - added by Martin Kou 10 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 Changed 11 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 11 years ago by Garry Yao

Attachment: 2862.patch added

comment:2 Changed 11 years ago by Garry Yao

Keywords: Confirmed Review? added

Proposed patch using CSS to fit viewport instead of size caculation approach back in v2.

comment:3 Changed 11 years ago by Garry Yao

Summary: plugin:fullscreen porting from v2plugin:fitwindow porting from v2

comment:4 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Version: CKEditor 3.0 Beta

I'm having problems to apply this patch. Some of the files are not matching the current trunk ones. Even the "\ No newline at end of file" message ended up at the bottom of the plugin.js file.

Can you please update your local copy and create a new patch? Is it possible to use TortoiseSVN to create the patch?

Changed 11 years ago by Garry Yao

Attachment: 2862_2.patch added

comment:5 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Sorry, I assumed we're using prototype branch for these new features, which is wrong. The text "\ No newline at end of file" seems to be unified diff format, please correct me. I'm using subclipse which is one sister product of TortoiseSVN and I believed they both generated unified format.

comment:6 Changed 11 years ago by Garry Yao

Keywords: Review? removed

Changed 11 years ago by Garry Yao

Attachment: 2862_3.patch added

comment:7 Changed 11 years ago by Garry Yao

The updated patch fix the following issues:

  1. Optimize codes when moving editor document.
  2. Simplify codes when save/restore editor document.
  3. Update to include the patch for #2907 and part of the patches for #2912.
  4. Add startupFitWindow config, currently not working on IE due to the bug #2816.

comment:8 Changed 11 years ago by Garry Yao

Keywords: Review? added

The current patch use a different approach with codes in v2, which lift the editor container up to be the first child of BODY element and use pure CSS to maximize the editor to fit viewport, compare with original approach from the following aspects:

  • Less style touching on host document which make less necessarily backup, but introduce changes to the document structure.
  • Pure css layout make no need for no attaching to onresize events and reduced the size calculation, which improve PF on host window resize.
  • Due to a nasty bug of FF and Webkit on Iframe reloads, reload the editing block is a mandatory workaround, which introduce PF decrease on these browers when fit to window.
  • While moving the editor container around, still keep the original form field untouched, which should make commands like save works fine as well.

comment:9 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • If we are looking to change the toolbar button name to "Maximize", then we should also rename the plugin and command name to "maximize". It makes more sense than "fitwindow" actually.
  • I've noted that you are proposing a completely new thing here. The V2 code is already well tested and works well. I've also check the size of each implementation and V2 gives me 2.4KB while the patch gives 3.9KB. The extra 1.5KB brings me many doubts on the real benefit of the new approach.
  • We can avoid the getTextArea function (which should be named getTextarea instead). Actually, it's not a big issue, and even a benefit, to have some API way to interact with the source textarea. You can add a "editor.sourceField = textarea" right before the "editor.fire( 'mode' )" call in the sourcearea plugin.
  • The setMode function should not suffer changes. You are using that to reload the contents. The "loadSnapshot" feature is being proposed at the latest patch in #2763, and that should be the way to go instead.
  • Generally, the strict equality operator (===) is being overused.
  • Be sure to have it tested over all browsers, under strict and quirks mode.

plugin.js:

  • The file contents was duplicated for me. Martin had this issue recently, so you may consult him.
  • I still have the "\ No newline at end of file" string in this file. There is something wrong with the patch. You will find the same thing in the Trac preview for this patch. Again, you should really use TortoiseSVN. Be sure it's the best tool for SVN.
  • The "onSave" and "onRestore" functions use a naming convention which makes them look like event related functions. It looks like they can be safely and simply named "save" and "restore".
  • Moving the editor back to the position right after the replaced element works, but, to be safer, I would save a reference to the previous node when maximizing, and then just move the editor after it when restoring. No one knows if that structure will have changes in the future.
  • Line 14: You have CKEDITOR.document for that. Also, initialized variables should go into separated lines:
var doc = CKEDITOR.document,
    bodyEl = doc.getBody();
  • Line 22: CKEDITOR.tools.extend ??? It's not needed there. Go ahead with "this.XXX".
  • Line 105: The jQuery chaining coding style is really not acceptable here. It's also not recommended to use codes for variable names. You can safely name that var "contents", instead of "ctEl" (and btw, reuse it at line 110).
  • Line 220: !( a === B ) !!! What about ( a !== b )? Also, you have an unneeded "if" chain. You can do a single if there. Also (again), there is no need to use the strict equality operator.
  • Line 312: The CKEDITOR.tools.setTimeout function should be used only if we really need one of its features. In this line we can safely use setTimeout purely.
  • Line 325: CKEDITOR.tools.extend is being overused. You can do "CKEDITOR.config.startupFitWindow = false" directly there.

There are still a few other things that could be improved and simplified, but we need to move forward with this. We can make a code lifting in the future.

Changed 11 years ago by Garry Yao

Attachment: 2862_4.patch added

comment:10 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed
Summary: plugin:fitwindow porting from v2plugin:maximize porting from v2
  • Update the patch according to Fred's review points, except for:
  • I've noted that you are proposing a completely new thing here. The V2 code is already well tested and works well. I've also check the size of each implementation and V2 gives me 2.4KB while the patch gives 3.9KB. The extra 1.5KB brings me many doubts on the real benefit of the new approach.

Thanks for reminding me on the footprint measurement, yes it add extra bytes all due to the nasty bug of iframe content lose, hopefully browsers could fix this bug later so the patch would be reduced a lot, while the valuable things bring by this approach would be it utilize a 'semantic correct' way to turn a element into fullscreen, without imposing changes to all other elements below document & body , it could avoid former bugs like #1211 and #2474.

  • The setMode function should not suffer changes. You are using that to reload the contents. The "loadSnapshot" feature is being proposed at the latest patch in #2763, and that should be the way to go instead.

I try to accomplish this with the proposed loadSnapshot, but result in founding the head part of the document unable to reload which contains document css definition, so temporarily I would be reluctance to change editor.setMode.

  • Currently tested on two modes of all supportable browsers.
  • The patch also proposing the following fix after discussing with Martin:

Add a forceSelectionCheck function to _source/plugins/selection/plugin.jswhich helps on manually check selection change.

comment:11 Changed 11 years ago by Garry Yao

Oops, 'supportable browsers' should be revised into 'compatible browsers'...

comment:12 Changed 11 years ago by Frederico Caldeira Knabben

Priority: HighNormal

comment:13 Changed 11 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • I'm having problems to apply this patch. I have a "The patch seems outdated!" error with the editingblock/plugin.js.
  • Please use "editor.sourceField", as specified.
  • When pointing out that setMode should not be changed, means that another solution must be found.
  • All occurrences of the strict equality operator must be changed to a simple equality operator (2 occurrences).
  • CKEDITOR.config.plugins items are supposed to be alphabetically ordered.
  • "recover" is to be renamed to "restore".
  • Line 103: The jQuery chaining coding style is really not acceptable here.
  • "forceSelectionCheck" is to be renamed to "checkSelection".

Changed 11 years ago by Garry Yao

Attachment: 2862_5.patch added

comment:14 Changed 11 years ago by Garry Yao

Keywords: Review? added; Review- removed

Updated patch according to the review points again. The unpatch-able problem due to one defect of my patch system when appending the no new line... indicator which behaviored different with Tortoise SVN. I've corrected it manually and tested with Tortoise SVN, so in this sense, Tortoise SVN did a better job.

comment:15 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

The patch works well. I've tested it with IE, Opera, Safari, Firefox; under Standards mode and Quirks mode; and with static positioning and absolute positioning. It worked for all cases. But there're still some minor style problems to be fixed in the code:

  1. Please follow the conventions we already have and use this._.restore instead of this._restore for private variable. (_source/plugins/maximize/plugin.js, line 25).
  2. Please don't add a space between "function" and "(". (_source/plugins/maximize/plugin.js, lines 48, 62, 84, 96, 112, 131, 158, 168, 180, 207, 220) - a regular expression substitution should fix that easily.
  3. Missing spaces in brackets. (Lines 19, 158, 338).
  4. I'm not really sure, but is the hack in Lines 344-347 still needed after your fix in #2927?
  5. The toolbar's button arrangement is now changed, please place your maximize button just before the show blocks button in your new patch.

Changed 10 years ago by Garry Yao

Attachment: 2862_6.patch added

comment:16 in reply to:  15 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to martinkou: I've updated according to Martin's review points except:

  1. I'm not really sure, but is the hack in Lines 344-347 still needed after your fix in #2927?

I believe this is still legal, it's a special case when register event listener in the same event handler which should be avoided, so without it the codes would somehow equal to:

	editor.on( 'mode', function()
	{
		editor.on( 'mode', function(){
			
			//Below function will executed twice. 
			alert('');
		} );
	} );

comment:17 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • In Firefox, I'm having an error when maximizing for the first time.
  • In IE, the Elements Path bar is not visible when maximize.
  • For z-index, you should use the baseFloatZIndex setting, instead of hardcoded "10". (If it conflicts with other floating stuff like dialogs and panels, it's not your fault and those things must be corrected separately).
  • The thing pointed by Martin (point 4) also sounds strange for me. Try removing the listener as the first thing in that function and check if it helps. Btw, you can call event.removeListener() directly to remove the current listener function.

Regarding the above IE issue, I noted that you are always using 100% to the width and height. You may try CKEDITOR.dom.window::getViewPaneSize instead and see the results (you should check it if the page has margins and padding also.

Changed 10 years ago by Garry Yao

Attachment: 2862_7.patch added

comment:18 in reply to:  17 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck:

  • In Firefox, I'm having an error when maximizing for the first time.

I've accomodate it with the undo/redo system which caused the error.

Regarding the above IE issue, I noted that you are always using 100% to the width and height. You may try CKEDITOR.dom.window::getViewPaneSize instead and see the results (you should check it if the page has margins and padding also.

This IE issue is addressed with some hacks in the codes, while relaying on viewport calculation is something accurate, but it introduced extra effort stated in my previous comment.

comment:19 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

This new patch is not working. It's simply moving the editor to the top of the document, with 100% width.

Changed 10 years ago by Garry Yao

Attachment: 2862_8.patch added

comment:20 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Just now the patch was influenced by the editor dom structure change of [3208].

comment:21 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

This patch is touching the "save" plugin, by duplicating its contents. You should really check everything you are including in the patch before creating it.

Changed 10 years ago by Garry Yao

Attachment: 2862_9.patch added

comment:22 in reply to:  21 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to fredck:

This patch is touching the "save" plugin, by duplicating its contents. You should really check everything you are including in the patch before creating it.

Yes, It's a bad mistake, the new patch has also introduced view-port calculation logic for IE and Opera since it's too hard to make table layout correct by pure CSS, logic for other browsers are remain the same.

comment:23 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It's not working with IE and Opera.

Changed 10 years ago by Garry Yao

Attachment: 2862_10.patch added

comment:24 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:25 Changed 10 years ago by Martin Kou

Cc: Senthil added
Keywords: Oracle added
Priority: NormalHigh
Version: SVN (CKEditor)

Senthil just phoned me, they're requesting to include this feature into the RC.

comment:26 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Review- because:

  1. Button status update is wrong in all browsers.
  2. It crashes both IE6 and IE7.

comment:27 in reply to:  26 Changed 10 years ago by Garry Yao

Replying to martinkou:

  1. It crashes both IE6 and IE7.

I've located the criminal was a 'removeAttribute' call on 'html' element, which should be easy to avoid, I'll come with a new patch after then.

Changed 10 years ago by Garry Yao

Attachment: 2862_11.patch added

comment:28 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:29 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: HasPatch added; Review? removed

Please hold on for a while with this ticket, while we're still considering some changes on the editor structure that could impact on this patch.

comment:30 Changed 10 years ago by Frederico Caldeira Knabben

Priority: HighNormal

comment:31 Changed 10 years ago by Martin Kou

Owner: changed from Garry Yao to Martin Kou
Status: assignednew

Changed 10 years ago by Martin Kou

Attachment: 2862_12.patch added

comment:32 Changed 10 years ago by Martin Kou

Keywords: Review? added

The patch has included a fix for #2474 and added some logic in the toolbar plugin to make the editing area resize with toolbar collapses.

comment:33 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It's not working on IE7 in both standards and quirks, and also IE6 in quirks. I'll check other browsers also.

comment:34 Changed 10 years ago by Frederico Caldeira Knabben

I've tested all browsers now, in standards and quirks:

  • Works well with Firefox 2 and 3.
  • Broken with IE6 in quirks and IE7 in both modes.
  • Works well with IE8 in standards. Broken in quirks though.
  • Broken with Safari (3 and 4).
  • Broken with Opera.

The toolbar collapser trick works well in all browsers though.

comment:35 Changed 10 years ago by Garry Yao

Another issue, scroll position is not restored correctly when switch back from full-screen.

Changed 10 years ago by Martin Kou

Attachment: 2862_13.patch added

comment:36 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:37 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I can confirm the following issue with all browsers:

  1. Maximize the editor;
  2. Switch to source; (the maximize button looses its state)
  3. Switch back to wysiwyg;

It's not possible anymore to restore the editor size. The editor blocks the rest page, so it's quite serious.

comment:38 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

Changed 10 years ago by Martin Kou

Attachment: 2862_14.patch added

comment:39 in reply to:  35 Changed 10 years ago by Garry Yao

Replying to garry.yao:

Another issue, scroll position is not restored correctly when switch back from full-screen.

This problem still exist, beside, the text area height is incorrect after maximize in IE6.

comment:40 Changed 10 years ago by Martin Kou

The bug with the source area text area exists in trunk, so it's not caused by my patch. I've opened #3400 for that.

I've noticed some slight inaccuracies when IE is restoring the horizontal scroll bar position in the outer window. That can be fixed. But I'm not sure if that's the same error you're seeing?

Changed 10 years ago by Martin Kou

Attachment: 2862_15.patch added

comment:41 Changed 10 years ago by Martin Kou

I've talked about Garry about the scrolling bug he mentioned. It concerns the editing area rather than the outer window.

To reproduce:

  1. Copy and paste the same row over and over in replacebyclass.html until you have something like 500 lines or more.
  2. Maximize the editor.
  3. Scroll to the bottom.
  4. Select the line at the bottom.
  5. Restore the editor.
  6. Now the selection is scrolled away from the viewable area.

Garry proposed to add clientWidth and clientHeight in 2862_11.patch, but that causes another similar bug. The real fix is to use CKEDITOR.dom.element::scrollIntoView(). The scrollIntoView() implementation we have doesn't handle horizontal scrolls. But it should be enough for most cases.

comment:42 in reply to:  41 Changed 10 years ago by Garry Yao

Replying to martinkou:

Garry proposed to add clientWidth and clientHeight in 2862_11.patch, but that causes another similar bug. The real fix is to use CKEDITOR.dom.element::scrollIntoView(). The scrollIntoView() implementation we have doesn't handle horizontal scrolls. But it should be enough for most cases.

I would suggest to have both approach implemented for scrolling position, the proposed solution assume there's always been a selection on the document which is not true, counting clientWidth/clientHeight is about resolve this case, which just adding few lines of code.

comment:43 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Please run the langtool before committing.

comment:44 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: newclosed

Fixed with [3406].

Click here for more info about our SVN system.

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