Toggling fullscreen progressively shrinks the editor with a border

[archseer]

I have the editor wrapped around a textarea, inside a form:

form
  #epiceditor
    textarea

The form is set to max-width: 720px and #epiceditor has a 1px #ccc border. So that means the editor is 718px wide + 2px border -- the internal iframe width is 718px (and EpicEditor correctly sets the style to "width: 718px").

Toggling on and off the fullscreen incorrectly sets a "width: 718px" on #epiceditor, and that in effect breaks the style on the iframe, which is now "width: 716px". Repeating the toggling on and off continually shrinks the area by 2px (note: if we set the border to a greater value, it shrinks that much more). Somewhere along the style calculations, the border is not correctly accounted for.

[OscarGodson]

Thanks for the report. I'll take a look at this and try to get it into the next release which will be whenever I fix this bug if it exists (which I believe it does).

[archseer]

I'll try and do some digging myself, but I believe you being the author, will be able to find it much easier than me :)

[OscarGodson]

Here's some links if you do want to take a look

• Here's all the code that saves the styles to be reset later: https://github.com/OscarGodson/EpicEditor/blob/develop/src/editor.js#L743-L798
• Here is where the styles are reset: https://github.com/OscarGodson/EpicEditor/blob/develop/src/editor.js#L821-L824
• Here's the _saveStyles code: https://github.com/OscarGodson/EpicEditor/blob/develop/src/editor.js#L53-L78
• Here is where I think the issue might be: https://github.com/OscarGodson/EpicEditor/blob/develop/src/editor.js#L749-L752

[archseer]

The issue is not in that place, I've tested it by commenting it out, besides, this caches the window height (for fullscreen), while we have problems with the editor div width (in normal mode).

The problem is here: https://github.com/OscarGodson/EpicEditor/blob/develop/src/editor.js#L829-L830

For some reason the reflowWidth is set, despite this being a dynamic container.

One thing I am not yet sure of is how the saving and restoring of styles works, because on the initial page load, div#epiceditor has no inline style other than height. Then by going into the fullscreen and back out, a large list of styles seems to be "restored"...

[archseer]

As far as the solution goes, simply removing those two lines works. On dynamic containers, no width is "restored", while on fixed width containers, the proper style is reapplied. It Just Works™

So I guess this commit should be reverted or fixed: https://github.com/OscarGodson/EpicEditor/commit/57f068f179f33a85a61986d3310eb40eaa73f3f6

[OscarGodson]

One thing I am not yet sure of is how the saving and restoring of styles works, because on the initial page load, div#epiceditor has no inline style other than height. Then by going into the fullscreen and back out, a large list of styles seems to be "restored"...

The styles are computedStyles. For example, even tho there's no inline styles on this text in this comment, the computedstyle is black. I grab all the styles I'll be modifying like the width and height and then upon closing fullscreen put the computed styles back to how they were before.

If you remove those two lines, do all the tests pass?

[OscarGodson]

Also: https://github.com/OscarGodson/EpicEditor/issues/160#issuecomment-9132651

You'll see my comment about fullscreen and reverting the styles:

1 bug when you reflow, then go into fullscreen, then close fullscreen, it's sized to how it was at the initial load of the editor

[archseer]

Yes, I do know what computed styles are, but what you're doing just makes no sense. If you want to "put the computed styles back to how they were before", why don't you just restore the inline styles to how they were at the initial page load, without explicitly writing the computed styles, instead allowing the browser to compute the computed styles on it's own? This text sure is black, but if I modify the style to change it to red, then I don't need to explicitly set it to color: black to restore it to it's defaults, just remove my modification from it's inline styles.

I have no idea how I could run tests, because mocha returns "ReferenceError: rnd is not defined", but I guess you could try commenting out those two lines yourself and run the tests.

[OscarGodson]

why don't you just restore the inline styles to how they were at the initial page load

The code to do this was added like a year and a half ago. I very well could have done this a better way, but I promise I didn't write all that code for no reason :) in fact, fullscreen and styling is was one of the hardest bits of code I remember writing for this to handle all of people's implementations. An element can be updated for all sorts of reasons: jQuery plugins, JS animations, etc. If, for example, you have a JS animation to fade it in or just .show() it will update the inline style. If you simply remove the inline style it'll revert back to the original CSS, so display:hidden. The idea is to put it back exactly how we found it. Closing fullscreen and handling multiple browsers and implementations has been tough.

If you know of a better way to deal with this and remove a bunch of code, I'd love to see what you come up with and I'm happy to review it. Unfortunately tho, I don't have time personally to redo all this code which has been working "ok" aside from a few bugs here and there; no more than any other part of the editor. The fullscreen stuff is super kludgy—I agree—and I'd love help cleaning it up, but right now I'm in the mode of stabilizing it so I can start pushing features people have been asking for.

I'm for sure looking into this and want to fix it this release.

As for testing: https://github.com/OscarGodson/EpicEditor/wiki/Contributing#requirements — do you have all of that installed?

[archseer]

I've guessed why you did it that way, and the way I'd fix this is by doing something among the lines of what you do now: you cache (or "save") the attributes at a certain point, and you restore them back to what they were when you exit the fullscreen. But instead of caching all of the attributes we're using, we should first check whether that attribute actually needs to be cached -- that is, is the attribute actually explicitly specified in the inline style?

So it would work somewhat along the lines of: If we have for instance, a div with a style="color: red; padding: 2px", and we want to use color and margin, when we cache, we check if the inline style contains any of those two, and we store them if they do (so as you can see, we store color, but not the computed margin). Then we modify it to our liking, let's say style="color: black; margin: 5px; padding: 2px")

When we want to restore it back using the cache we stored, we simply wipe all of the attributes we were using (so now we have style="padding: 2px"), then we add the values from our cache (style="color: red; padding: 2px"). Simple. The only difference we're doing is that we're checking whether the attributes we want to cache are not computed, but actual inline styles, and we make sure we wipe out any that didn't exist before our meddling.

For cross browser checking whether an inline style rule exists, we can use http://stackoverflow.com/a/476750/727386

Yes, I think I do:

node install -g jake mocha
node install
mocha

[OscarGodson]

You need to install all deps including Marked, for example. Did you do npm install too? If you did that and you're stilling getting that error I can take a look. The docs say:

Additional dev dependencies can be installed from the EpicEditor directory via npm as follows:

$ cd EpicEditor
$ npm install

If that's wrong I want to fix it.

For the bug/fix tho: it seems OK but we'd still need to fix this if all the required attributes have changed, right? Your suggestions seems more about optimization than a fix for this since we'd need to handle this even if there was inline styles.

[archseer]

Oops, that was a typo node-> npm. I did exactly that. I suggest you try running the tests on your own (or set up Travis, it's integrated into Github too)