Rethink code navigation with CodeMirror token selection

[olafurpg]

Previously, the token cursor feature flag used HTML href links for go to definition. This commit changes the behavior of the feature flag to use CodeMirror selections instead along with several other big changes:

• Cmd+Click for Go to definition
• Enter for Go to definition
• Space for Show popover
• Up/Down/Right/Left arrow to move the selection to the next token

Demo https://www.loom.com/share/a0fa53badd4d419081dcfd1a2fbc7e4e

Changes since the demo was recorded

• The hover popover looks the same as in the main branch
• No more context menu
• Improved accessibility
• Improved history handling

Please give it a try and let us know what you think.

TODOs before this PR is ready for review:

• Underline token when definition is available and meta key is being held
• Update selection when moving history inside current file
• Handle multiple definitions by linking to ref panel
• Document highlights
• Use ctrl key on linux and windows
• Highlight hovered token
• Only show pointer when over an interactive token
• Separate tooltip for hover and definition
• Allow cmd+click on any token including non-occurrences
• Move feature flag under experimental
• Fix position of "No definition found" tooltip (show at cursor)
• Fix stack trace from Thorsten
• Fix stack trace from Noah. Steps: select token, space for popover, click on right margin.
• Render link for "right-click > open in new tab" and middle-click
• Support long-press goto-def
• Support comprehensive navigation in search-based intel for languages with low-quality highlighting data
• Document new goto-def triggers in hover popover
• Trigger find-ref on goto-def at the definition (save vscode and intellij)
• Avoid pushing cycles into history (repeated goto-def from same location)
• Fix failing test cases

Improvements for the future: see https://github.com/sourcegraph/sourcegraph/issues/45006

Test plan

To test it manually with a local build. First start the server. Note that it's not possible to use web-standalone.

sg start 

Next, update setting "experimentalFeatures": {"codeNavigation": "selection-driven"}}.

App preview:

• Web

Check out the client app preview documentation to learn more.

[sg-e2e-regression-test-bob]

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (+0.11 kb)	0.11% (+16.65 kb) 🔺	0.14% (+16.54 kb) 🔺	0.69% (+5) 🔺

Look at the Statoscope report for a full comparison between the commits a443d8d and 63ba073 or learn more.

Open explanation

• Initial size is the size of the initial bundle (the one that is loaded when you open the page)
• Total size is the size of the initial bundle + all the async loaded chunks
• Async size is the size of all the async loaded chunks
• Modules is the number of modules in the initial bundle

[olafurpg]

I prematurely marked this PR as ready for review. I'm very happy with the functionality, but I'm planning to self-review the diff on Monday and clean up the code before tagging people for review.

[olafurpg]

@limitedmage Can you advice on the most appropriate role for the blob view? The "region" role did not work well with hjlk navigation.

[lrhacker]

I don't think main would be valid here, because we are using a <main> element in AppRouterContainer, which wraps the blob view. According to the MDN docs:

There should only be one main landmark role per document.

The <main> element has a role of main. Developers should use semantic HTML — in this case <main> — over using ARIA.

Can you elaborate more on what didn't work well when it was still region?

[olafurpg]

You're totally right that main is inappropriate. I've reverted this change so that it's region again. Selection driven navigation will need more iterations to get accessibility right. When I tried hjkl navigation with the region role then VoiceOver didn't speak out the selection.

[limitedmage]

I think region is fine. Does it show up in the Landmarks menu? (VoiceOver+U, use left/right arrows to go between menus).

[sourcegraph-bot]

Not notifying subscribers because the number of notifying subscribers (20) has exceeded the threshold (10).

[sourcegraph-bot]

Codenotify: Notifying subscribers in OWNERS files for diff 50fe3e7...a443d8d.

No notifications.

[olafurpg]

OK, we have a green CI and this PR is ready for review. I've cleaned up the implementation and I'm very(!) happy with the functionality. A few highlights of the changes since I recorded the Loom:

• We now render HTML links (<a href) once the definition has finished loading. This allows for features like "right-click>open link in new tab".
• Graceful fallback for languages with low-quality syntax highlighting data using CodeMirror's builtin word detection. Keyboard navigation won't work completely for this languages, but mouse navigation does.
• Significantly improved history handling. The editor selection state and the URL are properly synced now and it's much more intuitive what locations get pushed into the history (and what locations are not pushed).
• Hover popover looks the same as in our current app but the "Go to definition" button has been replaced with a ❓ tooltip explaining the new functionality.
• Reorganized feature flags and made the new functionality entirely hidden behind the flag.
• Feature parity: almost all codeintel features have been implemented (multiple defs, find refs, document highlights, find impls, ...). The only unimplemented feature I'm aware of is popover pinning, which we can fix separately.

I'm eager to get this PR merged so that we can collect more feedback before deciding what to do with this feature flag.

[lrhacker]

This is awesome - I will make time to take a look at it tomorrow! (cc @sourcegraph/code-exploration for visibility)

@olafurpg Would you be able to compare the work you've done against the items listed in this tracking issue and note which issues you think it should close in the PR?

[felixfbecker]

I'm getting "invalid settings: experimentalFeatures: Additional property codeNavigation is not allowed" when trying this out, is there something I'm doing wrong?

[olafurpg]

@felixfbecker Have you tried with sg start? This feature flag is not available with web-standalone.

[olafurpg]

