795984 - [AccessFu] Add support to TTS

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

This depends on bug #687879.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Add tts support to AccessFu.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Implement speech output with Web Speech API

Comment 3

[Marco Zehe (:MarcoZ)]

One thought: Can we make the method that plays the sound reusable for other events such as when scrolling, or when something gets activated, and also when certain events occur such as a new main screen having gained focus etc.?

I'm thinking about sound notifications such as seen in iOS.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 781803 [details] [diff] [review]
Implement speech output with Web Speech API

Review of attachment 781803 [details] [diff] [review]:
-----------------------------------------------------------------

Do you think there is a way to separate the "earcon bootstraping part" from the actual speech (internal) part. From what I understand, the bootstraping part might also eventually include adding and setting voices for the speech synthesis which will make the function harder to read.

Also, I am a little worried about the state when the earcons are not yet loaded but this.earconBuffers is already created, and another call to Speech is made. This might result in ignored earcons, as far as I understand.

::: accessible/src/jsat/AccessFu.jsm
@@ +499,5 @@
>  
>    Speech: function Speech(aDetails, aBrowser) {
> +    let window = Utils.win;
> +
> +    let _speechInternal =

Nit: I don't think there's a need to prefix this with _ since it is defined within the Speech function already.

@@ +501,5 @@
> +    let window = Utils.win;
> +
> +    let _speechInternal =
> +      function _speechInternal() {
> +        for each (let action in aDetails.actions) {

for (let action of aDetails.actions)

@@ +503,5 @@
> +    let _speechInternal =
> +      function _speechInternal() {
> +        for each (let action in aDetails.actions) {
> +          Logger.info('tts.' + action.method, '"' + action.data + '"', JSON.stringify(action.options));
> +          if (!action.options.enqueue)

Nit: {}

@@ +504,5 @@
> +      function _speechInternal() {
> +        for each (let action in aDetails.actions) {
> +          Logger.info('tts.' + action.method, '"' + action.data + '"', JSON.stringify(action.options));
> +          if (!action.options.enqueue)
> +            window.speechSynthesis.cancel();

window.speechSynthesis can be undefined. Should "media.webspeech.synth.enabled" be enabled for Speech from AccessFu?

@@ +507,5 @@
> +          if (!action.options.enqueue)
> +            window.speechSynthesis.cancel();
> +
> +          switch (action.method) {
> +          case 'speak':

A little hard to read without the wrapping {}.

Also, indentation of all case blocks.

@@ +513,5 @@
> +              new window.SpeechSynthesisUtterance(action.data));
> +          break;
> +          case 'playEarcon':
> +            {
> +              let audioBuffer = this.earconBuffers[action.data];

this.earconBuffers can be undefined.

@@ +514,5 @@
> +          break;
> +          case 'playEarcon':
> +            {
> +              let audioBuffer = this.earconBuffers[action.data];
> +              if (!audioBuffer)

Nit: {}.

I am guessing that this is the case when the earcon is not yet loaded (or failed)? Does it mean it will just be ignored if not yet ready?

@@ +516,5 @@
> +            {
> +              let audioBuffer = this.earconBuffers[action.data];
> +              if (!audioBuffer)
> +                break;
> +              let node = this.audioContext.createBufferSource();

this.audioContext can be undefined.

@@ +524,5 @@
> +              break;
> +            }
> +          }
> +        }
> +      }.bind(this);

Use a fat arrow to avoid calling bind. Same for all functions with 'bind' below.

@@ +528,5 @@
> +      }.bind(this);
> +
> +    if (!this.earconBuffers && 'AudioContext' in window) {
> +      this.earconBuffers = {};
> +      this.audioContext = new window.AudioContext();

audioContext from window will persist. Even though there's a possibility of Utils.win to be cleared on Accessfu detach.

@@ +530,5 @@
> +    if (!this.earconBuffers && 'AudioContext' in window) {
> +      this.earconBuffers = {};
> +      this.audioContext = new window.AudioContext();
> +
> +      const earcons = ['chrome://global/content/accessibility/tick.wav'];

Perhaps this can be defined outside of this function? Also having earcons as a const does not freeze the object so it is probably not necessary.

@@ +537,5 @@
> +      for (let earcon of earcons) {
> +        let xhr = new window.XMLHttpRequest();
> +        xhr.open('GET', earcon);
> +        xhr.responseType = 'arraybuffer';
> +        xhr.onerror = function (e) {

Nit: function name. Same for all other anonymous functions.

@@ +548,5 @@
> +              function (audioBuffer) {
> +                try {
> +                  let earconName = /.*\/(.*)\..*$/.exec(earcon)[1];
> +                  this.earconBuffers[earconName] = audioBuffer;
> +                  if (--earconsToLoad == 0) {

Nit: ===

You also need to --earconsToLoad on xhr error (and other places that will not let earconsToLoad to ever be 0), otherwise _speechInternal will never be called if there is a problem.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 795984 - Implement speech output with Web Speech API

OK, refactored this to be more readable. Also, hopefully fixed all the concerns you had in the process.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 784679 [details] [diff] [review]
Bug 795984 - Implement speech output with Web Speech API

Review of attachment 784679 [details] [diff] [review]:
-----------------------------------------------------------------

Looks really great and way more readable! r=me with a couple of nits.

::: accessible/src/jsat/AccessFu.jsm
@@ +518,5 @@
> +        let xhr = new window.XMLHttpRequest();
> +        xhr.open('GET', earcon);
> +        xhr.responseType = 'arraybuffer';
> +        xhr.onerror = () => {
> +          Logger.error("Error getting earcon:", xhr.statusText);

Nit: Double to single quotes (same for a case below).

@@ +519,5 @@
> +        xhr.open('GET', earcon);
> +        xhr.responseType = 'arraybuffer';
> +        xhr.onerror = () => {
> +          Logger.error("Error getting earcon:", xhr.statusText);
> +          if (--this.earconsToLoad === 0) {

This block:

if (--this.earconsToLoad === 0) {
  this.onInit();
}

repeats 3(4) times, perhaps makes sense to take it out as a small helper function.

@@ +555,5 @@
> +      this.delayedActions = [];
> +    },
> +
> +    output: function output(aActions) {
> +      if (this.earconsToLoad != 0) {

Nit: !==

@@ +585,5 @@
> +            new window.SpeechSynthesisUtterance(action.data));
> +        } else if (action.method === 'playEarcon' && this.webaudioEnabled) {
> +          let audioBuffer = this.earconBuffers[action.data];
> +          if (audioBuffer) {
> +            let node = this.audioContext.createBufferSource();

Still not sure what would happen if window was replaced with a new one (on detach/attach).

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #6)
> @@ +585,5 @@
> > +            new window.SpeechSynthesisUtterance(action.data));
> > +        } else if (action.method === 'playEarcon' && this.webaudioEnabled) {
> > +          let audioBuffer = this.earconBuffers[action.data];
> > +          if (audioBuffer) {
> > +            let node = this.audioContext.createBufferSource();
> 
> Still not sure what would happen if window was replaced with a new one (on
> detach/attach).

The weak reference thing didn't work that well, because this script is the exclusive reference holder of the buffer, and so if it is only a weak reference, it will get gc'ed.

I am replacing it with a single-member weak map where the window is the key. So if the window goes away, the buffer (that is owned by it) goes away too. This also solves the issue with multiple windows (which is entirely hypothetical now since there are many issues with multi-top-level-window support, and it is not needed).

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Implement speech output with Web Speech API. r=yzen

Updated patch.

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: Implement speech output with Web Speech API. r=yzen

OK, fixed one little bug i introduced in the refactor.

Comment 10

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/321a502794eb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/321a502794eb