Opened 5 years ago

Closed 5 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 5 years ago.
8050_2.patch (4.8 KB) - added by garry.yao 5 years ago.
8050_3.js (4.2 KB) - added by dinu 5 years ago.
8050_4.js (4.3 KB) - added by dinu 5 years ago.
8050_5.2.js (4.4 KB) - added by dinu 5 years ago.
8050_5.patch (4.8 KB) - added by dinu 5 years ago.
8050_5.js (4.4 KB) - added by dinu 5 years ago.
8050_3.patch (4.9 KB) - added by garry.yao 5 years ago.
8050_4.patch (4.5 KB) - added by garry.yao 5 years ago.
8050_6.patch (5.4 KB) - added by garry.yao 5 years ago.
8050_7.patch (3.3 KB) - added by garry.yao 5 years ago.
8050_8.patch (3.3 KB) - added by garry.yao 5 years ago.
8050_9.patch (4.4 KB) - added by dinu 5 years ago.
8050_10.patch (3.8 KB) - added by garry.yao 5 years ago.
8050_11.patch (4.1 KB) - added by garry.yao 5 years ago.
8050_12_broken_ie7.patch (4.8 KB) - added by dinu 5 years ago.
8050_12.patch (4.2 KB) - added by garry.yao 5 years ago.
8050_13.patch (4.0 KB) - added by garry.yao 5 years ago.
8050_14.patch (4.0 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (70)

comment:1 Changed 5 years ago by j.swiderski

  • Resolution set to duplicate
  • Status changed from new to closed

Looks like a DUP of #8014

comment:2 Changed 5 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 5 years ago by j.swiderski

  • Resolution duplicate deleted
  • Status changed from closed to reopened

comment:4 Changed 5 years ago by j.swiderski

  • Status changed from reopened to confirmed
  • Version changed from 3.6.1 (SVN - trunk) to 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 5 years ago by j.swiderski

  • Version changed from 3.4.2 to 3.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 5 years ago by garry.yao

comment:6 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

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

Patch takes consideration of the last element's margin.

comment:7 Changed 5 years ago by satya

  • Cc satya_minnekanti@… added

comment:8 Changed 5 years ago by wwalc

  • Milestone set to CKEditor 3.6.1

comment:9 Changed 5 years ago by Saare

  • Status changed from review to review_passed

comment:10 Changed 5 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7048].

comment:11 Changed 5 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 5 years ago by garry.yao

  • Resolution fixed deleted
  • Status changed from closed to reopened

Besides, patch breaks quirks mode.

comment:13 Changed 5 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 5 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 5 years ago by dinu

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

Changed 5 years ago by garry.yao

comment:16 Changed 5 years ago by garry.yao

  • Status changed from reopened to review

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 5 years ago by dinu

comment:17 Changed 5 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 5 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 5 years ago by dinu

comment:19 Changed 5 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 5 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 5 years ago by dinu

Changed 5 years ago by dinu

Changed 5 years ago by dinu

comment:21 Changed 5 years ago by Saare

  • Status changed from review to review_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 5 years ago by Saare (previous) (diff)

Changed 5 years ago by garry.yao

comment:22 Changed 5 years ago by Saare

  • Status changed from review_passed to review_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 follow-up: Changed 5 years ago by wwalc

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 5 years ago by Saare

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 5 years ago by wwalc

  • Milestone CKEditor 3.6.1 deleted
  • Version 3.6.1 (SVN - trunk) deleted

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

Changed 5 years ago by garry.yao

comment:26 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review
  1. Updating patch to current trunk;
  2. Host page scroll fixed;
  3. Floated elements in content fixed;

comment:27 Changed 5 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 5 years ago by satya

  • Keywords IBM added

Changed 5 years ago by garry.yao

comment:29 Changed 5 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 5 years ago by garry.yao

comment:30 Changed 5 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 5 years ago by garry.yao

comment:31 Changed 5 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 5 years ago by Saare

  • Status changed from review to review_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 5 years ago by dinu

comment:33 Changed 5 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 5 years ago by garry.yao

Manual tcs added to [7119].

Changed 5 years ago by garry.yao

comment:35 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

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

comment:36 Changed 5 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 follow-up: Changed 5 years ago by garry.yao

  • Status changed from review to review_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 5 years ago by garry.yao

comment:38 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

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 5 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 5 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 5 years ago by dinu

comment:41 Changed 5 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 5 years ago by Saare

  • Status changed from review to review_failed
body
{
 margin: 40px;
 padding: 20px;
}
p
{
 margin: 30px;
 border: 10px solid blue;
}

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

Last edited 5 years ago by Saare (previous) (diff)

Changed 5 years ago by garry.yao

comment:43 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:44 Changed 5 years ago by Saare

  • Status changed from review to review_failed

In Opera, TCs 5,6 fail.

Last edited 5 years ago by Saare (previous) (diff)

Changed 5 years ago by garry.yao

comment:45 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:46 Changed 5 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 5 years ago by garry.yao

comment:47 Changed 5 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 5 years ago by Saare

  • Status changed from review to review_failed

comment:49 Changed 5 years ago by Saare

  • Status changed from review_failed to review_passed

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

comment:50 Changed 5 years ago by fredck

  • Milestone set to CKEditor 3.6.2

comment:51 Changed 5 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7244].

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