Adding accessibility help for verbose hover

[aiday-mar]

Adding accessibility help for verbose hover

[meganrogge]

Thanks for working on this. I didn't test this, but it LGTM. I suggest adding the +/- actions to the accessible view implementation when they're available.

[aiday-mar]

Hi @meganrogge thank you for the review. I have added the usage of command<keybinding:{0}> in the content of the provider in order to display the keybindings. To do this I changed the type of the view and the provider from view to help.

I was testing the feature and I noticed that the when I would click outside of the accessible view, and make it disappear, the hover would not appear anymore thereafter. I dug around and found that this is because the accessible view does not call the onClose method when it disappears through a blur event. In my case the onClose method does some cleaning and listener disposing which is needed to reset the normal hover functioning. I looked at where onClose is called, and noticed it was called in the hide arrow function of the _render method of the accessibleView.ts file. In particular on line 595, when the editor is blurred, we call this._contextViewService.hideContextView(); but not hide and therefore not onClose.

https://github.com/microsoft/vscode/blob/869828c0978004ea059416b1ff4c596bbe64ddfe/src/vs/workbench/contrib/accessibility/browser/accessibleView.ts#L593C1-L597C1

I changed the code a bit there to also call hide as follows:

https://github.com/microsoft/vscode/pull/212783/files#diff-57bdd64acf33af25dee4c88c656bf7548ab00a0d9eaebaae5e627ddb299a8516R601-R605

I am wondering if there is a specific reason we only call this._contextViewService.hideContextView() on the editor widget blur event and if you think the change of the PR is okay?

[meganrogge]

@aiday-mar I think your change sounds good. Thanks for the info, I will test today to make sure everything still works as expected.

[meganrogge]

@aiday-mar, I just realized that in this PR, you've replaced the HoverAccessibleView with a HoverAccessibilityView. We want to have both.

See what I mean in insider's compared to oss:

1. click on variable in a.ts file
2. cmd+k+i to focus the hover
3. alt+f2 to open the accessible view for it ✅ works in insider's 🐛 doesn't work in OSS

We should also provide a custom help dialog for the hover's accessible view. That can be accomplished using this:

vscode/src/vs/platform/accessibility/browser/accessibleView.ts

Lines 55 to 59 in 1b60028

	/**
	* @returns a string that will be used as the content of the help dialog
	* instead of the one provided by default.
	*/
	customHelp?: () => string;

To see an example:

1. focus the terminal
2. alt+f2
3. alt+f1
4. you'll see the terminal's custom help info there

[meganrogge]

I am working on the fix

[meganrogge]

I noticed that these actions are present and enabled in the toolbar even when the actions can't be taken. Maybe a context key would be a good idea so they only show when actionable?

[meganrogge]

Summary of my changes:

• reverted your commit which changed this to a help dialog & removed the kb service and resolution
• provided hover content in the accessible view (alt+f2) and provided help content for that (alt+f1)
• reapplied your changes which removed the kb service and resolution

[meganrogge]

I think we additionally want an accessibility help dialog when the hover (vs the hover's accessible view) is focused. I will also add that.

[meganrogge]

I have added HoverAccessibilityHelp in addition to the HoverAccessibleView and my testing shows it's all working well now.

1. Focus a hover
2. Hear the hint to open the accessibility help dialog, alt+f1 takes you there
3. Escape that, hover is focused
4. alt+f2 to go to accessible view, hear hint about opening help
5. alt+f1 goes to the help dialog
6. Escape takes you back to the accessible view
7. Escape focuses the hover

[aiday-mar]

Hi @Meggan thanks a lot for looking into the PR and making the changes! I tested the changes with an extension which provides verbose markdown hovers, and have made some polishing changes on top. In particular I made the actions in the accessible view enabled when the actions can be taken.

Screen.Recording.2024-05-24.at.11.35.04.mov

[aiday-mar]

Hi @meganrogge I am thinking of merging this PR this iteration. Would you say this PR is ready for merging?