feat(lsp): show a picker for jump-to-definition with multiple targets

src/features/QuickViewManager.js

@@ -241,6 +241,23 @@
         animationRequest,
         quickViewLocked = false;
 
+    // True while some other menu-like UI is open - the top menu bar, a context menu (including the
+    // editor's right-click menu), the autocomplete Code Hints list, or an InlineMenu picker like
+    // jump-to-definition's multi-target picker (see languageTools/DefaultProviders.js) or Extract
+    // to Variable/Function. QuickView shouldn't pop up while any of those has the user's attention.
+    // All of them share the same underlying convention - a `<li class="dropdown ...">` root that
+    // gets an "open" class added while shown (see command/Menus.js openMenu()/registerContextMenu()
+    // and the codehint-menu/inlinemenu-menu markup in CodeHintList.js/widgets/InlineMenu.js) - so
+    // one generic selector covers all of them, including any future dropdown-based popup, without
+    // needing to name each one. Checked live at draw time, deliberately not via an imperative
+    // suppress/unsuppress pairing: those callers can close their popup through several different
+    // paths (Esc, selecting an item, clicking elsewhere - InlineMenu in particular doesn't notify
+    // on a plain click-away), and a state flag that isn't reliably unset on every one of those
+    // paths gets stuck "suppressed" forever.
+    function _isOtherEditorPopupOpen() {
+        return $(".dropdown.open").length > 0;
+    }
+
     // Constants
     const CMD_ENABLE_QUICK_VIEW       = "view.enableQuickView",
         QUICK_VIEW_EDITOR_MARKER = 'quickViewMark',
@@ -599,7 +616,7 @@
             clientY: event.clientY
         };
 
