Opened 13 years ago

Closed 13 years ago

#8050 closed Bug (fixed)

Autogrow problem with margins

Reported by: Dinu Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: General Version:
Keywords: IBM Cc: satya_minnekanti@…

Description

To reproduce: nightly build, autogrow demo.
Add a few <p style="margin:50px">&nbsp;</p> elements to trigger scrollbar. Scrollbar is always visible.
I haven't seen this behavior lately, so I assume it's a regression
Browsers: all
it always happens when the top or bottom element margin is taller than document margin. You can also test by setting body margin to 0px.

Attachments (19)

8050.patch (544 bytes) - added by Garry Yao 13 years ago.
8050_2.patch (4.8 KB) - added by Garry Yao 13 years ago.
8050_3.js (4.2 KB) - added by Dinu 13 years ago.
8050_4.js (4.3 KB) - added by Dinu 13 years ago.
8050_5.2.js (4.4 KB) - added by Dinu 13 years ago.
8050_5.patch (4.8 KB) - added by Dinu 13 years ago.
8050_5.js (4.4 KB) - added by Dinu 13 years ago.
8050_3.patch (4.9 KB) - added by Garry Yao 13 years ago.
8050_4.patch (4.5 KB) - added by Garry Yao 13 years ago.
8050_6.patch (5.4 KB) - added by Garry Yao 13 years ago.
8050_7.patch (3.3 KB) - added by Garry Yao 13 years ago.
8050_8.patch (3.3 KB) - added by Garry Yao 13 years ago.
8050_9.patch (4.4 KB) - added by Dinu 13 years ago.
8050_10.patch (3.8 KB) - added by Garry Yao 13 years ago.
8050_11.patch (4.1 KB) - added by Garry Yao 13 years ago.
8050_12_broken_ie7.patch (4.8 KB) - added by Dinu 13 years ago.
8050_12.patch (4.2 KB) - added by Garry Yao 13 years ago.
8050_13.patch (4.0 KB) - added by Garry Yao 13 years ago.
8050_14.patch (4.0 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (70)

comment:1 Changed 13 years ago by Jakub Ś

Resolution: duplicate
Status: newclosed

Looks like a DUP of #8014

comment:2 Changed 13 years ago by Dinu

Nope, completely different. The autogrow is triggered but the size is constantly (element_margin-document_margin) wrong. Also does not happen with 5.4 and http://dev.ckeditor.com/ticket/7173

comment:3 Changed 13 years ago by Jakub Ś

Resolution: duplicate
Status: closedreopened

comment:4 Changed 13 years ago by Jakub Ś

Status: reopenedconfirmed
Version: 3.6.1 (SVN - trunk)3.4.2

Yes you are right. I'm sorry for the mistake.

In #8014 if you add something to source, switch to wysiwyg and click inside editor - the autogrow is fired and editor size is adjusted to its contents.

Here the editors size is never adjusted correctly to its contents.

Reproducible from CKEditor 3.4.2 on all browsers.

comment:5 Changed 13 years ago by Jakub Ś

Version: 3.4.23.6.1 (SVN - trunk)

I did some further checking on this. It seems that on my local source versions this issue is reproducible from CKEditor 3.4.2

From http://rev.ckeditor.com/ it seems that this issue was fixed in rev [6921] and stopped working in [7032]

Changed 13 years ago by Garry Yao

Attachment: 8050.patch added

comment:6 Changed 13 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

You're right jakub, it's a regression of [7032].

Patch takes consideration of the last element's margin.

comment:7 Changed 13 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:8 Changed 13 years ago by Wiktor Walc

Milestone: CKEditor 3.6.1

comment:9 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:10 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7048].

comment:11 Changed 13 years ago by Dinu

Still does not work with body margin 0px. To reproduce, set body margin 0px; add any elements with margins (h1, li, etc). Scrollbar will be present. In conjunction with http://dev.ckeditor.com/ticket/8057#comment:4 the editor never settles and crashes webkit.

Browsers: all

comment:12 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

Besides, patch breaks quirks mode.

comment:13 Changed 13 years ago by Dinu

Test case #1: Chrome Body margin 0px Insert contents: http://pastebin.com/NwYZiYVw Click on the paragraphs below the table; editor size will flicker

comment:14 Changed 13 years ago by Dinu

Test case 2:
All browsers
Body margin 0px
http://pastebin.com/QYdHJ3c6
Size is line-height - font-size wrong.

comment:15 Changed 13 years ago by Dinu

Test case 3:
IE
Body margin 0px
Insert a bunch of paragraphs - scrollbars will be present.

Changed 13 years ago by Garry Yao

Attachment: 8050_2.patch added

comment:16 Changed 13 years ago by Garry Yao

Status: reopenedreview

There're just too much flavors from different browsers plus rendering mode to make a precise calculation of the desired frame height, considering all of the applied content styles (like those tcs inputted by dinu), the patch propose a lazier way of working this out: by reverse-engineering the scroll-bar at runtime, the performance penalty brought by it should just affects the initial resize and I've tried to reduce it to around half second.

