Working set context menu pops closed when right-clicking file that hasn't been opened yet

[peterflynn]

1. Place several files in the working set
2. Restart Brackets
3. Right-click a working set entry that hasn't been viewed in the editor yet

Result:
Context menu appears briefly, then closes again.

Expected:
Context menu stays open.

Workaround:
Right-click it again.

[peterflynn]

This also happens when right-clicking files in the project tree (if they are not open in an editor, i.e. not in the working set and not currently selected).

[RaymondLim]

I can't reproduce it on Win7. So must be a mac only issue.

[peterflynn]

I can repro on WinXP. I'm guessing it depends on how quickly the file is opened (try with a larger file, perhaps, if you're on a faster system?).

[pthiess]

Reviewed might be related to the popup management bug #1381. In Trello Card 643, closing.

[pthiess]

Re-opend since we want to keep issues visible if not ECR.

[peterflynn]

So here's how the bug plays out:

1. Right mousedown causes WorkingSetView to ask to open the document (via event.which !== 1 check in _reorderListItem() calling drop())
2. We begin loading the document from disk
3. Right mouseup followed by contextmenu event. Context menu is popped open.
4. Done loading the document; currentDocumentChange fires
5. In response, ProjectManager._redraw() is run three times. Evidently in order to "reposition the selection triangle," it forcibly triggers a scroll event on the file tree.
6. Whenever scroll occurs on the file tree, ProjectManager calls Menus.closeAll() -- snapping the context menu shut again.

In scenarios where the disk is fast or the file was already loaded, currentDocumentChange occurs before mouseup and thus the context menu opens after closeAll().

Seems like ths ideal fix is, don't fake a scroll event to trigger updating the triangle position. Barring that, we could detect fake scroll events and only call closeAll() when a real user-driven scroll has occurred.

[peterflynn]

Actually, there's a subtler bug here too: in the interval where the context menu has been opened but the document isn't yet loaded, the tree selection still lies on the old item. This means that if you manage to click a context menu command during that time, it will actually apply to the wrong file.

Probably near-impossible with our current local-disk APIs, but in browser with a network backend this could be a real problem. Along with many other similar bugs we have lurking -- so we should probably just spin that off as a separate issue.

[njx]

Bumping to medium priority since a number of people have run into it. I think we have too many bugs open for sprint 27 already, but should consider it for the next sprint.

[lkcampbell]

I did a little bit of research on this bug today.

Just as a test, I commented out the fake scroll event in ProjectManager._redraw(). It does get rid of those three Menus.closeAll() calls that come from the triggered fake scroll events, as @peterflynn said, but it still doesn't fix the bug because there is another Menus.closeAll() call that occurs after those three calls.

The last call is coming from Editor.js. It is an anonymous function that fires on the CodeMirror scroll event. I assume the Editor is scrolling somehow as the new document is loaded. So, this needs to be addressed as well.

[njx]

The "always close popups on scroll" behavior also causes #3391. I wonder if we need a smarter heuristic here, but I'm not sure what it would be. At the very least, scrolls in one area shouldn't affect popups that are associated with completely different areas of the UI. But even within a given area the existing behavior is problematic because the scrolls are happening asynchronously after the popup appears.

[lkcampbell]

@njx and @peterflynn, I think I will spend some more time researching this bug. That is, unless you guys were planning to make it a Sprint 31 bug. Otherwise, I am going to work on it this long weekend.

If you have any ideas or suggestions for possible solutions, I would welcome them as well.

[njx]

@lkcampbell Thanks for looking at this! I don't have any ideas off the top of my head...let us know what you come up with.

[lkcampbell]

@njx and @peterflynn, I tried to keep the fix as simple as possible. Instead of triggering a scroll event, which is much too broad just to update the triangle selector, I made a new, specific event, selectionRedraw, that only updates the selector. That way, we don't get the undesirable extra effects of a triggered scroll event.

[lkcampbell]

Also, the problem I was seeing, where the anonymous function in Editor.js is sending a scroll event that closes all menus, seems to occur much less frequently now. I have a difficult time reproducing it whereas, when I originally documented that observation, it happened almost every time I opened a new document.

I think the problem of a broad-based closing of popups needs to still be address, as @njx mentioned, but since it doesn't appear to be causing any major problems in this issue any more, I will try to address it in a fix for issue #3391.

[jasonsanjose]

FBNC @peterflynn

[lkcampbell]

@peterflynn, reminder FBNC.

[peterflynn]

Hmm, I got one case where the menu simply didn't appear at all, but I haven't been able to reproduce it. Seems fixed in general. I'll file a new bug if I can find reliable steps for that one glitch.

[lkcampbell]

@peterflynn, if it is at all helpful for discovering the repro steps, this disappearing menu problem might be due to a Scroll event listener that is set up in Editor.prototype._installEditorListeners. I have not yet been able to figure out why the Editor is firing off a scroll message when changing from one file to another but I have been able to trace the problem to a scroll from an anonymous function in Editor.