Autogrow

[Gankra]

Tested in Chrome, Safari, FF22, IE10.

No tests yet, working on it.

[Gankra]

So I've got 4 tests, and got everything working, but there's a frustrating problem with Firefox and the tests. All the other browsers will update the computed height right away on import, but firefox evidently needs a few milliseconds (100 seems to work alright) to catch up. As a solution, I've add a setTimeout on autogrow when importing. This fixes the issue, but means that Firefox fails all the tests (because it's only figuring out the change happened when the setTimeout comes around).

Any thoughts on how to deal with this?

[OscarGodson]

Anything in a setTimeout is considered async so your tests need to be async. You need to use the done parameter in your tests to tell mocha to wait (up to 2seconds max) for the change to happen and then you call done(). Info on that: http://visionmedia.github.io/mocha/#asynchronous-code

I'd do a setTimeout of like 125ms and inside of that have all your checks and after your checks the done(). By this time Firefox should have had enough time to compute it all and the tests should pass.

[Gankra]

Ah, that works great (oh hai, triply nested callbacks...). All the autogrow calls are now async, too, so I guess that was necessary anyway.

I think this is ready for review.

[Gankra]

Three things I might be interested in adding to this pull:

• change autogrow to autoGrow in the options to match autoSave (note that I find it weird that autosave is used everywhere else)?
• remove the _ prefix from _autogrow. External events might cause the min/maxHeight values to change, such as a wrapper or neighbour element resizing, in which case the user might want to manually inform the editor to re-evaluate its sizing choice.
• add an autogrow event, for the reverse reason of above (may want to resize a parent or neighbour element in response).

Thoughts?

[OscarGodson]

1. A quick google search showsautogrow as one work, not camelcase. Only 1 had AutoGrow. Others were Autogrow or autowgrow. Personally I think autogrow makes more sense. When you say "autosave is used everywhere else", do you mean in our code or in other projects?
2. Yeah, makes sense, but currently the method doesn't do anything but makes it grow, right? How would you stop autogrow with it? Do you see the method doing anything else? Also, we have reflow, would it make more sense to add a param for that to be like fixed or autogrow?
3. For sure, yes.

[Gankra]

1. alright; I meant in the code. autosave is the term used in the event, for instance.
2. Autogrow, technically, makes the iframe the smallest possible size that fits the text (constrained by min/maxHeight). However since settings.autogrow.min/maxHeight can be functions, their values can be changed out from underneath the system, in which case autogrow would need to be called again to enforce this change. There's admittedly no way to enable/disable autogrow on the fly, other than set settings.autogrow.maxHeight = settings.autogrow.minHeight to effectively disable it. I think it's different from reflow, though. Although I suppose reflow is fairly meaningless when autogrow is enabled.

[Gankra]

Actually, now that I think about it, since autogrow leverages reflow, if you want to know whenever an autogrow occurs, you can just listen for reflows.

[OscarGodson]

1. Events are usually lowercase is why autosave is lowercase. i.e. webkitfullscreenchange or keypress.
2. Maybe we hold off on autogrow as a public method until it's requested? Unless you see a need right now for it. I can't think of one personally, other than turning it off and on, but not just one way. Also, maybe it could take properties to update the option on the fly like: autogrow({minHeight: '100px'})?

As for the events, good point. Maybe just note that in the docs in the events section under reflow?

[Gankra]

I'm fine with leaving _autogrow as is. I'm perfectly comfortable calling _autogrow in my own code, especially since my coworkers are deathly afraid of ever updating a dependency, and therefore won't.

[Gankra]

herpderp broke fullscreen; fixed.

[Gankra]

Is there anything else needed? Can this be merged?

[OscarGodson]

Most likely not. I just need a little time to test this. Checking out your other PR now.

[OscarGodson]

Oh, and, buttons-test is the branch

[Gankra]

Anything I can do to help this along?

[OscarGodson]

Sorry, I've been sick :\

If you wanna test the hell out of this in all the browsers I trust you to merge it in. I will test all these again when this gets released in 0.2.2.

[Gankra]

I've already tested the hell out of it (even in iOS), anything in particular?

[OscarGodson]

Just IE9 and IE10 especially. IE11 if you have it, if not, no biggie.

[Gankra]

Just noticed this corner case. Fixing those scrollbars was probably more hassle than it was worth.

Oh well.

[Gankra]

Tested in Chrome, Firefox, Safari, IE10, IE10 (IE9 mode), and iOS (only fails a minor unit test about scrolling to anchors in preview mode). I do not have access to IE11.

One thing you may not have noticed is that this diff makes autogrow:true the default in the sample running in the docs. I do this because autogrow:true is the only way to get tolerable mobile support, as far as I'm concerned (nesting vertical scrollbars are, at best, extremely confusing in mobile). Using maxHeight will also mess up iOS support.

[Gankra]

I'm not sure how you work around the issues with merge conflicts from the build directory. A bit nervous about fucking up your VC pattern. I've pushed what a think is a bearable merge to my own develop: https://github.com/Gankro/EpicEditor

That look right to push you?

[OscarGodson]

@Gankro I'll take a look tonight. I'm a little worried about a default that changes will change the layout. Most editable textarea's, by default, do not grow as far as I know. TinyMCE you have to activate a plugin, browser textareas you manually drag it larger. wysihtml5, or whatever that one is, doesn't have it either. We can always have it on by default and see how many questions or comments we get about it, but I have a feeling it'll be "surprising" to people.

As for conflicts. Is the conflict just with the min.js? If so I just rebuild, commit merge.

[Gankra]

Yea, that's what I did. There was a conflict in the actual proper js, but both of the changes were mine (changing two configs conflicted), and it was a trivial fix.

And the actual library's default is to not autogrow, it's just that the sample is set up to autogrow so that the docs page works better on mobile. I would also officially recommend that other people default to autogrowing if they wish to better support mobile.

[Gankra]

As far as messing up the doc's layout, it shouldn't. All it will do it make the rest of the page's content shift up and down, which is basically what html/css is designed for.

Whether enabling autogrow will clobber other people's layouts is a different question. I would say it's a definite posibility, although any layout that is just using it as an otherwise normal text box should probably be fine (as the height of the textbox shouldn't matter much).

Although I could see it growing so tall that it punches through some container and screws up some stuff.

[OscarGodson]

Oh! I dont care about the docs layout. Thought you meant the library's default :) Disregard my concern then. I was just worried it'd start growing on people's sites and they wouldnt know why until they looked at the docs. If they explicitly want to turn on autogrow then they assume all the problems with the layout. There's no way we could think of every edge case.

[Gankra]

Went ahead and pushed this. Willing to take any flak if it blows up in someone's face.

[OscarGodson]

Rad! Sounds good to me. I'll play with it while im at OSCON too.