Down arrow moves focus to extensions list by JacksonKearl · Pull Request #53616 · microsoft/vscode

JacksonKearl · 2018-07-05T21:05:21Z

JacksonKearl · 2018-07-06T00:34:13Z

@joaomoreno you own the list view right? Should this be a part of all lists or keep it to extensions for now?

joaomoreno · 2018-07-06T06:32:00Z

src/vs/base/browser/ui/list/listWidget.ts

 		this.onDidFocus = mapEvent(domEvent(this.view.domNode, 'focus', true), () => null);
 		this.onDidBlur = mapEvent(domEvent(this.view.domNode, 'blur', true), () => null);
-
+		this.onDidFocus(() => this.focusFirstItemIfNothingFocused(), this, this.disposables);


I really don't think this is the best behaviour for the list. The default behaviour of DOM focus should be just that, not focusing an element.

src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts

+
+	focus(): void {
+		super.focus();
+		this.list.domFocus();


This reverts commit 2bdfeef.

JacksonKearl · 2018-07-10T17:33:26Z

@joaomoreno I'd prefer to keep the implementation for List's as is, as we tend to visibly hilight the focused element even when the parent List is not focused, which begins to be noisy. What do you think?

joaomoreno · 2018-07-11T09:19:15Z

Oh... that seems inconsistent. IMO a focused element in an inactive tree/list should not have a background... But now we have this: #46129 ... it's a pickle.

I propose to set list.inactiveFocusForeground to transparent on our base themes. Other themes may set that color.

I would not keep this implementation simply because it is an accessibility issue that a widget's state changes when it gets DOM focus. The state (that the first element is focused) should have been there all along.

JacksonKearl · 2018-07-12T19:00:40Z

@joaomoreno I changed all the lists from an onFocus implementation to an onContentChange one, but I'm not really happy about the change, as I'm finding it difficult to do this in a way that generalizes at all. Given there is no onContentChange type property of the List that I know of, each different instance requires a bespoke solution, relying on underlying features of each particular model (some models have onChange style events, others (such as IPagedModel) do not).

Also, it will be impossible to generalize this to extension provided list viewlets, as it requires knowledge of the model being used. Do you still think this is the best way forward? And is there a generalized content change event somewhere on either lists or trees that I'm not seeing?

JacksonKearl · 2018-07-13T22:25:31Z

This now only addresses the original issue of navigating to the extensions list view on down arrow, in response to #53755 (comment)

JacksonKearl · 2018-07-16T16:20:12Z

@ramya-rao-a this is entirely confined to the extensions viewlet now, if you want to take a look. We can update everything later, pending a decision on #53755 (comment)

ramya-rao-a · 2018-07-18T22:05:17Z

src/vs/workbench/parts/extensions/electron-browser/extensionsViews.ts

 			this.list.model = model;
 			this.list.scrollTop = 0;
 			const count = this.count();
+			this.list.focusNext();


this makes more sense inside the new focus

ramya-rao-a · 2018-07-18T22:05:28Z

src/vs/platform/theme/common/colorRegistry.ts

 export const listInactiveSelectionBackground = registerColor('list.inactiveSelectionBackground', { dark: '#3F3F46', light: '#CCCEDB', hc: null }, nls.localize('listInactiveSelectionBackground', "List/Tree background color for the selected item when the list/tree is inactive. An active list/tree has keyboard focus, an inactive does not."));
 export const listInactiveSelectionForeground = registerColor('list.inactiveSelectionForeground', { dark: null, light: null, hc: null }, nls.localize('listInactiveSelectionForeground', "List/Tree foreground color for the selected item when the list/tree is inactive. An active list/tree has keyboard focus, an inactive does not."));
-export const listInactiveFocusBackground = registerColor('list.inactiveFocusBackground', { dark: '#313135', light: '#d8dae6', hc: null }, nls.localize('listInactiveSelectionBackground', "List/Tree background color for the selected item when the list/tree is inactive. An active list/tree has keyboard focus, an inactive does not."));
+export const listInactiveFocusBackground = registerColor('list.inactiveFocusBackground', { dark: null, light: null, hc: null }, nls.localize('listInactiveFocusBackground', "List/Tree background color for the focused item when the list/tree is inactive. An active list/tree has keyboard focus, an inactive does not."));


not needed anymore?

Outdated

Down arrow focuses list view.

2949f82

JacksonKearl requested a review from ramya-rao-a July 5, 2018 21:05

Jackson Kearl added 2 commits July 5, 2018 14:06

Scoping

7848314

Generalize focusFirstIfNothingFocused logic to all list views

2bdfeef

JacksonKearl mentioned this pull request Jul 5, 2018

Viewlets should focus the first item, not the whole tree #52000

Closed

JacksonKearl self-assigned this Jul 5, 2018

joaomoreno added this to the Backlog milestone Jul 6, 2018

joaomoreno added the list-widget List widget issues label Jul 6, 2018

joaomoreno previously requested changes Jul 6, 2018

View reviewed changes

Jackson Kearl added 3 commits July 6, 2018 11:50

Revert "Generalize focusFirstIfNothingFocused logic to all list views"

40bcd17

This reverts commit 2bdfeef.

Generalize to list in debug view

bdbb9bf

Add to dispasables

1077411

JacksonKearl changed the title ~~Down arrow focuses list view.~~ Tab and down arrow focus first element in list view Jul 6, 2018

Same for scmViewlet

c59e049

Default 'list.inactiveFocusBackground' to null

a3fbacd

JacksonKearl force-pushed the a11y/extensions-down-arrow branch 2 times, most recently from 4f94847 to 80a8e08 Compare July 12, 2018 18:38

Migrate from onDidFocus to onDidChange where possible

654e4e2

JacksonKearl force-pushed the a11y/extensions-down-arrow branch from 80a8e08 to 654e4e2 Compare July 12, 2018 18:44

JacksonKearl mentioned this pull request Jul 12, 2018

Focus first tree view element on entry via tab #53755

Closed

Remove changes for non-extensions

522792f

JacksonKearl changed the title ~~Tab and down arrow focus first element in list view~~ Down arrow moves focus to extensions list Jul 16, 2018

JacksonKearl mentioned this pull request Jul 18, 2018

Autocomplete for extension search @-operators #53915

Merged

ramya-rao-a reviewed Jul 18, 2018

View reviewed changes

Jackson Kearl added 2 commits July 19, 2018 09:23

Undo changed to color registry

1bb0416

Move focus next position

93dc8e6

ramya-rao-a approved these changes Jul 19, 2018

View reviewed changes

ramya-rao-a merged commit 7db5f96 into microsoft:master Jul 19, 2018

ramya-rao-a mentioned this pull request Jul 19, 2018

Accessibility: Extension list items in the installed extensions treeview are in the tab order. #51496

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

JacksonKearl commented Jul 5, 2018

Uh oh!

JacksonKearl commented Jul 6, 2018

Uh oh!

joaomoreno Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

JacksonKearl commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joaomoreno commented Jul 11, 2018

Uh oh!

JacksonKearl commented Jul 12, 2018

Uh oh!

JacksonKearl commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacksonKearl commented Jul 16, 2018

Uh oh!

ramya-rao-a Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JacksonKearl commented Jul 10, 2018 •

edited

Loading

JacksonKearl commented Jul 13, 2018 •

edited

Loading