Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#10368 closed defect (fixed)

[patch][cla]Editor AutoExpand mode, excessive padding on <div>'s in content (FF)

Reported by: youngho Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.5
Component: Editor Version: 1.4.0b
Keywords: Cc: bill
Blocked By: Blocking:

Description (last modified by bill)

Hello,

The Editor in auto expand mode shows too much white space around div tags (in the original content).

You can see problem with the test file and attached screenshot.

Attachments (12)

autoexpand.patch (739 bytes) - added by youngho 9 years ago.
autoExpand.gif (60.5 KB) - added by youngho 9 years ago.
test result screen
test_Editor_AutoExpand.html (3.0 KB) - added by bill 9 years ago.
updated test w/auto-expand mode and non-auto-expand mode, for comparison
reason.gif (123.3 KB) - added by youngho 9 years ago.
test_Editor_AutoExpand.2.html (3.2 KB) - added by youngho 9 years ago.
fixed id duplication and min-height adjust
Editor_AutoExpand.html (3.4 KB) - added by youngho 9 years ago.
robot test modified from robot/Editor_misc.html
bodyDiv.patch (1.3 KB) - added by Jared Jurkiewicz 9 years ago.
Potential patch
style_cleanup.patch (3.6 KB) - added by Jared Jurkiewicz 9 years ago.
Overhaul of style setting. Removed what appeared to be unneeded styles and improved the selector defs.
RichText.js (57.0 KB) - added by Jared Jurkiewicz 9 years ago.
Full RichText?.js, so you don't have to apply the patch to see how it changes things. Cleaned up the doc text formatting a bit too, simply to make it easier to read when debugging.
style_cleanup2.patch (4.2 KB) - added by Jared Jurkiewicz 9 years ago.
New patch with improvements on setting of styles. Seems to work on all browsers.
style_cleanup2.2.patch (4.3 KB) - added by Jared Jurkiewicz 9 years ago.
Patch to CSS style setting to fix this issue. Seems to work on all browsers WRT scrollers and such. Tried: IE7, FF3, Safari 3 and 4, Chrome, and Opera
style_cleanup3.patch (5.2 KB) - added by Jared Jurkiewicz 9 years ago.
Followup patch to kill y scroller on body/html when using <div> wrapper and in autoheight mode.

Download all attachments as: .zip

Change History (42)

Changed 9 years ago by youngho

Attachment: autoexpand.patch added

comment:1 Changed 9 years ago by bill

Component: GeneralEditor
Owner: anonymous deleted

Hi Youngho,

Thanks for the patch/test case. Which browser does this happen on? It seems to be working fine for me... unless you are talking about the problem where the text momentarily scrolls up and then back down again?

comment:2 Changed 9 years ago by youngho

Hello Bill,

FF 3.5.5 in Windows which I tested.

Umm IE8 and Chormuim work find..

comment:3 Changed 9 years ago by bill

Summary: [patch][cla]Editor AutoExpand Error[patch][cla]Editor AutoExpand Error (FF)

OK, and what do you mean exactly by "doesn't work"? It doesn't expand? Or there's an error message?

It's actually working fine for me on FF3.5.5/win. Does the third example in http://download.dojotoolkit.org/release-1.4.0b2/dojo-release-1.4.0b2/dijit/tests/editor/test_Editor.html work for you?

comment:4 Changed 9 years ago by youngho

Hello Bill,

Please compare with my test screen and http://download.dojotoolkit.org/release-1.4.0b2/dojo-release-1.4.0b2/dijit/tests/editor/test_Editor.html's auto expansion test area. The code is same except the p tag replaced with div.

I see that my test screen editor block height is a twice of test_Editor.html height.

Well... this report name should be changed to 'When the editor set autoExpand in FF than the div block height set with the autoExpand setting value also.'

Thanks.

Youngho

Changed 9 years ago by youngho

Attachment: autoExpand.gif added

test result screen

comment:5 Changed 9 years ago by bill

Description: modified (diff)
Milestone: tbd1.5
Owner: set to Jared Jurkiewicz
Summary: [patch][cla]Editor AutoExpand Error (FF)[patch][cla]Editor AutoExpand mode, excessive padding on <div>'s in content (FF)

Oh I see, it's not a problem typing into the editor, it's a problem with the initial content. There's too much spacing between each div.

