blob page keyboard navigation by taras-yemets · Pull Request #46829 · sourcegraph/sourcegraph-public-snapshot

taras-yemets · 2023-01-24T07:28:36Z

Closes https://github.com/sourcegraph/sourcegraph/issues/46375

Adds keyboard shortcuts to switch between blob view, file, and symbol trees.
Available under blob-page-switch-areas-shortcuts feature flag.

Screen.Recording.2023-03-02.at.18.47.40.mov

Adds shortcut labels to the code view and files/symbols tags. I asked for design help in this thread. I think it's not blocking as this feature is behind the feature flag and we can address the feedback as we get it.

Test plan

Tested manually (video attached).

App preview:

Web

Check out the client app preview documentation to learn more.

sg-e2e-regression-test-bob · 2023-02-17T09:52:40Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.01% (+0.24 kb)	0.01% (+2.08 kb)	0.02% (+1.83 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits d6810db and fa271fb 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

taras-yemets · 2023-03-02T16:46:56Z

-                if (event.key.length === 1) {
-                    let currentId = getNextAccessible(data, id, expandedIds)
-                    while (currentId !== id) {
-                        if (currentId == null) {
-                            currentId = data[0].children[0]
-                            continue
-                        }
-                        if (data[currentId].name[0].toLowerCase() === event.key.toLowerCase()) {
-                            dispatch({
-                                type: treeTypes.focus,
-                                id: currentId,
-                                lastInteractedWith: id,
-                            })
-                            return
-                        }
-                        currentId = getNextAccessible(data, currentId, expandedIds)
-                    }
-                }


a-z, A-Z keys handlers interfere with keyboard shortcuts introduced by this PR so we disable the former.

Can we do this without patching the third-party library?

Seems like there's no option for that though. Let's add a comment then and explain the change.

philipp-spiess · 2023-03-03T12:19:35Z

Can we make these icons a bit (much, lol) more subtle? They're screaming for attention now 😬

Maybe ask in #ask-design for some ideas and rm for now?

taras-yemets · 2023-03-03T12:31:33Z

Converting back to the draft state to address implementation (https://github.com/sourcegraph/sourcegraph/pull/46829#discussion_r1124379369) and design (https://github.com/sourcegraph/sourcegraph/pull/46829#issuecomment-1453451232) issues.

philipp-spiess

Couple of comments but it's all gated anyway so, sure. I’m sad that we have to let the a-zA-Z listeners in the treeview go for it though 😭

Adds shortcuts labels when the feature is enabled:

btw it looks like you forgot to commit the code for this? I don't see the change anywhere haha

philipp-spiess · 2023-03-03T12:21:09Z

+    focusCodeEditor: {
+        title: 'Focus editor',
+        keybindings: [{ ordered: ['c'] }],
+        hideInHelp: true,


Why hide this in the help menu?

Not to show them when the feature flag is disabled.
Though we can filter them out based on the feature flag value.

Not to show them when the feature flag is disabled.

Ah, of course 😅

philipp-spiess · 2023-03-03T12:21:51Z

    },
+    focusCodeEditor: {
+        title: 'Focus editor',
+        keybindings: [{ ordered: ['c'] }],


All other file handlers in this file have a modifier to go first, are we certain we want to break this pattern here? Hm.

These shortcuts are from RFC 345.
I have a few doubts regarding it:

both JetBrains and VSCode IDEs use shortcuts with modifiers (Cmd+1 and Shift+Cmd+E respectively)

I'll do an extra round of testing to ensure c, f, s shortcuts to not hijack commonly used browser shortcuts (copy, interacting with panels, etc.)

@taras-yemets What, for example, happens when you press c, f, or s while inside a text input?

Maybe we should discuss this with other folks as well? 🤔 summoning @sourcegraph/code-exploration-devs

What, for example, happens when you press c, f, or s while inside a text input?

Nothing. Input behaves as usual.
It can be another argument favoring adding modifiers to focus files/symbols/code shortcuts so that we can switch from input to any of mentioned areas.

philipp-spiess · 2023-03-03T12:24:27Z

+            )}
+
+            {enableBlobPageSwitchAreasShortcuts &&
+                [focusFileTreeShortcut, focusSymbolsShortcut].map((shortcut, tabIndex) =>


very minor nit feel free to ignore but I think it'd be easier to read this without combining it into an array and then mapping over it and instead rendering two components. 😅

philipp-spiess · 2023-03-03T12:26:14Z

+    const [enableBlobPageSwitchAreasShortcuts] = useFeatureFlag('blob-page-switch-areas-shortcuts')
+    const focusFileTreeShortcut = useKeyboardShortcut('focusFileTree')
+    const focusSymbolsShortcut = useKeyboardShortcut('focusSymbols')
+    const [focusKey, setFocusKey] = useState('')


Hmmm I wonder if there are any issues here with reusing the same key across both components in combination with keeping the components mounted (so when we focus one, both are technically getting focused). Can you try to apply this line and see if it still works?

https://github.com/sourcegraph/sourcegraph/pull/48610/files#diff-b71212ff6b850bb481f9a4742d142fcf94b40a544f771c837aa10e24163d7d5fR115

Seems to work fine with <Tabs {...props} behavior="memoize" />

Screen.Recording.2023-03-03.at.15.21.59.mov

But you're right, focus() on the FocusableTree ref is called twice (once per tab). Looking into it 👀

Addressed in https://github.com/sourcegraph/sourcegraph/pull/46829/commits/0f321dc0ab9498cd58d8dc4c18a86fe56de718c8.
Thanks for pointing this out!

philipp-spiess · 2023-03-03T12:27:52Z

+}
+
+/**
+ * Renders {@link Tree} and focuses it when {@link focusKey} changes.


Are the @link like this even working? In JS, the Tree module is not global so i figured this is not really helpful anyway and will just render incorrectly, hence I've never bothered to use it. Happy to be proven wrong though :D

It works for me in JetBrans IDE 🤷🏻

Screen.Recording.2023-03-03.at.14.48.08.mov

Ah because it uses the Tree that is available within the current module I guess? 😱 And what about focusKey, no way it finds that?

And what about focusKey, no way it finds that?

See sec 6 on the video. It takes it from the props interface.

Awesome, thanks for showing.

philipp-spiess · 2023-03-03T12:29:19Z

+export function FocusableTree<N extends TreeNode>(props: FocusableTreeProps & TreeProps<N>): JSX.Element {
+    const { focusKey, ...rest } = props
+
+    const ref = useRef<HTMLUListElement>(null)


Suggested change

const ref = useRef<HTMLUListElement>(null)

const ref = useRef<TreeRef>(null)

How did this not fail? 😱

🤷🏻 maybe because HTMLUListElement matches TreeRef interface (has focus method)?

I mean it has focus but it also has some properties that only DOM nodes have as well so this should really not work. Maybe the typescript workaround for generic refs doesn't work? :(

Maybe the typescript workaround for generic refs doesn't work?

It seems to work

But yes, we can be more explicit and define ref type as TreeRef 👍🏻

https://github.com/sourcegraph/sourcegraph/pull/46829/commits/7851d3d7e82c5d373dad931a9f1da1acad7910d2

I’m still confused here. Does this mean useImperativeHandle merges the ref with the object you define? e.g. what happens if you do ref.current.innerHTML = 'foobar'? This would be valid on a UList but not on the TreeRef 😅

Also the error message is confusing because we don't want to assign HTMLUListElement ref into TreeRef but instead it should be assigned the other way around haha. Seems like this is a covariant/contravariant issue with your workaround because it looses the nuance that this is one over the other (I don't know exactly which term is the correct one here lol)

microsoft/TypeScript#30748 (comment)

Ah the sound type system strikes again 🙃

what happens if you do ref.current.innerHTML = 'foobar'? This would be valid on a UList but not on the TreeRef

You're right! With TreeRef as a type we get an expected error

Seems like this is a covariant/contravariant issue with your workaround

Maybe 🤷🏻
Do you have ideas on how we can define safer types on the Tree component with generic type and forwardRef combo?

Do you have ideas on how we can define safer types on the Tree component with generic type and forwardRef combo?

no, I did a quick google session but it's not as simple as in Flowtype to define a property as contravariant so I'd say we ignore it for now 😓

taras-yemets · 2023-03-03T12:51:59Z

btw it looks like you forgot to commit the code for this? I don't see the change anywhere haha

@philipp-spiess, exactly! https://github.com/sourcegraph/sourcegraph/pull/46829/commits/6e2dae2e9720f40ceb867086958d19453892d4d9

This reverts commit 6e2dae2.

taras-yemets · 2023-03-03T13:07:08Z

Can we make these icons a bit (much, lol) more subtle? They're screaming for attention now 😬

Maybe ask in #ask-design for some ideas and rm for now?

Agree. Removing them for now. https://github.com/sourcegraph/sourcegraph/pull/46829/commits/fa4a6c44aecac509b9fa75cd8a5c055c4fd84774

sourcegraph-bot · 2023-03-07T13:45:29Z

Codenotify: Notifying subscribers in OWNERS files for diff fa271fb...d56b5b8.

Notify	File(s)
@sourcegraph/code-exploration-devs	client/wildcard/src/components/Tree/Tree.tsx client/wildcard/src/components/Tree/react-accessible-treeview/index.tsx client/wildcard/src/components/index.ts
@vovakulikov	client/wildcard/src/components/Tree/Tree.tsx client/wildcard/src/components/Tree/react-accessible-treeview/index.tsx client/wildcard/src/components/index.ts

initial commit

0c75def

cla-bot Bot added the cla-signed label Jan 24, 2023

taras-yemets added 3 commits February 16, 2023 08:52

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

a669491

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

966e4b6

focus code editor

01f598b

taras-yemets added 9 commits February 28, 2023 14:21

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

18ead3b

ugly and buggy

8ad316b

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

89c843e

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

f0307da

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

2c30d9a

wip

3fc0fc0

wip

af1ec4c

wip

b4fa6d1

wip

0447d96

taras-yemets commented Mar 2, 2023

View reviewed changes

taras-yemets added 4 commits March 3, 2023 10:34

fix lint errors

4d15f07

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

cb9c6aa

put behind the feature flag

617a723

hide shortcuts in help

15933ad

taras-yemets marked this pull request as ready for review March 3, 2023 11:49

taras-yemets requested a review from a team March 3, 2023 11:49

philipp-spiess approved these changes Mar 3, 2023

View reviewed changes

add shortcuts labels

6e2dae2

taras-yemets marked this pull request as draft March 3, 2023 12:57

Revert "add shortcuts labels"

fa4a6c4

This reverts commit 6e2dae2.

taras-yemets added 8 commits March 3, 2023 15:14

explicitly specify ref type

7851d3d

use separate focus key per tab

0f321dc

check if list is already focused

6aa7b49

allow default actions for shortcuts

96b811a

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

1070062

merge leftovers

7aba910

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

1dc2714

merge leftovers

5565704

taras-yemets marked this pull request as ready for review March 7, 2023 13:43

taras-yemets added the needs-design Design requests - add to 'design priorities' project, add a deadline, if possible.. label Mar 7, 2023

taras-yemets added 5 commits March 7, 2023 17:23

update changelog

fbab161

Merge branch 'main' into taras-yemets/file-page-keyboard-nav

031a960

add shortcut labels

d56b5b8

fix lint errors

c85bd82

fix imports order

d6810db

taras-yemets merged commit fa95c86 into main Mar 8, 2023

taras-yemets deleted the taras-yemets/file-page-keyboard-nav branch March 8, 2023 10:42

This was referenced Mar 8, 2023

blob: remove shortcut labels #48952

Merged

blob: show experimental keyboard shortcuts in help #48955

Merged

Ignore key presses when modifier is held but shortcut has no modifier defined #49055

Merged

	const ref = useRef<HTMLUListElement>(null)
	const ref = useRef<TreeRef>(null)

Conversation

taras-yemets commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

App preview:

Uh oh!

sg-e2e-regression-test-bob commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipp-spiess commented Mar 3, 2023

Uh oh!

taras-yemets commented Mar 3, 2023

Uh oh!

philipp-spiess left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipp-spiess Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taras-yemets commented Mar 3, 2023

Uh oh!

taras-yemets commented Mar 3, 2023

Uh oh!

sourcegraph-bot commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taras-yemets commented Jan 24, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Feb 17, 2023 •

edited

Loading

philipp-spiess Mar 3, 2023 •

edited

Loading

sourcegraph-bot commented Mar 7, 2023 •

edited

Loading