Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add progress signal for progress service and progress indicator #210516

Closed
wants to merge 2 commits into from

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Apr 16, 2024

note that this is very much a WIP 🚧 and I want some feedback on it early bc I will be OOF starting Friday.

Progress bars around the workbench/editor aren't reported to screen readers ATM despite us doing the right thing to make that happen on the progress bar. #209661 (comment)

Therefore, this PR refactors the chat accessibility signal scheduler that we use for chat response pending into AccessibilityProgressSignalScheduler, which is used by the ProgressService (workbench components) and ProgressIndicator (editors).

I am not sure how necessary this is as most of our progress starts and finishes pretty quickly, meaning this won't typically play with a delay of 1 second and a looping time of 3 seconds. We could of course change those, but I don't want it to be noisy. #209661 (comment)

This does not address the notebook progress signal request because that has its own custom implementation using a progress bar cc @amunger and @rebornix.

I considered implementing this in the progress bar file, so this would also capture notebook progress, but that has layer issues and also is more UI than model.

I would like your feedback @bpasero and @isidorn on this 🙏🏼 and whether or not you think we should have this.

@meganrogge meganrogge marked this pull request as draft April 16, 2024 22:29
@meganrogge meganrogge self-assigned this Apr 16, 2024
@meganrogge meganrogge added this to the April 2024 milestone Apr 16, 2024
@meganrogge meganrogge requested review from bpasero and isidorn April 16, 2024 22:30
@bpasero
Copy link
Member

bpasero commented Apr 17, 2024

Should we work on getting this into the base progress bar if that is the best fix? Yes, there is a layer problem, but we have ways around it. For example, the hover work Ben has been doing also hit layer issues and we pragmatically solved it by letting the workbench component set a helper into base:

export function setHoverDelegateFactory(hoverDelegateProvider: ((placement: 'mouse' | 'element', enableInstantHover: boolean) => IScopedHoverDelegate)): void {
hoverDelegateFactory = hoverDelegateProvider;
}

from

setHoverDelegateFactory((placement, enableInstantHover) => instantiationService.createInstance(WorkbenchHoverDelegate, placement, enableInstantHover, {}));

I also think for alerts, we do something similar for ARIA alerting:

// ARIA
setARIAContainer(this.mainContainer);

So I am not opposed to having another thing we set into the base progress bar to participate in progress globally and report it via alerts. maybe it could be combined with the existing ARIA thing we have.

@meganrogge
Copy link
Contributor Author

meganrogge commented Apr 17, 2024

Sounds good, I'll do something similar. It will still very rarely play AFAICT since we will set the delay to 3 seconds. #209661 (comment)

I have moved part of this to #210582 to make this PR simpler. Edit: going to start from scratch.

@meganrogge meganrogge closed this Apr 17, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 5, 2024
@meganrogge meganrogge deleted the merogge/progress branch December 18, 2024 21:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants