plugin:maximize porting from v2

[Garry Yao]

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

[Garry Yao]

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

[Frederico Caldeira Knabben]

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?

[Garry Yao]

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.

[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.

[Garry Yao]

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.

[Frederico Caldeira Knabben]

• 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.

[Garry Yao]

• 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.

[Garry Yao]

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

[Frederico Caldeira Knabben]

• 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".

[Garry Yao]

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.

[Martin Kou]

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.

[Garry Yao]

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

4. 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('');
		} );
	} );

[Frederico Caldeira Knabben]

• 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.

[Garry Yao]

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.

[Frederico Caldeira Knabben]

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

[Garry Yao]

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

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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.

[Frederico Caldeira Knabben]

It's not working with IE and Opera.

[Martin Kou]

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

[Martin Kou]

Review- because:

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

[Garry Yao]

Replying to martinkou:

2. 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.

[Frederico Caldeira Knabben]

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.

[Martin Kou]

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.

[Frederico Caldeira Knabben]

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

[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.

[Garry Yao]

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

[Frederico Caldeira Knabben]

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.

[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.

[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?

[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.

[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.

[Frederico Caldeira Knabben]

Please run the langtool before committing.

[Martin Kou]

Fixed with [3406].

Click here for more info about our SVN system.