Stop using flexbox in editor's parent hierarchy #1847
Conversation
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.
…ex-box * origin/master: (482 commits) ... Conflicts: src/styles/brackets.less
* 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
|
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. |
ghost
assigned 
| * 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} | ||
| */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point. I'll add a comment in main-view.html.
|
Done with initial review. |
* 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
|
Fixes pushed. This fixes the bug Randy spotted and one more I found (see commit comments), plus code review feedback. |
| <!-- | ||
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
|
Done reviewing. Just 1 minor question about a comment. |
|
@redmunds: pushed up a clarification to that comment. |
|
Looks good. Merging. |
Stop using flexbox in editor's parent hierarchy
|
Thanks! (Also thanks to @gruehle who acted as another scenario-testing guinea pig for this change) |
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.