phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

Best way to reference utteranceQueue #7

Closed jessegreenberg closed 4 years ago

jessegreenberg commented 4 years ago

utteranceQueue was recently added as an instance variable to the Display, as described in https://github.com/phetsims/utterance-queue/issues/1. The downside to this is that every usage requires a reference to the display and so most usages look something like

phet.joist.sim.display.utteranceQueue.addToBack('...');

We should consider a better way to reference this or consider reverting back to a singleton. to be discussed during a11y meeting.

zepumph commented 4 years ago

This felt like it belonged in the utterance-queue repo.

@jessegreenberg I totally agree! Part of the cleanup steps outlined over in https://github.com/phetsims/utterance-queue/issues/1#issuecomment-550148158 was to try to sort this out a bit, but I think this issue is much better suited to do it well.

many usages of phet.joist.sim.display.utteranceQueue could be factored out to: const utteranceQueue = phet.joist.sim.display.utteranceQueue;

What more do you think would be possible? We could have a closure around Sim.js, like one of these:

phet.joist.sim.utteranceQueue phet.joist.sim.getUtteranceQueue()

Then the implementation would be

get utteranceQueue() {
  return this.display.utteranceQueue;
}

I like the idea of factoring out at least some of the logic.

Any other thougths?

zepumph commented 4 years ago

many usages of phet.joist.sim.display.utteranceQueue could be factored out to:

The issue with this is we need to create the variable after the sim has started, so this can never be part of the requirejs load step, like in the // constants section.

jessegreenberg commented 4 years ago

From #1,

some runtimes where more than one display is being created, this implementation breaks everything because utteranceQueue is initialized multiple times (and it is a singleton).

I think ive forgotten what the issue was here, why was the singleton pattern problematic? Could we have a singleton utteranceQueue shared between all displays?

zepumph commented 4 years ago

Could we have a singleton utteranceQueue shared between all displays?

I don't know how that would work, because aria-live elements are inside a display. I didn't see a clear path to having a singleton global that conditionally offered elements to add to the DOM, or perhaps needed to know about elements elsewhere. Even so, we should discuss further in our meeting.

UPDATE: And the aria-live elements must be in the Display because of https://github.com/phetsims/molarity/issues/155.

jessegreenberg commented 4 years ago

because aria-live elements are inside a display.

Ah right, thanks for the reminder.

zepumph commented 4 years ago

After discussing further with @jessegreenberg, we decided to implement the simple getter on Sim.js to remove one section of the access on utteranceQueue. Overall this is only a minor improvement, but the work was also minor.

In the future is we want to change the way utteranceQueue is accessed, then we may elect for a more intense operation.

Today @jessegreenberg and I discussed trying to keep UtteranceQueue a requirejs singleton, and the having each Display create an AriaHerald instance with their own aria-live elements. Then the UtteranceQueue has to "attach" itself to a single AriaHerald's elements to alert, and while attached, no other aria-live alerts are made to other aria-live elements outside of the attached AriaHerald instance. This would allow us to user the same singleton pattern via requirejs for UtteranceQueue, but it also muddied the "hello world" example of how to use utterance queue:


// the way it currently is now
this.utteranceQueue = new UtteranceQueue();
appendElement( this.utteranceQueue.getAriaLiveContainer());
this.utteranceQueue.addToBack( 'hi');

// the way of "attached" AriaHerald instances
const utteranceQueue = require( 'utteranceQueue' );
const ariaHerald = new AriaHerald();
appendElement( ariaHerald.getAriaLiveContainer());
utteranceQueue.addToBack( 'hi') <---- big loud error
utteranceQueue.setActiveHerald( ariaHerald);
utteranceQueue.addToBack( 'my alert');

@jessegreenberg did I miss anything? Anything else for this issue? Please close if not

jessegreenberg commented 4 years ago

Looks great, I think that captures the ideas we were brainstorming. As you said we may come back to this in the future so good to have notes here if we end up returning. Thanks for doing the work to attach to sim. I think this can be closed.