Stop using flexbox in editor's parent hierarchy #1847

Merged
merged 5 commits into from Oct 17, 2012

Projects

None yet

3 participants

Member

Noticeably improves performance when scrolling, dragging selections, etc.

Should fix bugs #1354 and adobe/brackets-shell#61.

Many thanks to @chicu123 for suggesting this as a performance bottleneck (and @jasonsanjose for confirming by noting suspicious repaint regions). Test data from Alex's original proof-of-concept patch suggest this change may improve typing performance by 1/3 and nearly double scrolling framerate!

So, the two wrinkles with this are:

Vertical layout
Without flexbox, you'd typically have to do the equivalent vertical layout programmatically with JS -- which is no problem in this case since CodeMirror needs to get a JS call anyway each time the editor's height changes. So all that's really needed is code to calculate the desired editor height, since we can't just measure its flex container any more.

Horizontal layout
The horizontal layout of the sidebar + content area (the vertical toolbar/editor/panels stack) used to use a horizontal flexbox. It now uses 'float: left' for the sidebar and an equivalent margin on the content stack. The margin is needed since non-floated boxes actually underlap their floated siblings (only the flowed content inside them is actually pushed inward). This would have caused several problems: CodeMirror's mouse handling blocked the sidebar, and 'position: relative' items like the toolbar would have drawn on top of the sidebar.

peterflynn added some commits Aug 13, 2012
@peterflynn peterflynn Quick prototype of laying out editors without flex-box anywhere in their
parent hierarchy. Has a few bugs:
- All mouse interaction with sidebar is broken while an editor is open!
- Menu bar extends across top of sidebar.
(Both of the above are due to use of float:left for sidebar in place of .hbox)
- No-editor watermark bg isn't tall enough

To the naked eye, scrolling and drag selecting appear significantly faster
than before this patch. And Timeline view confirms that CEF no longer
repaints the entire window while typing now.
3c2aa36
@peterflynn peterflynn Merge remote-tracking branch 'origin/master' into pflynn/no-editor-fl…
…ex-box

* origin/master: (482 commits)
  ...

Conflicts:
	src/styles/brackets.less
e258103
@peterflynn peterflynn Fixes to flex-box-free layout:
* Use margin to keep content from overlapping the floated sidebar, which
broke the sidebar (toolbars drew on top of it and CodeMirror blocked all
mouse events on it).
* Fix bugs where window resize didn't kick layout enough. Now uses same
codepath as other things that resize the editor area.
* Correctly resize the "no-editor" placeholder
* Generalize _calcEditorHeight() so ALL sibling panels/toolbars are taken
into account, including those that extensions might add.
* Clarify docs in a few places
2caf138
Member

Randy noticed a bug when you use the show/hide sidebar toggle. I have a fix that I'll incorporate along with the first round of code review changes -- so don't actually merge this yet.

