Ticket #1659 (closed Bug: fixed)

Opened 2 years ago

Last modified 3 months ago

Setting DocType to Standards Compliant Collapses <body> in IE

Reported by: Scott Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.1
Component: General Version:
Keywords: Confirmed Oracle IE Review+ Cc: Senthil

Description

When you set FCKConfig.DocType to a standards compliant type, the min-height: 100% css trick on the body no longer works.

This means the body collapses to the size of the elements contained within the body.

IE only.

Attachments

1659.patch (1.2 KB) - added by fredck 2 years ago.
1659_1.patch (1.3 KB) - added by alfonsoml 2 years ago.
Proposed SVN patch
1659_2.patch (1.2 KB) - added by alfonsoml 2 years ago.
Revised patch
1659_3.patch (0.6 KB) - added by martinkou 12 months ago.
1659_4.patch (1.4 KB) - added by martinkou 11 months ago.
1659_5.patch (3.4 KB) - added by martinkou 10 months ago.
1659_6.patch (3.1 KB) - added by martinkou 10 months ago.
1659_7_ref.patch (2.3 KB) - added by tobiasz.cudnik 9 months ago.
1659_8.patch (2.0 KB) - added by garry.yao 4 months ago.
1659_9.patch (1.8 KB) - added by garry.yao 3 months ago.

Change History

  Changed 2 years ago by alfonsoml

  • keywords Confirmed IE added

I bet that there was a ticket filed about this previously and I did close it as WFM because I couldn't reproduce it with the current nightly (but it was buggy in the previous released version), but now I can't find it and for sure the behavior of IE is really unpleasant.

Steps to reproduce:

  1. set for example 
    FCKConfig.DocType = '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">' ;
  2. to see more easily the weird behavior add to fck_editorarea.css this rule 
    body
{
	background-color: #AAA;
	padding: 5px 5px 5px 5px;
	margin: 0px;
	border:2px solid green;
}
  1. load sample01.html in IE.

Symptoms:

  1. the green border is just on the first line of content instead of covering all the text area.
  2. move the mouse up and down. it only change to a text cursor over that first line.

And more important to understand why it is severe:

  1. Make the editor initialize with empty contents (oFCKeditor.Value = '' ;).
  2. Now try to click in the editor area to start writing.

It only works if you click on the first line and it's really strange for any user.

  Changed 2 years ago by Scott

I was messing around today on an IE 6 development machine today. But how about using IE proprietry expressions to fix this... something similar to this perhaps?

body {

height: expression(document.documentElement.clientHeight);

}

It does make a scrollbar always show, but perhaps subtracting something like the margins or padding off the client height could fix this?

I am not exactly sure why the scroll bars appear when using this code.

  Changed 2 years ago by alfonsoml

css expressions are expensive with regards to CPU, so I would rather avoid them unless there is no other solution.

Changed 2 years ago by fredck

  Changed 2 years ago by fredck

  • keywords HasPatch added
  • milestone set to FCKeditor 2.6

The attached patch should give similar results as Scott solution with expression. I've set the height to 90% because, as Scott described, the box model on strict mode will force the scrollbars to appear. With 90%, only a small part of the editing area will not be clickable, but the end user should not even note that in the every day use of the editor.

I've used some CSS hacks to make the selectors visible to IE6 and IE7 only.

I'm not yet asking it for review, as I would like to have your impressions first.

  Changed 2 years ago by Scott

My impression is ... yuck! I think that setting it to 90% is just a temporary hack which doesnt fix the problem. We use a lot of drag and drop stuff with the editor and it becomes really evident when the bottom 10% is not a drop target.

I think it may be possible to fix the issue by specifying a negative margin and setting the height to 100%. I have done a little bit of testing with this, but have not managed to come up with a solution which will work with arbitrary paddings on the body.

I have however developed a solution using expressions (yes I know they are expensive). But they seem to be the only way to get it to work with totally arbitrary CSS. 

html
{
	height: expression(document.documentElement.clientHeight);
}

body
{
	height: expression(document.documentElement.clientHeight 
			- parseInt(this.currentStyle.paddingTop)
			- parseInt(this.currentStyle.paddingBottom));
}

  Changed 2 years ago by fredck