I'm a little wary of the patch though as I'm not sure what that original CSS was for... looks like it's for avoiding spurious scrollbars and it seems like your change would break that?

Changed 9 years ago by bill

Attachment: test_Editor_AutoExpand.html added

updated test w/auto-expand mode and non-auto-expand mode, for comparison

comment:6 Changed 9 years ago by youngho

I can not understand what the original CSS was for too.

But because of this css, the div was set the min-height by force from editor. I attached the firebug screen for that. How can we guarantee the content starts with single div block always for FF users?

I tested the patch with test_Editor.html and test_Editor_AutoExpand.html for different browsers and the result looks fine.

IE8 IE7(IE8) FF3.5 Chromium Safari(Windows)

Before Patch O O X O O

After Patch O O O O O

I could not test with IE6 because I have not and during the short test, I did not find any spurious scrollbars.

Changed 9 years ago by youngho

Attachment: reason.gif added

comment:7 Changed 9 years ago by bill

That body > div selector dates back to [10091], apparently fixing #2276, the problem where a horizontal scrollbar wouldn't appear for a long string without any spaces.

The <div> reference is because in some cases / on some browsers Editor puts a <div> inside the <iframe>, as part of the editor boilerplate. It's added in getIframeDocTxt():

if(dojo.isIE || (!this.height && !dojo.isMoz)){
	// For some reason we need an extra <div> on IE (TODOC)
	html = "<div></div>";

And there's corresponding code in onLoad():

if(!dojo.isIE && (this.height || dojo.isMoz)){
	this.editNode=this.document.body;
}else{
	this.editNode=this.document.body.firstChild;
	...

The wrapper <div> appears on IE and also on Safari when height="" (auto-expand mode), but never on FF, thus leading to this problem.

Back in [10091] the wrapper div was added whenever height="", regardless of browser:

if(!this.height){
        html="<div>"+html+"</div>";
}

As for min-height, that setting is used in auto-expand mode, and it was originally applied to <body>, but was changed in [16552], for #6401, to fix a problem with safari.

The current CSS is:

body,html{overflow-y:hidden;/*for IE*/}
body > div {
	overflow-x:auto;/*FF:horizontal scrollbar*/
	overflow-y:hidden;/*safari*/
	min-height:"+this.minHeight+";/*safari*/
}

So anyway, I agree that the current code is broken, but just changing body > div to body will likely break safari. I think the CSS rules added need to branch based on whether or not the wrapper <div> was added.

And then do tests on all browsers for the horizontal scrollbar appearing (when typing a long string w/no spaces), auto-expand mode working, and for fixed-size mode make sure that both vertical and horizontal scrollbars work as expected.

comment:8 Changed 9 years ago by bill

PS: it would be nice if we didn't need that <div> at all, or at least if we could create it unconditionally on all browsers. It might not be needed... would need some testing.

I can see references to a nested <div> back to at least [3676] , but that's for the non-iframe code path (back then editor supported both iframe and non-iframe modes)

[7001] changed the <div> to be added conditionally, only when height="", but I'm not sure why.

comment:9 Changed 9 years ago by bill

PPS: apparently (at a minimum) safari needs that wrapper div. The AlwaysShowToolbar plugin has code to expand/contract the editor as the user types, so it needs to query the scrollHeight of the editor's content, but (on safari) neither dojo.marginBox(body) nor body.scrollHeight nor body.clientHeight show that actual height of the content... only works if there's a wrapper div.

comment:10 Changed 9 years ago by bill

(In [20878]) Add comments, refs #10368 !strict.

Changed 9 years ago by youngho

fixed id duplication and min-height adjust

Changed 9 years ago by youngho

Attachment: Editor_AutoExpand.html added

robot test modified from robot/Editor_misc.html

comment:11 Changed 9 years ago by Jared Jurkiewicz

I odn't understand what test_Editor_AutoExpand.2.html and Editor_AutoExpand.html are. Are they testcases? Something else?

Changed 9 years ago by Jared Jurkiewicz

Attachment: bodyDiv.patch added

Potential patch

comment:12 Changed 9 years ago by Jared Jurkiewicz

Please see attached patch. If I'm understanding the issue right from the description regarding CSS being set when it shouldn't be, then I think the noted patch will resolve it. Can you try said patch in your system and let us know?

comment:13 Changed 9 years ago by youngho

Hello bill and jaredj

The jaredj's patch works fine for me (tested with FF3.5).

Thanks for your kind consideration.

comment:14 Changed 9 years ago by bill

It'll probably work, but I think it can be cleaned up more. For example, the patch says:

body > div {overflow-x:auto;/*FF:horizontal scrollbar*/

Yet that rule will never do anything useful on FF since FF doesn't has the <div>. From my testing on FF, overflow-x is auto by default... maybe the rule can just be removed completely. Just need to check that typing a long text string (on an auto-expand Editor) triggers a horizontal scrollbar in all browsers.

As for min-height, that's getting set twice now, once on body and once on body > div. Probably we should branch where we set it rather than setting it twice. Set it on the <div> if the <div> exists, otherwise on <body>. (Probably it doesn't hurt to set it two places but it's just confusing.)

Similarly overflow-y is getting set twice. Not sure if that's needed. I don't see why it would be needed on the <div>. Should try removing that setting and then seeing if any of the browsers get transient scrollbars (just for a split second) while the editor is expanding to fit a new row of text.

Oh, also, maybe setBodyDivCss should be set in "this" so that it can be referenced from onLoad(), rather than repeating the same if condition in onLoad() and _getIframeTxt().

comment:15 Changed 9 years ago by bill

One more note: body > div is a dangerous selector on IE6 since it will match any div inside of body. This could be avoided by giving an id to the div and referencing the id from the selector, like this:

<body>
    <div id="editNode">
         ...
#editNode { ... }

Actually, when there isn't a wrapper <div> we could set that id on body itself:

<body id="editNode">

That might simplify the CSS selectors used.

Changed 9 years ago by Jared Jurkiewicz

Attachment: style_cleanup.patch added

Overhaul of style setting. Removed what appeared to be unneeded styles and improved the selector defs.

Changed 9 years ago by Jared Jurkiewicz

Attachment: RichText.js added

Full RichText?.js, so you don't have to apply the patch to see how it changes things. Cleaned up the doc text formatting a bit too, simply to make it easier to read when debugging.

Changed 9 years ago by Jared Jurkiewicz

Attachment: style_cleanup2.patch added

New patch with improvements on setting of styles. Seems to work on all browsers.

Changed 9 years ago by Jared Jurkiewicz

Attachment: style_cleanup2.2.patch added

Patch to CSS style setting to fix this issue. Seems to work on all browsers WRT scrollers and such. Tried: IE7, FF3, Safari 3 and 4, Chrome, and Opera

comment:16 Changed 9 years ago by Jared Jurkiewicz

Cc: bill added

Bill, added a new patch that tweaks the CSS setting a bit better (based on your input). I tried it out on FF3, IE6, IE7, Opera, Safari 3, Safari 4, And chrome without any obvious problems. Expanding editor still works, and scrollers appear when on fixed size and content exceeds window.

comment:17 Changed 9 years ago by bill

I see a scrollbar on IE6 for the auto-expanding editor (in test_Editor.html). Just type quickly, hitting alphabetic characters and the return key. Usually the scrollbar disappears after a split second but sometimes it even stays around permanently.

Presumably since you've removed the overflow:hidden settings on <html> and <body>.

comment:18 Changed 9 years ago by Jared Jurkiewicz

Which I had to do because of other issues. Let me see if I can flag those to be applied only when it's in autoheight mode.

Changed 9 years ago by Jared Jurkiewicz

Attachment: style_cleanup3.patch added

Followup patch to kill y scroller on body/html when using <div> wrapper and in autoheight mode.

comment:19 Changed 9 years ago by bill

Thanks, I'll take a look. What are the issues when overflow: hidden is set on <body> and <html>?

comment:20 Changed 9 years ago by Jared Jurkiewicz

Scrollers don't appear on the content window when the iframe is larger than the wrapping div for browsers that don't use the embedded <div> element. In other words scrollers don't appear on FF, WebKit?, etc.

So they have to be left alone except in the case of IE and that internal <div> element.

comment:21 Changed 9 years ago by Jared Jurkiewicz

In the 'not auto expanding case', that is.

comment:22 Changed 9 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [21339]) Fix for improper padding on DIVs in FireFox? due to CSS rules. \!strict fixes #10368

comment:23 Changed 9 years ago by davidmark

Resolution: fixed
Status: closedreopened

What we have at this point (and likely before) is a huge mess. It's unreadable and nearly impossible to maintain (as we've seen). The problems need to be investigated and solved once and for all, not patched endlessly (QA testing is expensive). The patchwork just keeps getting more and more complex. Before long, there will be a branch for every known browser and version, but then a new browser (or version) will come out and invalidate the assumptions made. It's an endless cycle of futility and needs to be broken.

Making it seem to work today by referencing the UA string is mortgaging the future. Also, it can only be _expected_ to work in the browsers and configurations tested. IE8 alone has so many configurations and modes that it is impossible to test all of them, so you have to first understand the problem and then fix it properly. Then use testing to confirm that you did it right. With browser sniffing, testing is shaping the "design", which is backwards.

comment:24 Changed 9 years ago by davidmark

 // FIXME: IE 6 won't understand min-height? 

Good place to start. Clearly there is a misunderstanding as the comment is an open question. The answer is yes, IE6 does not understand min-height. Neither does IE7/8 in quirks mode. It is easy enough to find out:-

typeof document.documentElement.style.minHeight == 'string'

That will be false if the style is unsupported.

"\tmin-height:", this.minHeight, ";", 
"\tline-height:", lineHeight, 
"}", 
"p{ margin: 1em 0; }", 
(this.height ? // height:auto undoes the height:100% 
"" : "body,html{overflow-y:hidden;/*for IE*/}

This makes no sense. Set for either the body (quirks mode) or the HTML element (standards mode), not both.

body > div {overflow-x:auto;/*FF:horizontal scrollbar*/ overflow-y:hidden;/*safari*/ min-height:"+this.minHeight+";/*safari*/}" 

What do those last two bits have to do with Safari? And what about DIV's that are children of the BODY? "FF:horizontal scrollbar" doesn't tell me (or anyone else trying to unravel these hacks) anything about the problem (real or perceived).

(dojo.isMoz && label.length ? "<title>" + label[0].innerHTML + "</title>\n" : ""), 

Certainly this is an enigma. What does this have to do with Mozilla? Nobody else gets a title?

"<meta http-equiv='Content-Type' content='text/html'>\n", 
"<style>\n", 

Missing required type attribute.

"\tbody,html {\n", 

See above.

"\t\tbackground:transparent;\n", 
"\t\tpadding: 1px 0 0 0;\n", 
"\t\tmargin: -1px 0 0 0;\n", // remove extraneous vertical scrollbar on safari and firefox 

How was this arrived at? Testing is driving the design. You can't do that or you will be back in here every other month to adjust to the perceived state of correctness (which is likely just an illusion).

// Set the html/body sizing.  Webkit always needs this, other browsers 
// only set it when height is defined (not auto-expanding), otherwise  
// scrollers do not appear. 
((dojo.isWebKit)?"\t\twidth: 100%;\n":""), 
((dojo.isWebKit)?"\t\theight: 100%;\n":""), 
"\t}\n", 

The comments don't match the code. And why does Webkit always _need_ this? According to testing done in a couple of browsers with a couple of test documents? Understanding of the issue is needed to proceed.

comment:25 Changed 9 years ago by davidmark

First thing to do is remove all of the tangled up hacks and browser sniffing. Then make a list of everything perceived to be wrong. Investigate and solve each in turn (without referencing the baseless UA string, which tells you absolutely _nothing_ about these issues, past, present or future).

comment:26 Changed 9 years ago by Adam Peller

feel free to make such a list and provide a patch. Only what is relevant to this ticket should be posted here.

comment:27 Changed 9 years ago by Jared Jurkiewicz

David,

Either provide a useful patch to help solve the issues going on, or quit being a pain. I'm getting very tired of this.

comment:28 Changed 9 years ago by Jared Jurkiewicz

I should also point out, the line you refer to: body > div {overflow-x:auto;/*FF:horizontal scrollbar*/ overflow-y:hidden;/*safari*/ min-height:"+this.minHeight+";/*safari*/}"

Was explicitly removed by the patch applied to the editor.

comment:29 Changed 9 years ago by Jared Jurkiewicz

Resolution: fixed
Status: reopenedclosed

comment:30 Changed 9 years ago by Jared Jurkiewicz

None of your complaints are related to the issue corrected in this ticket. Therefore closing again.

Note: See TracTickets for help on using tickets.