[AccessFu] Add support to TTS

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
This depends on bug #687879.
(Assignee)

Updated

6 years ago
Depends on: 687879
(Assignee)

Comment 1

6 years ago
Created attachment 666601 [details] [diff] [review]
Add tts support to AccessFu.
(Assignee)

Updated

5 years ago
Assignee: nobody → eitan
(Assignee)

Comment 2

5 years ago
Created attachment 781803 [details] [diff] [review]
Implement speech output with Web Speech API
Attachment #666601 - Attachment is obsolete: true
Attachment #781803 - Flags: review?(yura.zenevich)
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 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.
Attachment #781803 - Flags: review?(yura.zenevich)
(Assignee)

Comment 5

5 years ago
Created attachment 784679 [details] [diff] [review]
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.
Attachment #781803 - Attachment is obsolete: true
Attachment #784679 - Flags: review?(yura.zenevich)
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).
Attachment #784679 - Flags: review?(yura.zenevich) → review+
(Assignee)

Comment 7

5 years ago
(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).
(Assignee)

Comment 8

5 years ago
Created attachment 798610 [details] [diff] [review]
Implement speech output with Web Speech API. r=yzen

Updated patch.
(Assignee)

Comment 9

5 years ago
Created attachment 798828 [details] [diff] [review]
Implement speech output with Web Speech API. r=yzen

OK, fixed one little bug i introduced in the refactor.
Attachment #784679 - Attachment is obsolete: true
Attachment #798610 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/321a502794eb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.