-        if (!enabled || quickViewLocked
+        if (!enabled || quickViewLocked || _isOtherEditorPopupOpen()
             || $previewContainer[0].contains(window.document.activeElement)) {
             // activeElement check as, if the popup has an active element, say a text input, user may
             // move the mouse outside popup to type in the input, in which case we should not close popup.

src/languageTools/LSPClient.js

@@ -341,6 +341,7 @@ define(function (require, exports, module) {
         "textDocument/signatureHelp": "Sig",
         "textDocument/hover": "Hover",
         "textDocument/definition": "Def",
+        "textDocument/implementation": "Impl",
         "textDocument/references": "Ref",
         "textDocument/codeAction": "Fix",
         "completionItem/resolve": "Res",
@@ -570,14 +571,15 @@ define(function (require, exports, module) {
         return deferred.promise();
     };
 
-    LanguageClient.prototype.gotoDefinition = function (params) {
-        const self = this;
+    // Shared by gotoDefinition/gotoImplementation - both are "resolve a position to one or more
+    // {uri, range} locations" requests that only differ in LSP method name.
+    function _requestLocations(client, params, method) {
         const deferred = $.Deferred();
         (async function () {
             try {
-                await DocumentSync.flush(self, params.filePath);
-                const result = await self._request("textDocument/definition", {
-                    textDocument: { uri: self.uriForPath(params.filePath) },
+                await DocumentSync.flush(client, params.filePath);
+                const result = await client._request(method, {
+                    textDocument: { uri: client.uriForPath(params.filePath) },
                     position: _positionOf(params.cursorPos)
                 });
                 if (!result || (Array.isArray(result) && !result.length)) {
@@ -600,6 +602,18 @@ define(function (require, exports, module) {
             }
         }());
         return deferred.promise();
+    }
+
+    LanguageClient.prototype.gotoDefinition = function (params) {
+        return _requestLocations(this, params, "textDocument/definition");
+    };
+
+    // Finds concrete implementations of the symbol at a position. Used as a fallback when
+    // gotoDefinition resolves to a single canonical declaration (e.g. a base class/interface
+    // method) for what is actually a polymorphic call site - see DefaultProviders.js doJumpToDef
+    // and https://github.com/phcode-dev/phoenix/issues/3093.
+    LanguageClient.prototype.gotoImplementation = function (params) {
+        return _requestLocations(this, params, "textDocument/implementation");
     };
 
     LanguageClient.prototype.findReferences = function (params) {

src/nls/root/strings.js

@@ -1577,6 +1577,9 @@ define({
 
     // extensions/default/JavaScriptCodeHints
     "CMD_JUMPTO_DEFINITION": "Go to Definition",
+    "JUMPTO_DEFINITION_SELECT_TARGET": "Select a definition",
+    "JUMPTO_DEFINITION_LINE_LABEL": "Line {0}",
+    "JUMPTO_DEFINITION_IMPLEMENTATION_BADGE": "Implementation",
     "CMD_SHOW_PARAMETER_HINT": "Show Parameter Hint",
     "NO_ARGUMENTS": "<no parameters>",
     "CODE_HINT_IMPORT_FROM_N": "{0} imports…",

src/styles/brackets.less

@@ -2639,6 +2639,39 @@ a, img {
     p:first-child { margin-top: 0; }
     p:last-child { margin-bottom: 0; }
 
+    // File + line label the jump-to-definition multi-target picker prepends before its code
+    // excerpt - see languageTools/DefaultProviders.js showJumpTargetPicker()/showExcerptFor().
+    // Filename (+ dimmer directory, when the candidate isn't in the file you jumped from) on the
+    // left, line number flush to the right.
+    .jump-to-def-excerpt-title {
+        display: flex;
+        justify-content: space-between;
+        align-items: baseline;
+        gap: 10px;
+        font-size: 11px;
+        color: @bc-text-medium;
+        margin-bottom: 4px;
+
+        .dark & {
+            color: @dark-bc-text-medium;
+        }
+
+        &-name {
+            overflow: hidden;
+            text-overflow: ellipsis;
+            white-space: nowrap;
+        }
+
+        &-path {
+            opacity: 0.6;
+        }
+
+        &-line {
+            flex: 0 0 auto;
+            white-space: nowrap;
+        }
+    }
+
     // Headings (e.g. the package-name header in npm hint docs): kill the browser's large default
     // margins - a first-child heading otherwise adds its top margin to the popup's own padding and
     // the spacing reads top-heavy vs the bottom.

src/styles/brackets_patterns_override.less

@@ -694,31 +694,80 @@ a:focus {
             }
         }
 
-        // Styles used for inlinemenu widget header
+        // Styles used for inlinemenu widget header. Previously a solid @bc-panel-bg-alt chip -
+        // against the menu's own @bc-menu-bg/@dark-bc-menu-bg surface (white / near-black) that
+        // read as a mismatched floating block sitting on top of the list, especially in dark theme.
+        // Instead, keep it on the same surface as the rest of the menu (no background at all) and
+        // read it as a plain muted title with a hairline divider separating it from the items
+        // below. Same treatment as the code-excerpt popup's own title bar
+        // (.jump-to-def-excerpt-title in brackets.less), so the two pieces of one picker look like
+        // one cohesive, deliberately minimal surface instead of two.
         li.inlinemenu-header a {
-            background-color: @bc-bg-tool-bar;
-            color: @bc-inlinemenu-text;
-            padding-bottom: 5px;
+            background-color: transparent;
+            color: @bc-text-medium;
+            padding-top: 6px;
+            padding-bottom: 6px;
+            // Same divider color this menu family already uses for its .divider rule above, so the
+            // header's separator reads as an intentional, on-brand line rather than an arbitrary
+            // one-off - and unlike a low-alpha white/black overlay, it stays visible against the
+            // near-black @dark-bc-menu-bg surface.
+            border-bottom: 1px solid @bc-menu-separator;
 
             .dark & {
-                background-color: @dark-bc-bg-tool-bar;
-                color: @dark-bc-inlinemenu-text;
-                padding-bottom: 5px;
+                color: @dark-bc-text-medium;
+                border-bottom-color: @dark-bc-menu-separator;
             }
 
             &:hover {
-                background-color: @bc-bg-tool-bar;
-                color: @bc-inlinemenu-text;
-                padding-bottom: 5px;
+                background-color: transparent;
+                color: @bc-text-medium;
 
                 .dark & {
-                    background-color: @dark-bc-bg-tool-bar;
-                    color: @dark-bc-inlinemenu-text;
-                    padding-bottom: 5px;
+                    color: @dark-bc-text-medium;
                 }
             }
         }
 
+        // Muted directory suffix on jump-to-definition's multi-target picker items - see
+        // languageTools/DefaultProviders.js showJumpTargetPicker(). Scoped here since
+        // .quick-open-path (used for the same muted-path look in QuickOpen) only applies under
+        // .quick-search-container.
+        .jump-to-def-item-path {
+            color: @bc-text-medium;
+            font-size: 11px;
+
+            .dark & {
+                color: @dark-bc-text-medium;
+            }
+        }
+
+        // "Implementation" marker shown flush right on a jump-to-definition item's row when the
+        // row represents one (see showJumpTargetPicker's implementationStartIndex) - the
+        // declaration row and its implementations otherwise look identical once merged into one
+        // list, with nothing else to tell them apart. Right-aligned via float, deliberately not
+        // display:flex on a wrapper: that was tried and reliably added ~11px of unexplained row
+        // height, because a block-level flex child inside InlineMenu's own inline <span> wrapper
+        // (widgets/InlineMenu.js _addItem) gets boxed in an anonymous block that still carries the
+        // parent's own line-height as a "strut". A float is taken out of normal flow entirely, so
+        // it reaches its containing block (the menu item's <a>, which is display:block) without
+        // that side effect.
+        .jump-to-def-item-badge {
+            float: right;
+            font-size: 11px;
+            color: @bc-text-medium;
+            // fa-code-branch reads as a vertical git-branch glyph by default - lay it on its side
+            // so it reads more like a generic "derives from" mark than a literal branch icon.
+            transform: rotate(90deg);
+            // li a's own right padding (see the shared "li a" rule above) is 20px, sized for plain
+            // text - pull the badge back into most of that so it doesn't float in its own patch of
+            // empty space, while still leaving a little breathing room off the popup's edge.
+            margin-right: -14px;
+
+            .dark & {
+                color: @dark-bc-text-medium;
+            }
+        }
+
         li {
             a {
                 // Less padding than on menus. More padding on top than bottom

test/UnitTestSuite.js

@@ -36,6 +36,7 @@ define(function (require, exports, module) {
     require("spec/EditorRedraw-test");
     require("spec/EditorCommandHandlers-test");
     require("spec/EditorCommandHandlers-integ-test");
+    require("spec/JumpToDefinitionMultiTarget-integ-test");
     require("spec/EditorManager-test");
     require("spec/EncodingDetector-test");
     require("spec/EventDispatcher-test");