@lrhacker This PR enables keyboard navigation but I don't think the solution is fully accessible as is. When using VoiceOver, the currently selected token is read out but you have to use hjkl navigation instead of up/down/right/left. To achieve this, I made a small change to use the main role for the blob component (instead of region) and adding the aria-current=true attribute to the selected token. We can fairly easily customize the HTML structure to add any other roles or aria attributes, but I don't think we can use links for the reasons mentioned here https://docs.google.com/document/d/1cXZEXfui1ylXBfNoB5iK7sUYhbNm51Y7wQMW36RScTA/edit#

[taras-yemets]

Great work, @olafurpg!

Here are some comments and random thoughts I had while testing it.

Description	Attachment
Code-intel popup width seems too narrow
'You are at the definition' and 'Loading' popups need some padding and brand styling
Some tokens (like the arrow on the screenshot) are not expected to have valuable code intel information. Maybe they shouldn't support 'Space' clicks for code-intel popups display. Hovering these tokens on dotcom doesn't trigger the code-intel popup.
I got some unexpected code-intel popup content which I didn't get on dotcom. Not sure if it may be caused by my local Sourcegraph instance.
Selecting a token and opening a code-intel popup with 'Space' key and then navigating to another token with arrow keys leaves the popup open which is nice in my opinion. But the token we see the code-intel popup for is not longer highlighted in any way and it may not be clear the popup for which token exactly is open (especially when there are a few tokens in a row)
If we open the popup for a selected token and then navigate to another one and want to open a code-intel popup for it we need to press 'Space' two times: the first one to close the existing code-intel popup and the second one to open the one for the currently selected token. I think it could be better to open a popup for a selected token with a single press	https://user-images.githubusercontent.com/25318659/204614611-bfec5658-6508-4bf2-8b7f-fabdaa8daa58.mov

I haven't tested these changes from the accessibility standpoint, will do it shortly.
I also haven't checked whether the keybindings and overall approach match the suggested keyboard interactions system (RFC, thread).

[lrhacker]

A few different chunks of feedback and comments so far. I like @taras-yemets's table but this might be a little verbose for columns (sorry 😅).

---

From that tracking issue I linked, I do think this satisfies #43175, in that its goal is to include up/down/left/right keyboard navigation. The primary difference from the acceptance criteria I see is that up/down navigation now focuses on a single token, whereas what @umpox had prototyped originally was selecting just line to start with (and left/right navigation would start token selection).

One question: in your Loom it appears that the current line is highlighted along with the token, but I'm not seeing that locally - is that an expected change? (I personally prefer to see the line highlight so it's easier to see where I am.)

---

For #42421 from the tracking issue, while I wouldn't expect this PR itself to include tab-driven navigation, that is something I find myself wanting. Or, maybe more specifically, I want to make sure that pressing tab doesn't put me in an unexpected navigation situation.

Currently, if tab is typed while in the blob view, it will switch focus to the actions panel, and then the keyboard navigation/controls for the blob view do not work. If you tried to hit space at this point to close an open intel popover, it would instead trigger a click on the next element that's focused (in my case, the "View on GitHub" link).

At this point, the goal would just be to make sure we aren't intentionally doing anything in this PR that would make supporting tab-driven navigation prohibitively challenging in the future.

---

The RFC that @taras-yemets linked above will also be something to consider and be a good place to discuss what specific keyboard interactions we want to settle on. Not that we can't differ from what's in the RFC today, but it will be good to make sure we're all in agreement with what is expected (e.g. should enter immediately go to definition like in this PR, or just open the intel popover, as suggested in the RFC and #42421).

Side note: one specific interaction specified in the RFC that I found myself missing here, though, is using esc close the intel popover. 😄

---

In terms of accessibility, I still need to do more testing, but I think we'll need to keep iterating on interactions with VoiceOver (or, more generally, all screen readers) over time. At the very least, it's not always the simplest problem to solve on the first pass.

When I did a very quick test, using the actual VoiceOver controls for navigation (e.g., Ctrl + Opt + Right arrow) does not selecting things as expected (the entire row of text is selected/read). I still need to do more research on standards around supporting specific screen reader controls, versus having other keyboard-based navigation available (e.g. h/j/k/l) that works in tandem with the reader to announce something meaningful. @limitedmage do you happen to have any resources on this?

That being said, there are almost certainly some things we can do using ARIA roles to better describe the tokens. The first thing that comes to mind would be to use something like the aria-expanded (or similar) role on each token to indicate it has a menu it can open (the intel popover). That might not be exactly the correct role in this case, but the general idea is that we should be able to find a way to mark them as interactive and what they are intended to do.

---

Lastly, like @taras-yemets mentioned, the intel popover does not close when you begin navigating again. I think the preferred interaction for now would actually be to close it when you start navigating again, because it easily has the potential to cover/hide where the selection has moved to, like so:

[sourcegraph-bot]

Codenotify: Notifying subscribers in CODENOTIFY files for diff 50fe3e7...a443d8d.

No notifications.

[olafurpg]

@taras-yemets Thank you for the detailed review. Thanks to your suggestions, I was able to remove facets.ts altogether including codeintelFacet resulting in much cleaner code 😊 The token-selection extension now accepts no parameters since everything is already part of blobPropsFacet.

Can you please repost your accessibility comment in https://github.com/sourcegraph/sourcegraph/issues/45006 ? It's outside the scope of this PR to fix accessibility.