The result is pretty impressive that after than we could forget about all the bells and whistlers regard climbing up the box model.

Changed 13 years ago by Dinu

Attachment: 8050_3.js added

comment:17 Changed 13 years ago by Dinu

Finally, I combined Garry's latest patch with something I made a while ago for a similar purpose, and found an elegant way that looks about right. Tested on: IE 7,8,9; FF4; Chrome, Safari, Opera both Web3 and CK sample page. NOT tested on: IE6; any quirks modes.

Also found a scrollbar bug specific to Chrome (and that is really a browser bug regarding scrollbar display policy) that keeps the scrollbars on when having a block element with width:100%; any place to forward this?

Still a couple things left in the file under comments "TODO:", "???", "!!!". I couldn't figure these out, please help.

comment:18 Changed 13 years ago by Dinu

One more TODO: autogrow should be triggered on subsequent object loads (sometimes an image inserted in source mode takes a while to load). Can't remember if this is doable so I'll leave it as food for thought.

Changed 13 years ago by Dinu

Attachment: 8050_4.js added

comment:19 Changed 13 years ago by Garry Yao

Finally, I combined Garry's latest patch with something I made a while ago for a similar purpose

I can tell Dinu's patch is not really based off my patch.

comment:20 Changed 13 years ago by Dinu

After last chat with Garry, made a few adjustments:

  • actually accounts for floated content
  • wasn't downsizing on IE9
  • removed one unnecessary recalculation when downsizing

Changed 13 years ago by Dinu

Attachment: 8050_5.2.js added

Changed 13 years ago by Dinu

Attachment: 8050_5.patch added

Changed 13 years ago by Dinu

Attachment: 8050_5.js added

comment:21 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

8050_5 had passed all of my tests in various browsers, well done Dinu and Garry!

Just a note, the patch contains multiple coding style problems, please fix those before committing.

Last edited 13 years ago by Sa'ar Zac Elias (previous) (diff)

Changed 13 years ago by Garry Yao

Attachment: 8050_3.patch added

comment:22 Changed 13 years ago by Sa'ar Zac Elias

Status: review_passedreview_failed

Oops, a failing TC (IE9, FF4; works fine for others):
contentsCss:

body
{
 margin: 40px;
 padding: 20px;
}
p
{
 margin: 30px;
 border: 10px solid blue;
}

Always shows v-scrollbars.

comment:23 Changed 13 years ago by Wiktor Walc

8050_2.patch

  1. Open _samples/autogrow.html
  2. Click inside of the editing area
  3. Editor resizes itself to match the single paragraph, the editing area is very small.

8050_3.patch

  1. Open _samples/autogrow.html
  2. Scroll down to the second editor
  3. Click inside of the editing area
  4. Result: the whole page scrolls up to the top.

comment:24 in reply to:  23 Changed 13 years ago by Sa'ar Zac Elias

Replying to wwalc:

8050_3.patch

  1. Open _samples/autogrow.html
  2. Scroll down to the second editor
  3. Click inside of the editing area
  4. Result: the whole page scrolls up to the top.

Not all the way top though. In FF, page blinks as well.

comment:25 Changed 13 years ago by Wiktor Walc

Milestone: CKEditor 3.6.1
Version: 3.6.1 (SVN - trunk)

[6921] and all further changes to the autogrow plugin (including [7032]) were reverted with [7056].

Changed 13 years ago by Garry Yao

Attachment: 8050_4.patch added

comment:26 Changed 13 years ago by Garry Yao

Status: review_failedreview
  1. Updating patch to current trunk;
  2. Host page scroll fixed;
  3. Floated elements in content fixed;

comment:27 Changed 13 years ago by Dinu

8050_4.patch:

1)Start editor with

<div style="width:2000px; height:2000px"></div>

Delete div
Now editor will always resize 1 scrollbar too high

2) crash test, synthetic, shouldnt happen in real life but when it does:

<div style="position:absolute; height:100%; width:2000px"></div>

IE freezes down

comment:28 Changed 13 years ago by Satya Minnekanti

Keywords: IBM added

Changed 13 years ago by Garry Yao

Attachment: 8050_6.patch added

comment:29 Changed 13 years ago by Garry Yao

