Need to consolidate popup handling code into PopUpManager

[njx]

(Original description: Clicking on code hint menu item or outside of code hint menu doesn't go through CodeHintList.close())

When the user dismisses a code hint menu using ESC or hitting Return, CodeHintList.close() gets called, but it doesn't get called if the user just clicks on a menu item or clicks out of the menu--those only go through clearMenus() in bootstrap-dropdown.js. This doesn't seem to have any major user-visible issues right now since the cleanup we do in CodeHintList.close() isn't critical, but there may be a memory leak here since we maintain a list of popups in PopUpManager. We should make sure CodeHintList.close() gets called from all the relevant code paths.

(Long term, we might just want to ditch bootstrap-dropdown.js entirely--it doesn't really do all that much and we could just centralize that logic in PopUpManager.)

[njx]

I'm planning to fix the click-on-code-hint case, but there isn't a simple fix for the click-outside-menu case since that goes directly to the bootstrap-dropdown handler (attached to html). I think eventually we need to replace bootstrap-dropdown so we can get more control/notifications over cases like these.

Once we fix that case, we should be able to get rid of the logic in CodeHintList.isOpen().

[pthiess]

reviewed assigned to NJ.

[njx]

A few other things to think about when we centralize popup handling in PopupManager:

• Unlike bootstrap-dropdown, it shouldn't assume that the dropdown is inside a top-level menu bar created with <ul><li>.
• The recent projects extensions should be rewritten to use this new popup handling (so things like Esc key handling will close the dropdown).

[njx]

Already fixed the recent project extension to use PopUpManager, but the fact that we don't have a general way to close all popups when a new one is created is causing a number of problems--we have various patches in various places to make this happen.

[njx]

Another thing to consider when we fix this: we currently use the sledgehammer of Menus.closeAll() whenever we do something like scroll the project tree or the code view. This can make it so that unrelated popups end up closing (for example, the project tree scrolling causes the code hint popup to close). We should have a better scheme for managing this.

[njx]

Added a placeholder card to the backlog for this: https://trello.com/c/o7jf6s6W

[peterflynn]

Before ditching bootstrap-dropdown we should probably check whether Bootstrap 2.x has improved the functionality in ways we might find useful. I assume they must have done some work on it to support cascading sub menus...

But given that due diligence, I'd have no objection to ditching it -- in general the JS parts of Bootstrap seem so deliberately bare-bones that they border on useless.

[njx]

Yet another cleanup that should ideally be done as part of this...context menus and code hints have similar (duplicated) code for creating and positioning popups. (They're actually using the Bootstrap CSS infrastructure for popups, although they're not actually using the JS in bootstrap-dropdown.js.) Recent projects is doing something different, and isn't taking into account the window borders the same way context menus and code hints do (which is generally okay since it pops up close to the left edge of the window, but has problems if the menu gets long enough to overlap the bottom of the window).

PopUpManager should take care of all this creating and positioning in a consistent way across all popups. This will be useful for other things that want to pop something up, like toolbar icons that need their own menus. Once this is built, it would also be easy to generalize it to display a "speech balloon"-style popover--that would probably just be a slightly different skin on the same popup.

[peterflynn]

Another consideration: should we try to break the dependency of Editor on Menus, which causes two of the circular dependencies in #1998. The dependency exists only to close popups on scroll.

[njx]

Yes--Editor should just depend on PopUpManager, though it might be the case that PopUpManager will still need a dependency on EditorManager eventually. (Right now it's not actually using that dependency.)

[njx]

Yet another thing to clean up here: make the dropdown styles from brackets_patterns_override.less more general so they can apply to dropdowns in extensions (right now they're hardcoded to only apply to .toolbar .nav, .context-menu, .codehint-menu). For example, there's redundancy in the RecentProjects extension because of this (we have to duplicate the item hover color)--see #2489.

[redmunds]

Consolidating issue #1411 into this one. Remaining issues are summarized in this comment: #1411 (comment)

[njx]

See also #5648. Bumping this back up to medium priority so it's on our radar.

[njx]

Actually, closing as move to backlog--we already have a card pointing to this: https://trello.com/c/o7jf6s6W