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

Best way to reference utteranceQueue #7

Closed
jessegreenberg opened this issue Nov 19, 2019 · 7 comments
Closed

Best way to reference utteranceQueue #7

jessegreenberg opened this issue Nov 19, 2019 · 7 comments

Comments

@jessegreenberg
Copy link
Contributor

utteranceQueue was recently added as an instance variable to the Display, as described in #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
Copy link
Member

zepumph commented Nov 19, 2019

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

@jessegreenberg I totally agree! Part of the cleanup steps outlined over in #1 (comment) 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
Copy link
Member

zepumph commented Nov 19, 2019

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
Copy link
Contributor Author

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
Copy link
Member

zepumph commented Nov 19, 2019

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 phetsims/molarity#155.

@jessegreenberg
Copy link
Contributor Author

because aria-live elements are inside a display.

Ah right, thanks for the reminder.

zepumph added a commit to phetsims/reactants-products-and-leftovers that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/balloons-and-static-electricity that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/gravity-force-lab-basics that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/energy-forms-and-changes that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/area-model-introduction that referenced this issue Nov 20, 2019
zepumph added a commit to phetsims/rutherford-scattering that referenced this issue Nov 20, 2019
@zepumph
Copy link
Member

zepumph commented Nov 20, 2019

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
Copy link
Contributor Author

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.

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

No branches or pull requests

3 participants