Updates:

  1. Considering of h-scrollbar; (Dinu's tc 1)
  2. Call stop the growing when scrollbar is always shown; (Dinu's tc 2)

Changed 13 years ago by Garry Yao

Attachment: 8050_7.patch added

comment:30 Changed 13 years ago by Garry Yao

To avoid adding further complexities to this feature, which doesn't desires as the entire struggling here is just to avoid the scrollbar, so a simple work around exists: Considering the margin area (padding/margin of body/html) below the actual content (which reveals the scrollbar) isn't really useful for user, we can always hide the scrollbar in such situation which makes the editor looks just like precisely resized.

Changed 13 years ago by Garry Yao

Attachment: 8050_8.patch added

comment:31 Changed 13 years ago by Garry Yao

Saar and Wiktor pointed out there existed a problem with the autoGrow_bottomSpace config in previous patch.

comment:32 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • [IE<9] Not really a bug, more like a consistency feature:
    • Load the sample with the following CSS:
      p {
      	margin: 10px;
      	padding: 20px;
      }
      
    • Put cursor in the beggining of the existing line in the sample.
    • Hit ENTER a few times, till the paragraph gets to the end of the editor contents.
    • See that some bottom margin is shown.
    • Hit BACKSPACE.
    • See that the margin is not shown anymore.
  • [WebKit] Host scrolling bug still exists.

Changed 13 years ago by Dinu

Attachment: 8050_9.patch added

comment:33 Changed 13 years ago by Dinu

Added 8050_9:

  • removed contentHeight function:

1) While it did sort of work it was for a completely unexpected reason: it forced WebKit to recompute the iframe layout, for which I added lines 41-47
2) Both using inline or block <span> are flawed: inline honors vertical-align, doesn't account for floated elements, while block separates last element margin and document margin (so if we have last element margin=15, body margin=20, it will have 35 height margin instead of max=20)

  • added 3 step resize (for shrinking): 1px, then scrollHeight then scrollHeight again
  • resized the iframe rather than container in order to avoid parent scroll problems and to work around the webkit bug; iframe and container resize order _is_ important for webkit

Sorry for coding style, willing to improve on it :)

comment:34 Changed 13 years ago by Garry Yao

Manual tcs added to [7119].

Changed 13 years ago by Garry Yao

Attachment: 8050_10.patch added

comment:35 Changed 13 years ago by Garry Yao

Status: review_failedreview

@Dinu can u check if the latest simplified patch is functionally identical to your last patch?

comment:36 Changed 13 years ago by Dinu

Hi, not exactly; TC:

First invoke resizes too high; second gets it right because vertical scrollbar is removed on first invoke. I recall the need for line 41-47 forced recomp on webkit will appear as one tries to fix this.

comment:37 Changed 13 years ago by Garry Yao

Status: reviewreview_failed

I found the 9th/10th wouldn't be accepted because it brings a serious scrolling issue that scrap usability, discovered by this tc which will make document always scrolls to top.

Changed 13 years ago by Garry Yao

Attachment: 8050_11.patch added

comment:38 Changed 13 years ago by Garry Yao

Status: review_failedreview

Jump to continue from Saar's review:

  1. This's exactly the "first resizes too high issue" described by Dinu's comment, now addressed with new patch;
  2. Webkit scrolling issue is instead #6212.

comment:39 Changed 13 years ago by Garry Yao

Both using inline or block <span> are flawed: inline honors vertical-align, doesn't account for floated elements, while block separates last element margin and document margin

True, but the marker used in patch 11th (1px size clear float block) works well with inline and floats, while it simply doesn't account for document margin (body padding and margin) which won't be a problem as explained by my previous comment.

comment:40 in reply to:  37 Changed 13 years ago by Dinu

Replying to garry.yao:

I found the 9th/10th wouldn't be accepted because it brings a serious scrolling issue that scrap usability, discovered by this tc which will make document always scrolls to top.

Shouldn't this be trivially fixed by saving/restoring scrollTop around the resize? I'll try.

Changed 13 years ago by Dinu

Attachment: 8050_12_broken_ie7.patch added

comment:41 Changed 13 years ago by Dinu

I couldn't make scrollTop work on IE7... It's a pity as it works so good on others and it contains only 1 hack :) Attached for reference.

comment:42 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
body
{
 margin: 40px;
 padding: 20px;
}
p
{
 margin: 30px;
 border: 10px solid blue;
}

Always shows scrollbars.
Manual TCs added with [7180], [7182].

Last edited 13 years ago by Sa'ar Zac Elias (previous) (diff)

Changed 13 years ago by Garry Yao

Attachment: 8050_12.patch added

comment:43 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:44 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

In Opera, TCs 5,6 fail.

Last edited 13 years ago by Sa'ar Zac Elias (previous) (diff)

Changed 13 years ago by Garry Yao

Attachment: 8050_13.patch added

comment:45 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:46 Changed 13 years ago by exim

The patch doesn't seem to allow minHeight being zero due to the newly introduced

var min = editor.config.autoGrow_minHeight || 200,

declaration that seems to reset it to default (200) when the value is zero (false).

Prior to the patch the following expression was used to set minHeight to default:

( min == undefined ) && ( editor.config.autoGrow_minHeight = min = 200 );

and the assignment was triggered only when min was undefined.

I think it can be fixed by replacing the patch declaration line with something like this:

var min = editor.config.autoGrow_minHeight == undefined ? 200 : editor.config.autoGrow_minHeight,

Changed 13 years ago by Garry Yao

Attachment: 8050_14.patch added

comment:47 Changed 13 years ago by Garry Yao

The patch doesn't seem to allow minHeight being zero...

Ok, that's a mistake, manual tc added: http://ckeditor.t/tt/8050/10.html

comment:48 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

comment:49 Changed 13 years ago by Sa'ar Zac Elias

Status: review_failedreview_passed

Sorry, that was a cache issue, everything looks good, congratulations.

comment:50 Changed 13 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.2

comment:51 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7244].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy