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"> </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)
Change History (70)
comment:1 Changed 13 years ago by
Resolution: | → duplicate |
---|---|
Status: | new → closed |
comment:2 Changed 13 years ago by
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
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
comment:4 Changed 13 years ago by
Status: | reopened → confirmed |
---|---|
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
Version: | 3.4.2 → 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 13 years ago by
Attachment: | 8050.patch added |
---|
comment:6 Changed 13 years ago by
Owner: | set to Garry Yao |
---|---|
Status: | confirmed → review |
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
Cc: | satya_minnekanti@… added |
---|
comment:8 Changed 13 years ago by
Milestone: | → CKEditor 3.6.1 |
---|
comment:9 Changed 13 years ago by
Status: | review → review_passed |
---|
comment:10 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [7048].
comment:11 Changed 13 years ago by
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
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Besides, patch breaks quirks mode.
comment:13 Changed 13 years ago by
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
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
Test case 3:
IE
Body margin 0px
Insert a bunch of paragraphs - scrollbars will be present.
Changed 13 years ago by
Attachment: | 8050_2.patch added |
---|
comment:16 Changed 13 years ago by
Status: | reopened → 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 13 years ago by
comment:17 Changed 13 years ago by
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
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
comment:19 Changed 13 years ago by
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
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
Attachment: | 8050_5.2.js added |
---|
Changed 13 years ago by
Attachment: | 8050_5.patch added |
---|
Changed 13 years ago by
comment:21 Changed 13 years ago by
Status: | review → 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.
Changed 13 years ago by
Attachment: | 8050_3.patch added |
---|
comment:22 Changed 13 years ago by
Status: | review_passed → 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: 24 Changed 13 years ago by
8050_2.patch
- Open _samples/autogrow.html
- Click inside of the editing area
- Editor resizes itself to match the single paragraph, the editing area is very small.
8050_3.patch
- Open _samples/autogrow.html
- Scroll down to the second editor
- Click inside of the editing area
- Result: the whole page scrolls up to the top.
comment:24 Changed 13 years ago by
Replying to wwalc:
8050_3.patch
- Open _samples/autogrow.html
- Scroll down to the second editor
- Click inside of the editing area
- 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
Milestone: | CKEditor 3.6.1 |
---|---|
Version: | 3.6.1 (SVN - trunk) |
Changed 13 years ago by
Attachment: | 8050_4.patch added |
---|
comment:26 Changed 13 years ago by
Status: | review_failed → review |
---|
- Updating patch to current trunk;
- Host page scroll fixed;
- Floated elements in content fixed;
comment:27 Changed 13 years ago by
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
Keywords: | IBM added |
---|
Changed 13 years ago by
Attachment: | 8050_6.patch added |
---|
comment:29 Changed 13 years ago by
Updates:
- Considering of h-scrollbar; (Dinu's tc 1)
- Call stop the growing when scrollbar is always shown; (Dinu's tc 2)
Changed 13 years ago by
Attachment: | 8050_7.patch added |
---|
comment:30 Changed 13 years ago by
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
Attachment: | 8050_8.patch added |
---|
comment:31 Changed 13 years ago by
Saar and Wiktor pointed out there existed a problem with the autoGrow_bottomSpace config in previous patch.
comment:32 Changed 13 years ago by
Status: | review → 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.
- Load the sample with the following CSS:
- [WebKit] Host scrolling bug still exists.
Changed 13 years ago by
Attachment: | 8050_9.patch added |
---|
comment:33 Changed 13 years ago by
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 :)
Changed 13 years ago by
Attachment: | 8050_10.patch added |
---|
comment:35 Changed 13 years ago by
Status: | review_failed → review |
---|
@Dinu can u check if the latest simplified patch is functionally identical to your last patch?
comment:36 Changed 13 years ago by
Hi, not exactly; TC:
- editor fixed width 500px, body margin 0px
- contents: http://pastebin.com/sH4w3uFR[[BR]]
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: 40 Changed 13 years ago by
Status: | review → 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 13 years ago by
Attachment: | 8050_11.patch added |
---|
comment:38 Changed 13 years ago by
Status: | review_failed → review |
---|
comment:39 Changed 13 years ago by
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 Changed 13 years ago by
Changed 13 years ago by
Attachment: | 8050_12_broken_ie7.patch added |
---|
comment:41 Changed 13 years ago by
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
Status: | review → review_failed |
---|
Changed 13 years ago by
Attachment: | 8050_12.patch added |
---|
comment:43 Changed 13 years ago by
Status: | review_failed → review |
---|
Changed 13 years ago by
Attachment: | 8050_13.patch added |
---|
comment:45 Changed 13 years ago by
Status: | review_failed → review |
---|
comment:46 Changed 13 years ago by
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
Attachment: | 8050_14.patch added |
---|
comment:47 Changed 13 years ago by
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
Status: | review → review_failed |
---|
In Opera, the following TCs still fail: http://ckeditor.t/tt/8050/5.html, http://ckeditor.t/tt/8050/6.html
comment:49 Changed 13 years ago by
Status: | review_failed → review_passed |
---|
Sorry, that was a cache issue, everything looks good, congratulations.
comment:50 Changed 13 years ago by
Milestone: | → CKEditor 3.6.2 |
---|
comment:51 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [7244].
Looks like a DUP of #8014