@redmunds redmunds was assigned Oct 16, 2012
@redmunds redmunds commented on the diff Oct 16, 2012
src/editor/EditorManager.js
@@ -313,29 +313,54 @@ define(function (require, exports, module) {
}
+ /**
+ * Calculates the available height for the full-size Editor (or the no-editor placeholder),
+ * accounting for the current size of all visible panels, toolbar, & status bar.
+ * @return {number}
+ */
redmunds
redmunds Oct 16, 2012 Contributor

This code implies that all children of editor-holder are containers which are stacked vertically. Currently they are, but someone could add a new element that doesn't play by that rule. At the very least, you should state this in a comment in the main-view.html file. You could also verify this assumption programmatically, but that may impact performance.

peterflynn
peterflynn Oct 16, 2012 Member

Good point. I'll add a comment in main-view.html.

@redmunds redmunds and 1 other commented on an outdated diff Oct 16, 2012
src/project/SidebarView.js
@@ -94,10 +94,14 @@ define(function (require, exports, module) {
$sidebar.width(width);
$sidebarResizer.css("left", width - 1);
- // the following three lines help resize things when the sidebar shows
- // but ultimately these should go into ProjectManager.js with a "notify"
- // event that we can just call from anywhere instead of hard-coding it.
- // waiting on a ProjectManager refactor to add that.
+ // This maintains the editor position to the right of the sidebar. (A simple float is not enough
+ // because the non-floated content technically underlaps the floated sidebar; this breaks CodeMirror
+ // listeners and relative positioning).
+ // TODO: Ultimately this should go into EditorManager.js, triggered via an event we dispatch
+ $(".content", $sidebar.parent()).css("margin-left", width);
redmunds
redmunds Oct 16, 2012 Contributor

Reminder: Also need to do this in toggleSidebar().

peterflynn
peterflynn Oct 16, 2012 Member

My fix for that bug you found is a little different: doing this in the isSidebarClosed case of _setWidth() rather than just the else will fix it without needing to modify toggleSidebar() (since it calls _setWidth()).

@redmunds redmunds commented on the diff Oct 16, 2012
src/styles/brackets.less
position: relative;
/* Placeholder shown when there is no editor open */
#not-editor {
- .box-flex(1);
+ height: 100%;
redmunds
redmunds Oct 16, 2012 Contributor

Yay! Good riddance flex-box!

jasonsanjose
jasonsanjose Oct 16, 2012 Member

Haha, well, if it performed well we would love to use it. Floats and margins make me nauseous.

peterflynn
peterflynn Oct 16, 2012 Member

Yep, I totally agree. Maybe one of these days we can donate to webkit the faster implementation Flex used for VBox/HBox ;-)

redmunds
redmunds Oct 16, 2012 Contributor

In theory, flex-box sounds awesome, but it took very little code to replace it. Yeah, I've been using floats and margins for years and they still seem wrong :) Maybe the sidebar/editor layout could have been implemented using inline-boxes, but we may have run into the same performance problems as with flex-box.

Contributor

Done with initial review.

@peterflynn peterflynn Fixes to flexbox-less editor layout:
* Fix bug where hiding sidebar entirely didn't update .content left/width
(SidebarView change).
* Fix bug where no-editor placeholder didn't appear on launch if no open
files were restored (EditorManager change).
* Remove unused EditorManager._resizeTimeout & rename _updateEditorSize()
for clarity
* Add note in main-view.html about assumptions/requirements imposed on
.content children's layout
8a3bac9
Member

Fixes pushed. This fixes the bug Randy spotted and one more I found (see commit comments), plus code review feedback.

@redmunds redmunds and 1 other commented on an outdated diff Oct 16, 2012
src/htmlContent/main-view.html
@@ -62,7 +62,12 @@
</div>
</div>
- <!-- Right-hand content: toolbar, editor, bottom panels -->
+ <!--
+ Right-hand content: vertical stack of toolbar, editor, bottom panels, status bar
+ (status bar is injected later - see StatusBar.init()).
+ Note: all children must be a vertical stack with fixed px heights. Otherwise the
redmunds
redmunds Oct 16, 2012 Contributor

Bottom panels are now resizable (when hooked up to resizer module), so I don't think "fixed px height" is required. Maybe I am not sure what you mean by that. Do you mean "explicit" height?

peterflynn
peterflynn Oct 17, 2012 Member

I meant explicit and in px (as opposed to percent or em). The size can change at runtime as long as EditorManager.resizeEditor() is called each time. Want me to add a comment saying that?

redmunds
redmunds Oct 17, 2012 Contributor

I think it was "px" that had me confused. I think it should just say "fixed height". There are many ways to specify a fixed length other than px such as pt, cm, in, pica, etc..

Contributor

Done reviewing. Just 1 minor question about a comment.

Member

@redmunds: pushed up a clarification to that comment.

Contributor

Looks good. Merging.

@redmunds redmunds merged commit afe3456 into master Oct 17, 2012
Member

Thanks! (Also thanks to @gruehle who acted as another scenario-testing guinea pig for this change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment