Generalize Ctrl+DownArrow and Ctrl+UpArrow to most input-result widgets (Fix #179967) #187077

hsfzxjy · 2023-07-05T18:44:09Z

This adds Ctrl+UpArrow and Ctrl+DownArrow navigation to

ExtensionsViewPaneContainer
KeybindingsEditor
CommentsPanel
MarkersView
Repl

The two keybindings are uniformly controlled by two new commands inputResults.focusResults and inputResults.focusInput.

meganrogge · 2023-07-06T18:52:10Z

Hey @hsfzxjy thanks so much for working on this. Something to consider - not sure if you have already thought of this - is are there pre-existing applications of this keybinding for each of the contexts?

IE in these views, does ctrl/cmd + up/downArrow have a function already that we will be overriding. I haven't gone through these yet individually, but that's what we'll need to do before proceeding with this so as to not break muscle memory.

for example, I adopted such a change in the search view - but did not realize that I broke muscle memory a bit since ctrl/cmd+downArrow when in the search in put box with multiline text should jump to the bottom of that, but currently instead jumps to the next input box.

hsfzxjy · 2023-07-06T19:24:59Z

@meganrogge Yes, I've investigated on this and ensure not breaking muscle memory.

The hardest part is that those Lists/Tables/Trees in the "results panel" by default use Ctrl+Up for scrolling up, which could conflict with the newly added action of focusing "input widget". I harmonize these two into:

If the List/Table/Tree is not scrolled at top most, Ctrl+Up will scroll it up as in the old version.
Otherwise, Ctrl+Up will focus corresponding "input widget", which is a new action compared with the old version.

Here's a list of behavior that this PR affects:

Extensions
Search box
Before: Ctrl+Down will focus extension list;
After: As-is.

Extension List
Before: Ctrl+Up will scroll up the extension list. If at top most position, nothing happens;
After: Ctrl+Up will still scroll up the extension list. If at top most position, focus the search box.

Keybinding Editor
Input box
Before: Ctrl+Down will focus keybinding table;
After: As-is.

Keybinding Table
Before: Ctrl+Up will scroll up the keybinding table. If at top most position, nothing happens;
After: Ctrl+Up will still scroll up the keybinding table. If at top most position, focus the input box.

MarkerView
Filter Box
Before: Ctrl+Down will focus view body;
After: As-is.

View Body
Before: Ctrl+Up will scroll up the view body. If at top most position, nothing happens;
After: Ctrl+Up will still scroll up the view body. If at top most position, focus the filter box.

CommentsView
Filter Box
Before: Ctrl+Down will focus the tree;
After: As-is.

Tree
Before: Ctrl+Up will scroll up the tree. If at top most position, nothing happens;
After: Ctrl+Up will still scroll up the tree. If at top most position, focus the filter box.

CommentsView
Filter Box
Before: nothing;
After: Ctrl+Down will focus the REPL Input.

REPL Input
Before: nothing;
After: Ctrl+Up will focus the filter box.

meganrogge · 2023-07-07T03:01:23Z

Amazing 🙌 thank you. I'll review this next week.

sandy081 · 2023-07-12T07:46:43Z

@meganrogge I see you are reviewing this PR. Is there anything you would need from me?

meganrogge · 2023-07-12T12:15:50Z

@sandy081 a look at your particular area(s) would be good. Thanks

alexr00

commentsView.ts and commentsView.test.ts look good!

meganrogge · 2023-07-12T15:02:44Z

@roblourens could you pls make sure it looks good for your area?

src/vs/platform/inputResults/browser/inputResultsService.ts

src/vs/workbench/browser/parts/views/viewPane.ts

src/vs/workbench/browser/actions/inputResultsCommands.ts

src/vs/platform/list/browser/listService.ts

src/vs/editor/standalone/browser/standaloneServices.ts

joaomoreno · 2023-07-14T10:57:10Z

src/vs/platform/list/browser/listService.ts

+function createScrollAtTopObserver(contextKeyService: IContextKeyService, widget: WorkbenchListWidget): IDisposable {
+	const listScrollAtTop = RawWorkbenchListScrollAtTopContextKey.bindTo(contextKeyService);
+	listScrollAtTop.set(widget.scrollTop === 0);
+	return widget.onDidScroll((e) => {
+		const shouldUpdate = (e.oldScrollTop === 0) !== (e.scrollTop === 0);
+		if (shouldUpdate) {
+			listScrollAtTop.set(e.scrollTop === 0);
+		}
+	});
+}


AFAIK there is no need to check for should update since that's taken care of further down the line.

Suggested change

function createScrollAtTopObserver(contextKeyService: IContextKeyService, widget: WorkbenchListWidget): IDisposable {

const listScrollAtTop = RawWorkbenchListScrollAtTopContextKey.bindTo(contextKeyService);

listScrollAtTop.set(widget.scrollTop === 0);

return widget.onDidScroll((e) => {

const shouldUpdate = (e.oldScrollTop === 0) !== (e.scrollTop === 0);

if (shouldUpdate) {

listScrollAtTop.set(e.scrollTop === 0);

}

});

}

function createScrollAtTopObserver(contextKeyService: IContextKeyService, widget: WorkbenchListWidget): IDisposable {

const listScrollAtTop = RawWorkbenchListScrollAtTopContextKey.bindTo(contextKeyService);

const update = () => listScrollAtTop.set(widget.scrollTop === 0);

update();

return widget.onDidScroll(update);

}

src/vs/workbench/browser/actions/listCommands.ts

joaomoreno · 2023-07-14T11:02:57Z

src/vs/workbench/browser/parts/views/viewFilter.ts

+	private readonly _onDidBlur = this._register(new EventMultiplexer<void>());
+	readonly onDidBlur = this._onDidBlur.event;
+	private readonly _onDidFocus = this._register(new EventMultiplexer<void>());
+	readonly onDidFocus = this._onDidFocus.event;
+


Instead of allocating additional objects, these two events should just be the same instances as the focusTracker's events. One suggestion: createInput could return the focusTracker instance along with the ContextScopedHistoryInputBox instance, then the events could simply be assigned in the constructor.

joaomoreno · 2023-07-14T11:04:19Z

src/vs/workbench/contrib/comments/browser/commentsView.ts

+		this._register(this._onDidFocusResults.add(focusTracker.onDidFocus));
+		this._register(this._onDidBlurResults.add(focusTracker.onDidBlur));


This one is trickier to untangle, but should still be possible to do without any additional allocations. Let me know if you need pointers for that.

joaomoreno · 2023-07-14T11:04:43Z

src/vs/workbench/contrib/debug/browser/repl.ts

+		this._register(this._onDidFocusResults.add(this.replInput.onDidFocusEditorWidget));
+		this._register(this._onDidBlurResults.add(this.replInput.onDidBlurEditorWidget));


Same allocation comment as above.

joaomoreno · 2023-07-14T11:08:21Z

src/vs/platform/inputResults/browser/inputResultsService.ts

+	register(view: IInputResultsView): IDisposable;
+}
+
+export class InputResultsService implements IInputResultsService {


This is such a strange thing to become a service. First, I don't think we've ever defined such a concept as an input-result widget. But let's say we do... still, we don't have a class to encapsulate that widget's definition and behavior. We seem to be creating an abstract concept of a widget without creating the widget itself. 🤔

Actually I made it as a service so that we can define a unified command to make the navigation keybinding consistent across several widgets. Do you find it necessary to define unified commands for these navigation? If so, what's the better way for this?

Also after reading other comments you left, I agree that the term input-results is poorly defined and too constrained. For example in repl, it's hard to say which one of the filter box and the input box is the "input". Thus I feel like to rewrite this PR and extend it to more general cases -- use Ctrl+Up/Down to navigate among widgets in a vertical-layout container like the search panel did. This should come with the following advantages:

No more confusion of "input" vs "results"

Can be applied to container with more than two child widgets, instead of simple "input" and "results".

I am wondering what's your attitude to this proposal? Many thanks to your kind instructions.

Do you find it necessary to define unified commands for these navigation? If so, what's the better way for this?

We have what's called a MultiCommand - for example

vscode/src/vs/workbench/contrib/accessibility/browser/accessibilityContribution.ts

Lines 71 to 88 in c780d0d

export const AccessibilityHelpAction = registerCommand(new MultiCommand({

id: 'editor.action.accessibilityHelp',

precondition: undefined,

kbOpts: {

primary: KeyMod.Alt | KeyCode.F1,

weight: KeybindingWeight.WorkbenchContrib,

linux: {

primary: KeyMod.Alt | KeyMod.Shift | KeyCode.F1,

secondary: [KeyMod.Alt | KeyCode.F1]

}

},

menuOpts: [{

menuId: MenuId.CommandPalette,

group: '',

title: localize('editor.action.accessibilityHelp', "Open Accessibility Help"),

order: 1

}],

}));

Your approach above sounds good to me, though I believe would still use the service concept?

We have what's called a MultiCommand

Thanks for pointing out, never know this before.

I am actually rewriting this PR to realize general widget navigation in a container using Ctrl+Up/Down. With MultiCommand I think the service concept can be eliminated. But the time complexity of executing action may increase since there could be many containers attached to the action. Is it acceptable?

Yes, that sounds good. Thanks so much for working on this

@meganrogge I have another idea to eliminate service, which I'm not sure is appropriate or not. I've put the pseudo code at https://gist.github.com/hsfzxjy/5beae21297024f443c2d69faffc4cfe5 . Could you check it and see whether this pattern is OK or why it's not acceptable?

joaomoreno · 2023-07-14T11:10:43Z

src/vs/workbench/contrib/markers/browser/markersView.ts

+	private readonly _onDidBlurResults = this._register(new EventMultiplexer<void>());
+	private readonly _onDidFocusResults = this._register(new EventMultiplexer<void>());
+


Same allocation comment.

hsfzxjy · 2023-07-15T13:57:28Z

I've completed a full rewrite on this PR in respond to @roblourens 's constructive suggestions. The changes at present are:

Disable command list.scrollUp when scrollbar is at top-most position. Also, disable command list.scrollDown when scrollbar is at bottom-most position.
Provide registerNavigatableContainer() function to enable Ctrl+Up/Down navigation among widgets in a container. Two new commands widgetNavigation.focusNext and widgetNaviagation.focusPrevious are added.
Enable such widget navigation under more cases, including ExtensionsViewContainer, KeybindingsEditor, Repl, CommentsPanel, MarkersView and TestingExplorerView.

@meganrogge I think we should start a new review? Sorry about the occupation of your precious time. Thanks! 🙏🏼

meganrogge · 2023-07-17T15:19:07Z

@hsfzxjy thank you so much for your continued work on this. based on my testing, things are working very well 🎉

I'm seeing this error though:

Error: vs/platform/list/browser/listService.ts(296,69): error TS2345: Argument of type 'this' is not assignable to parameter of type '{ readonly onDidScroll: Event<any>; readonly scrollTop: number; readonly scrollHeight: number; readonly renderHeight: number; }'.
  Type 'WorkbenchList<T>' is missing the following properties from type '{ readonly onDidScroll: Event<any>; readonly scrollTop: number; readonly scrollHeight: number; readonly renderHeight: number; }': onDidScroll, renderHeight

EDIT:
I am not seeing that error when I compile locally

addressed

hsfzxjy · 2023-07-17T15:30:10Z

@meganrogge Saw this. I am troubleshooting it.

hsfzxjy · 2023-07-17T17:49:01Z

The error is gone now. It was caused by an overly aggressive tree-shaking.

…t boundary

meganrogge · 2023-07-18T12:43:59Z

@joaomoreno if you want to give this another pass, I plan to test and approve today if all works well

meganrogge

👏🏼

hsfzxjy force-pushed the ctrl-arrow branch from 66f5b6e to 64b3cb8 Compare July 5, 2023 18:45

hsfzxjy changed the title ~~Generalize Ctrl+DownArrow and Ctrl+UpArrow to all input-result widgets (Fix #179967)~~ Generalize Ctrl+DownArrow and Ctrl+UpArrow to most input-result widgets (Fix #179967) Jul 5, 2023

vscodenpa assigned meganrogge Jul 5, 2023

meganrogge added this to the July 2023 milestone Jul 5, 2023

hsfzxjy force-pushed the ctrl-arrow branch 4 times, most recently from b68e8ca to 5d64ccf Compare July 6, 2023 18:30

hsfzxjy force-pushed the ctrl-arrow branch 5 times, most recently from 5664660 to 79caf94 Compare July 10, 2023 13:34

meganrogge requested review from alexr00, roblourens and sandy081 July 10, 2023 14:42

hsfzxjy force-pushed the ctrl-arrow branch from 79caf94 to dbea7be Compare July 12, 2023 05:57

hsfzxjy force-pushed the ctrl-arrow branch 2 times, most recently from 679b20b to 7f73b97 Compare July 12, 2023 11:59

alexr00 previously approved these changes Jul 12, 2023

View reviewed changes

roblourens reviewed Jul 12, 2023

View reviewed changes

meganrogge requested a review from joaomoreno July 12, 2023 19:02

hsfzxjy dismissed alexr00’s stale review via a24cc1b July 13, 2023 03:04

hsfzxjy requested a review from roblourens July 13, 2023 03:17

joaomoreno previously requested changes Jul 14, 2023

View reviewed changes

hsfzxjy dismissed roblourens’s stale review via b578352 July 15, 2023 13:47

hsfzxjy force-pushed the ctrl-arrow branch from a24cc1b to b578352 Compare July 15, 2023 13:47

hsfzxjy force-pushed the ctrl-arrow branch from 9e55715 to 61575de Compare July 17, 2023 15:18

hsfzxjy force-pushed the ctrl-arrow branch from 61575de to eb983ac Compare July 17, 2023 16:55

hsfzxjy added 9 commits July 18, 2023 14:03

Disable command list.scrollUp/scrollDown when the list scrollbar is a…

f215fbe

…t boundary

Add commands and registry for widgetNavigation

5e9ea7e

Make ExtensionsViewContainer widget navigatable

872a821

Make KeybindingsEditor widget navigatable

71cab13

Expose onDidFocus, onDidBlur, hasFocus on FilterWidget

070fa59

Make Repl widget navigatable

aea7c5b

Make CommentsPanel widget navigatable

c3a8226

Make MarkersView widget navigatable

6ed1671

Make TestingExplorerView widget navigatable

927510d

hsfzxjy force-pushed the ctrl-arrow branch from eb983ac to 927510d Compare July 18, 2023 06:03

sandy081 approved these changes Jul 18, 2023

View reviewed changes

meganrogge approved these changes Jul 18, 2023

View reviewed changes

meganrogge enabled auto-merge July 18, 2023 15:33

meganrogge merged commit a9fcc43 into microsoft:main Jul 18, 2023

meganrogge mentioned this pull request Jul 18, 2023

correct word #188170

Merged

hsfzxjy deleted the ctrl-arrow branch July 19, 2023 06:09

meganrogge mentioned this pull request Jul 19, 2023

keybinding conflict in inline chat #188317

Closed

hsfzxjy mentioned this pull request Aug 8, 2023

Should it be possible to use ctrl+up/ctrl+down to navigate within a comment thread #188810

Closed

github-actions bot locked and limited conversation to collaborators Sep 1, 2023

		this._register(this._onDidFocusResults.add(focusTracker.onDidFocus));
		this._register(this._onDidBlurResults.add(focusTracker.onDidBlur));

	export const AccessibilityHelpAction = registerCommand(new MultiCommand({
	id: 'editor.action.accessibilityHelp',
	precondition: undefined,
	kbOpts: {
	primary: KeyMod.Alt \| KeyCode.F1,
	weight: KeybindingWeight.WorkbenchContrib,
	linux: {
	primary: KeyMod.Alt \| KeyMod.Shift \| KeyCode.F1,
	secondary: [KeyMod.Alt \| KeyCode.F1]
	}
	},
	menuOpts: [{
	menuId: MenuId.CommandPalette,
	group: '',
	title: localize('editor.action.accessibilityHelp', "Open Accessibility Help"),
	order: 1
	}],
	}));

		private readonly _onDidBlurResults = this._register(new EventMultiplexer<void>());
		private readonly _onDidFocusResults = this._register(new EventMultiplexer<void>());

Generalize Ctrl+DownArrow and Ctrl+UpArrow to most input-result widgets (Fix #179967) #187077

Generalize Ctrl+DownArrow and Ctrl+UpArrow to most input-result widgets (Fix #179967) #187077

Uh oh!

Conversation

hsfzxjy commented Jul 5, 2023

Uh oh!

meganrogge commented Jul 6, 2023

Uh oh!

hsfzxjy commented Jul 6, 2023

Uh oh!

meganrogge commented Jul 7, 2023

Uh oh!

sandy081 commented Jul 12, 2023

Uh oh!

meganrogge commented Jul 12, 2023

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

meganrogge commented Jul 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

hsfzxjy Jul 14, 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!

meganrogge Jul 14, 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!

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!

hsfzxjy commented Jul 15, 2023

Uh oh!

meganrogge commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsfzxjy commented Jul 17, 2023

Uh oh!

hsfzxjy commented Jul 17, 2023

Uh oh!

meganrogge commented Jul 18, 2023

Uh oh!

meganrogge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hsfzxjy Jul 14, 2023 •

edited

Loading

meganrogge Jul 14, 2023 •

edited

Loading

meganrogge commented Jul 17, 2023 •

edited

Loading