Expressions! yuck! yuck! one for each entry (just kidding :)

Do you have the the same results by changing the "html" selector body to "height: 100%"? If I recall it well, the body is making the scrollbars appearing, you would have less one yuck!.

In the other hand, to avoid expressions, I think a feasible solution would be listening to the resize event, and set the height style at that point. In this way we avoid making the browser recalculate it thousands of times while using the editor (potentially crashing IE, btw).

  Changed 2 years ago by alfonsoml

for me, it seems that the html rule isn't even necessary to set it to 100%. Just using 

body
{
	height: expression(document.documentElement.clientHeight 
			- parseInt(this.currentStyle.paddingTop)
			- parseInt(this.currentStyle.paddingBottom));
}

works fine, but I think that the best solution as pointed out by Fred is to listen to the onresize event and then manually adjust the height of the body because that way the code is executed only when needed. The problem with expressions is that apparently they are executed continuously, as soon as anything changes.

  Changed 2 years ago by Scott

Ok sure. I am no fan of CSS expressions either.

Sorry - I thought that the html CSS rule was required. With html{ height: 100% }, it shows a scroll bar. With no html rule, it works fine.

The following code appears to work. Feel free to integrate it into the code-base wherever you feel it fits, changing the function names. 

function AnInitializationFunction()
{
	FCKTools.AddEventListenerEx( FCK.EditingArea.Window, 'resize', TheResizeWindowFunction );
	TheResizeWindowFunction();
},
	
function TheResizeWindowFunction()
{
	var oBody = FCK.EditingArea.Document.body;

	var iHeight = FCK.EditingArea.Document.documentElement.clientHeight 
		- parseInt( oBody.currentStyle.paddingTop )
		- parseInt( oBody.currentStyle.paddingBottom );

	oBody.style.height = iHeight + 'px';
}

Cheers guys - happy new year.

  Changed 2 years ago by Scott

Note! Note! Note! I just realized...

Using the above function puts a style="height: xxx" on the body tag when full page editing!

So this probably needs to be taken out when converting to source.. and protection added if a body height pre-exists.

Changed 2 years ago by alfonsoml

Proposed SVN patch

  Changed 2 years ago by alfonsoml

  • owner set to alfonsoml

I've attached a patch based mostly on the last code, but it does use runtimeStyle so there are no problems with serialization.

It runs only if the document is in strict mode. Now that I think about it, the check should be done only when setting the event listener. I'll upload a new patch.

Changed 2 years ago by alfonsoml

Revised patch

  Changed 2 years ago by alfonsoml

  • keywords Review? added; HasPatch removed
  • status changed from new to assigned

The new patch avoids the continuous check of IsStrictMode

  Changed 2 years ago by Scott

If the contents of the editor are larger than the body height, the body element doesnt extend past the original window height. To replicate, just add a lot of empty paragraphs until a scroll bar appears. Then insert something like a table. Use the IE DOM inspector and inspect the body element. Move your mouse to the right of the table, and you will see the cursor becomes a pointer instead of a text caret.

If you click the editor when the cursor is a pointer, it scrolls the editor all the way to the top which is very frustrating.

Perhaps we need to look at scrollHeight when calculating this. The problem then is if we start using scrollHeight, then this function needs to be run every time the content changes height - which I assume is very difficult to detect. Otherwise, the height of body will be stuck at the height when the content was first loaded. 

var oBody = FCK.EditingArea.Document.body;
		
// Remove the height so that scrollHeight calculates correctly
oBody.runtimeStyle.height = '';

var iHeight = FCK.EditingArea.Document.documentElement.clientHeight 
	- parseInt( oBody.currentStyle.paddingTop )
	- parseInt( oBody.currentStyle.paddingBottom )
	;

if( iHeight > oBody.scrollHeight )
{
	oBody.runtimeStyle.height = iHeight + 'px';
}

  Changed 2 years ago by Scott

It is a pity we can't make the html element editable. It may have fixed the problem. Sadly, it doesn't work. 

oDoc.documentElement.contentEditable = true ;

  Changed 2 years ago by Scott

Ok I think I have a solution. You may or may not like it. It is a hack, but I have now done a fair bit of research on this.

What if we add the following CSS: 

body
{
	position: absolute;
	bottom: 0px;
	right: 0px;
	overflow-y: scroll;
}

Then remove the overflow statement at line ~190 of fck_1.js.

  Changed 2 years ago by Scott

I have been testing this method a fair bit now and it appears to be working ok.

The only problem is when the body has a margin. It pushes the scroll in from from the sides of the editor.

Can I suggest using this block of CSS, (perhaps with overflow: auto), and also running a bit of javascript which sets the runtime style margin to 0px, and adds the old margin to the runtime style padding?

  Changed 2 years ago by Scott

After days of working with this... I have a solution :D... check it out...

If you havent been reading my previous messages, it basically works like this:

1) Absolute position the body element. Then set the top, left, bottom, right to 0px. This is wierd but it works. This allows us to make the body have 100% height without it messing up when the height of the content is greater than that of the window.

2) The scroll bar is no longer on the document element. The scroll bar now appears on the body element. This is done by setting overflow to auto. We have to do this as a result of #1

3) Because we are now scrolling from the body, any margins move the scroll bars away from the sides of the editor. So we add any margins to the padding. This does not matter as on the body element, padding and margin are virtually the same as the body element doesnt have any parent nodes

4) Any CSS styles that set the height of the body must be muted out because they cause dramas with the bottom CSS attribute.

Could you guys please integrate this into fck at a location of your choice and test. 

	Initialize: function()
	{
		// These fixes are only for IE under strict mode
		if ( !FCKBrowserInfo.IsIE || FCKRegexLib.Html4DocType.test( FCKConfig.DocType ) )
		{
			return;
		}
	
		var oBody = FCK.EditingArea.Document.body;

		// Set the base CSS
		var arrStyles = {'position': 'absolute', 'top': '0px', 'right': '0px', 'bottom': '0px', 'left': '0px'};
		
		for(var strStyle in arrStyles)
		{
			oBody.runtimeStyle[strStyle] = arrStyles[strStyle];
		}
		
		var arrSides = ['Top', 'Right', 'Bottom', 'Left'];
		
		// Convert all margin to padding so the scroll bars appear at the right position
		for(var i = 0; i < arrSides.length; i++)
		{
			var strSide = arrSides[i];

			oBody.runtimeStyle['padding' + strSide] = parseInt( oBody.currentStyle['padding' + strSide] )
				+ parseInt( oBody.currentStyle['margin' + strSide] )
				+ 'px';
			
			oBody.runtimeStyle['margin' + strSide] = '0px';
		}
		
		// The height must be auto or it messes up the absolute bottom: 0px css attribute
		oBody.runtimeStyle.height = 'auto';
		oBody.runtimeStyle.minHeight = 'auto';
	}

  Changed 2 years ago by Scott

PS: I moved all the body CSS Styles here to be in JS so because they are only for IE.

  Changed 2 years ago by Scott

Sorry guys, I had some cached CSS and forgot to add the overflow style to the JS. I wish you could edit these posts.

Updated function: 

	Initialize: function()
	{
		// These fixes are only for IE under strict mode
		if ( !FCKBrowserInfo.IsIE || FCKRegexLib.Html4DocType.test( FCKConfig.DocType ) )
		{
			return;
		}
	
		var oBody = FCK.EditingArea.Document.body;

		// Set the base CSS
		var arrStyles = {'position': 'absolute', 'top': '0px', 'right': '0px', 'bottom': '0px', 'left': '0px', 'overflow': 'auto'};
		
		for(var strStyle in arrStyles)
		{
			oBody.runtimeStyle[strStyle] = arrStyles[strStyle];
		}
		
		var arrSides = ['Top', 'Right', 'Bottom', 'Left'];
		
		// Convert all margin to padding so the scroll bars appear at the right position
		for(var i = 0; i < arrSides.length; i++)
		{
			var strSide = arrSides[i];

			oBody.runtimeStyle['padding' + strSide] = parseInt( oBody.currentStyle['padding' + strSide] )
				+ parseInt( oBody.currentStyle['margin' + strSide] )
				+ 'px';
			
			oBody.runtimeStyle['margin' + strSide] = '0px';
		}
		
		// The height must be auto or it messes up the absolute bottom: 0px css attribute
		oBody.runtimeStyle.height = 'auto';
		oBody.runtimeStyle.minHeight = 'auto';
	}

Also, you still need to take out that overflow line in line ~190 of fck_1.js

  Changed 2 years ago by Scott

Update: The absolute positioning is no longer necessary. And causes flicker when hovering over the toolbar.

This fixes it. 

	Initialize: function()
	{
		// These fixes are only for IE under strict mode
		if ( !FCKBrowserInfo.IsIE || FCKRegexLib.Html4DocType.test( FCKConfig.DocType ) )
		{
			return;
		}
	
		var oBody = FCK.EditingArea.Document.body;

		// Set the base CSS
		var arrStyles = {'height': '100%', 'overflow': 'auto'};
		
		for(var strStyle in arrStyles)
		{
			oBody.runtimeStyle[strStyle] = arrStyles[strStyle];
		}
		
		var arrSides = ['Top', 'Right', 'Bottom', 'Left'];
		
		// Convert all margin to padding so the scroll bars appear at the right position
		for(var i = 0; i < arrSides.length; i++)
		{
			var strSide = arrSides[i];

			oBody.runtimeStyle['padding' + strSide] = parseInt( oBody.currentStyle['padding' + strSide] )
				+ parseInt( oBody.currentStyle['margin' + strSide] )
				+ 'px';
			
			oBody.runtimeStyle['margin' + strSide] = '0px';
		}
	}

  Changed 2 years ago by Scott

*** FINAL - Ignore all previous posts ***

Argh. Sorry - caching fooled me again. I am using a custom script to bundle and compress plugins & css using YUI compressor and it makes development on the editor complicated - I need to fix that. I have been developing this from plugin-space.

Ok this should be the last post I make on this. The height of the body was not expanding after rebuilding and clearing the cache. So merged with the original height calculation function, it appears to work.

- So the body has a fixed height to be the same as the containing window. It is calculated on initialization and resize.

- The body has overflow: auto, so that it scrolls whenever the bodys size exceeds that of the fixed height. The scroll bar is effectively moved from being on the documentElement to being a part of the body.

- Because we are now scrolling from the body, any margins move the scroll bars away from the sides of the editor. So we add any margins to the padding. This does not matter as on the body element, padding and margin are virtually the same as the body element doesnt have any parent nodes

- If you see double scroll bars, the overflow line in fck_1.js needs to be removed around line 190. 

function FCK.InitializeIEStrict()
{
	var oBody = FCK.EditingArea.Document.body;

	// Set the base CSS
	oBody.runtimeStyle.overflow = 'auto';
		
	var arrSides = ['Top', 'Right', 'Bottom', 'Left'];
		
	// Convert all margin to padding so the scroll bars appear at the right position
	for(var i = 0; i < arrSides.length; i++)
	{
		var strSide = arrSides[i];

		oBody.runtimeStyle['padding' + strSide] = parseInt( oBody.currentStyle['padding' + strSide] )
			+ parseInt( oBody.currentStyle['margin' + strSide] )
			+ 'px';
			
		oBody.runtimeStyle['margin' + strSide] = '0px';
	}
		
	FCKTools.AddEventListenerEx( FCK.EditingArea.Window, 'resize', Doc_OnResize );
	Doc_OnResize();
},
	
function Doc_OnResize() 
{
	var oBody = FCK.EditingArea.Document.body;

	var iHeight = FCK.EditingArea.Document.documentElement.clientHeight 
		- parseInt( oBody.currentStyle.paddingTop )
		- parseInt( oBody.currentStyle.paddingBottom )
		;
	
	oBody.style.height = iHeight + 'px';
}

