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 alert so braille users can detect events #201328

Merged
merged 7 commits into from Jan 3, 2024
Merged

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Dec 20, 2023

fixes #195282

Braille users don't benefit from audio cues, so we should offer alert as a customizable / additional alternative. I have heard from users that some might use both cues and the alerts.

@meganrogge meganrogge marked this pull request as draft December 20, 2023 19:30
@meganrogge meganrogge self-assigned this Dec 20, 2023
@meganrogge meganrogge added this to the December / January 2024 milestone Dec 20, 2023
@meganrogge meganrogge marked this pull request as ready for review January 3, 2024 15:44
@meganrogge meganrogge changed the title add configurable alert for line changes add alert so braille users can detect events Jan 3, 2024
@meganrogge meganrogge merged commit d8a75ea into main Jan 3, 2024
6 checks passed
@meganrogge meganrogge deleted the merogge/alert-cue branch January 3, 2024 22:42
@@ -55,11 +55,26 @@ export const IAccessibleNotificationService = createDecorator<IAccessibleNotific
*/
export interface IAccessibleNotificationService {
readonly _serviceBrand: undefined;
notify(event: AccessibleNotificationEvent, userGesture?: boolean): void;
notify(event: AccessibleNotificationEvent, userGesture?: boolean, forceSound?: boolean, allowManyInParallel?: boolean): void;
notifyLineChanges(event: AccessibleNotificationEvent[]): void;
Copy link
Member

@hediet hediet Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this LineChanges?
I don't understand what notifyLineChanges([AccessibleNotificationEvent.Save]) would mean. Maybe that needs to be a different enum?

@@ -210,7 +213,7 @@ class MarkerLineFeature implements LineFeature {

class FoldedAreaLineFeature implements LineFeature {
public readonly audioCue = AudioCue.foldedArea;

public readonly event = AccessibleNotificationEvent.Folded;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fishy that every LineFeature now has an event and audioCue and that the audioCue can be derived from the event.

@@ -41,7 +42,8 @@ export class AudioCueLineFeatureContribution
@IEditorService private readonly editorService: IEditorService,
@IInstantiationService private readonly instantiationService: IInstantiationService,
@IAudioCueService private readonly audioCueService: IAudioCueService,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IAudioCueService private readonly audioCueService: IAudioCueService,

It is very fishy that this contribution now uses both the audioCueService and the accessibleNotificationService.

And indeed this causes bugs, as the line-feature enablement state is taken from the audio cue service. So if some audio cues are disabled, the corresponding accessibleNotificationEvent will not be sent either.

@hediet
Copy link
Member

hediet commented Jan 4, 2024

Except the mentioned bug (that line-based features don't send accessible notification events when the corresponding audio cue is disabled), I can see this implementation working.

However, from a software-design perspective, I think this feature should be implemented in a different way.
I don't think it makes sense to have accessible notification events and audio cues as separate systems.
This can quickly cause inconsistencies (which audio cue is tied to which accessible notification?) and is a source for bugs (like when the audio cue is used to check if something is enabled, but the accessible notification event is used to play the sound/cause the aria alert).

Rather, I'd rethink audio cues (which are already an abstract thing, decoupled from the actual sounds). I see two options:

  1. Either I would extend the audio cue service to optionally do the aria alert (and extend the audio cue instances with the text to read).
  2. Or I would replace audio cues with "accessible notification events" or "signals" entirely.
    Then some signal service takes care of playing the configured sounds or triggering the aria alert and computing if that signal even has any listeners.

Also, as a thought I just had, with the line features, maybe it even makes sense to distinguish event-based signals/audio-cues (such as the save-signal) from state-based signals/audio-cues (such as the current line has errors/warnings/breakpoints etc).

I'd start with 1. and would think about 2. as a separate refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider alert as option instead of audio cue for components
3 participants