FCK.InitializeBehaviors = function( dontReturn )  
{
	...

	// Catch cursor selection changes.  
	this.EditorDocument.attachEvent("onselectionchange", Doc_OnSelectionChange ) ;  

	// #1659 The body in standards mode doesn't get full height.  
	// These fixes are only for IE under strict mode
	if ( !FCKBrowserInfo.IsIE || FCKTools.IsStrictMode( FCK.EditorDocument ) ) 
	{  
		FCK.InitializeIEStrict() ;  
	}
}

Enjoy :D

PS: This may break the auto-grow plugin as it broke mine (my version is totally different to the fck one though).

  Changed 2 years ago by fredck

  • keywords Review- HasPatch added; Review? removed

Based on Scott's comments, I'm assuming Alfonso's patch is not the definitive solution. Actually, I've just tested it. I was not able to reproduce the table problem as Scott described, but I was able to check that the cursor changes to arrow between paragraphs placed in the scrolled bottom of the document, and clicking there makes the editor scroll to the top, just like Scott said. It is an unpleasant thing, really.

Scott proposed a dirty hack to fix it. It could work, but I would be really happier if we could solve this issue with some kind of CSS magic. Also, the "break the auto-grow plugin" means more work should be done on this.

  Changed 2 years ago by Scott

I have used my dirty hack for a little while now. It is not a definitive solution either. Because we are scrolling on the body tag, and not the window, all absolute positioned elements are stuffed up.

I have another fix which uses min-height on the body. But this is no good either, as IE 6 does not support this without expressions.

  Changed 2 years ago by fredck

  • milestone changed from FCKeditor 2.6 to FCKeditor 2.6.1

  Changed 2 years ago by martinkou

It is currently not possible to use standards compliant mode by setting DocType in any browser due to #2102.

  Changed 20 months ago by alfonsoml

  • owner alfonsoml deleted
  • status changed from assigned to new
  • milestone changed from FCKeditor 2.6.3 to CKEditor 3.0

I don't have any solution for the next release, so moving to a future milestone and let's hope that someone is able to find the magic fix.

  Changed 20 months ago by fredck

I would be happy if we could have a IE7+ solution for it at least. Let's investigate it for V3.

  Changed 18 months ago by arczi

#960 has been marked as a DUP

  Changed 12 months ago by martinkou

  • owner set to martinkou
  • status changed from new to assigned

  Changed 12 months ago by martinkou

  • keywords Review? added; Review- removed

I've found a very simple way to fix in in CKEditor v3 - it doesn't need resize handlers.

Changed 12 months ago by martinkou

  Changed 12 months ago by Scott

Hmm - Are you sure setting height: 100% works?

From my memory... Try adding a whole lot of <p> tags to the page, then when there is enough content to scroll, click in the white area to the right where the cursor is a pointer, not an editing caret. The editor will likely scroll to the top.

Note: I have not tested your solution in v3.

  Changed 11 months ago by martinkou

  • keywords Review- added; Review? removed

Just did some tests... it works in IE6, but doesn't work in IE7.

Changed 11 months ago by martinkou

  Changed 11 months ago by martinkou

  • keywords Review? added; Review- removed

minHeight in body seems to work well in IE7.

  Changed 11 months ago by fredck

  • keywords Review- added; Review? removed
  • There should be a way to have that "22" pixel difference calculated from the document styles.
  • The hardcoded styles at the end of the patch could go inside contents.css, making it easier to customize.

  Changed 10 months ago by martinkou

  • keywords Review? added; Review- removed

I've changed the CSS hack to be specific to IE6, since with the new JS logic IE7 doesn't need the CSS hack anymore.

I've also added support for IE8.

Changed 10 months ago by martinkou

  Changed 10 months ago by martinkou

  • keywords Review- added; Review? removed

Oops, retracted, the patch in content.css is affecting the style drop down combos.

Changed 10 months ago by martinkou

  Changed 10 months ago by martinkou

  • keywords Review? added; Review- removed

So editor.addCss() is still the simplest way to do it - adding the hack to content.css would break combo boxes in IE6.

  Changed 10 months ago by fredck

  • milestone changed from CKEditor 3.0 to CKEditor 3.1

  Changed 9 months ago by martinkou

  • cc Senthil added
  • keywords Oracle added

follow-up: ↓ 40   Changed 9 months ago by fredck

#1659 has been marked as DUP.

in reply to: ↑ 39   Changed 9 months ago by tobiasz.cudnik

Replying to fredck:

#1659 has been marked as DUP.

Actually #3832 is a DUP.

I'm attaching patch from that ticket for consideration as it quite resolves this issue using CSS only. It's probably possible to have it 100% working using only css if we could introduce wrapper element inside iframe's body, which could stretch body and handle margin/padding from content.

Changed 9 months ago by tobiasz.cudnik

  Changed 8 months ago by garry.yao

#3990 has been marked as a DUP.

  Changed 5 months ago by simshaun

Just wanted to make sure people are aware, 1659_7_ref.patch breaks styles, format, and size dropdown floats in IE6. (It causes the scrollbars to disappear.)

  Changed 5 months ago by simshaun

Here is the fixed 1659_7 patch (works fine in IE6 and IE7). Sorry its not in .patch format; I dont have SVN on this PC to make one with. 

/* #3832 IE8 Can't always click in the editor and give focus */
body, html
{
	height: 95%;
	/* IE6 */
	_height: 85%;
}

body.cke_panel_frame
{
	height: auto;
	/* IE6 */
	_height: auto;
}
/* END #3832 */

  Changed 4 months ago by fredck

#4591 has been marked as DUP.

Changed 4 months ago by garry.yao

  Changed 4 months ago by garry.yao

I think we're heading into a wrong direction on this problem, the thing to fix is not making <body> fit the viewport (which is wrong for various evil results we already seen above), but to fix the IE missing behavior of "focus the editable body content when click on document".
So here I it make sense to propose here let's recall our original purpose on this ticket and I'm attaching a patch targeting exactly this issue.
Ticket Test added at :
http://ckeditor.t/tt/1659/1.html.

  Changed 4 months ago by fredck

  • keywords Review- added; Review? removed

The idea of the patch is good, and it should be enough for our needs here. Some review though:

  • Check if it's possible to make the code simpler, without having to do all the offset checks, but still taking into consideration the scrollbars issue.
  • The current behavior is a bit strange, as it shows the browser's focus box around the editor for a fraction of second when clicking. There should be another way to set the focus, or even a way to avoid the focus box.
  • The caret disappears at every second click in the bottom empty area.

  Changed 4 months ago by garry.yao

  • owner changed from martinkou to garry.yao
  • status changed from assigned to new

Changed 3 months ago by garry.yao

  Changed 3 months ago by garry.yao

  • keywords Review? added; Review- HasPatch removed
  • status changed from new to assigned

New patch targeting the above mentioned issues.

  Changed 3 months ago by fredck

#4737 has been marked as DUP.

  Changed 3 months ago by fredck

  • keywords Review+ added; Review? removed

Wow... this solution is pretty good. Congrats!

  Changed 3 months ago by garry.yao

  • status changed from assigned to closed
  • resolution set to fixed

Fixed with [4563].

  Changed 3 months ago by Scott

I might be wrong here because I haven't tested the patches above...

I can see this fixing the click focus problem. But the cursor is going to be pointer rather than a text caret. This one of the reasons why in my earlier attempts I was trying to extend the viewport / body element.

Is this patch going to be used in conjunction with some CSS to restore the text caret cursor?

  Changed 3 months ago by Scott

Just another note... You can set documentElement.style.cursor = 'text', and it gives you a nice text cursor for the area below the body.

But you still get cursor: default between paragraphs or to the right of paragraphs that dont extend the full length of the line.

Also, as you are all probably aware, setting cursor styles has no effect on elements nested underneath a contentEditable element.

  Changed 3 months ago by fredck

Scott, we'll be releasing the 3.0.2 and 3.1 soon, which will contain this fix. You can then test it and check if it impacts on usability. It works well for me, and I doubt users will feel any issue because of the cursor.

In any case, if you think it's worth, feel free to opening a new dedicated ticket after you confirm it in the release. Thanks!

Note: See TracTickets for help on using tickets.