1166365 - Speak the article (speech synthesis in reader mode)

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

The time is right; let's help users consume articles eyes-free. The exact feature here might evolve but right now we're thinking users should have a way of activating TTS output of articles in reader mode. Word highlighting here would be great for literacy.

Eitan is good for implementation. We need Ux...

CC'ing Ehsan because he brought this up separately (he currently switches to Safari for this functionality).

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Sevaan, would you have cycles to Ux this? We are thinking something like a simple button (at least for version 1).

Comment 2

[Sevaan Franks [:sevaan]]

Hey DavidB, I'm currently dedicated to Hello work, but have flagged this for the Firefox Desktop Backlog so it can be reviewed and prioritized.

This would be particularly handy for mobile as it would allow users to listen to articles while on the go.

Comment 3

[Marco Zehe (:MarcoZ)]

I am super excited about this!

I have a few ideas about this too, coming from my experiences working with Freedom Scientific before coming to Mozilla. Especially for multilingual environments, it might be necessary to give the users choice about the voice that speaks the text somehow. I am thinking this:

1. Choose a default language voice based on the browser language.
2. If the page has a lang attribute or xml-lang, try to find a matching language voice for that and use it instead. If there is no such attribute, or the lookup fails, fall back to 1.

We may also want speech rate be controllable somewhere.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> I am super excited about this!
> 
> I have a few ideas about this too, coming from my experiences working with
> Freedom Scientific before coming to Mozilla. Especially for multilingual
> environments, it might be necessary to give the users choice about the voice
> that speaks the text somehow. I am thinking this:
> 
> 1. Choose a default language voice based on the browser language.
> 2. If the page has a lang attribute or xml-lang, try to find a matching
> language voice for that and use it instead. If there is no such attribute,
> or the lookup fails, fall back to 1.
> 

This behavior is exactly how the speech api spec is designed and how we currently have it implemented, so it should just work.

> We may also want speech rate be controllable somewhere.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: WIP Reader mode speech

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Reader speech screenshot

With no UX guidance, I started playing around with this. The screenshot shows what the patch has now. If you want to test it, you need speech synthesis, which is not yet available for desktop platforms. In the meantime, you could use the espeak addon:
https://addons.mozilla.org/en-US/firefox/addon/espeak-web-speech-api/

More info on the addon:
http://blog.monotonous.org/2015/04/29/espeak-web-speech-api-addon/

Comment 7

[Justin Dolske [:Dolske]]

I think we need to think carefully about if this should be part of the built-in UI or not. Feels like add-on material to me, especially since Reader View is intended to be minimal UI to focus on the article.

Comment 8

[Eitan Isaacson [:eeejay]]

I completely agree that we should avoid feature creep. In reader mode, especially.

The reason I think this would fit into reader mode is because it is a feature that is common and expected in e-readers. The first Kindle introduced this, and it has remained a popular feature since.

Comment 9

[Markus Jaritz [:designakt] (UX)]

Shouldn't such a function apply to every text I want read, and not only to articles?
I agree with :Dolske that this will make a great add-on that can be applied to every text. Either full text, or a selection. On any website or in reader view.

Comment 10

[(no longer active)]

(In reply to Markus Jaritz [:maritz] (UX) from comment #9)
> Shouldn't such a function apply to every text I want read, and not only to
> articles?

Ideally, yes.  But if you try this in Safari's implementation for example you'll see that it's a pain to use in practice because we won't have any idea where in the page the interesting content resides, so we would have to speak it all.

I think that with a good UI, we should be able to overcome this issue.  What I have in mind is a small overlay around the text that is being spoken, which the user could quickly drag to the part of the page they want spoken.  But getting that right takes some effort on the UI and the underlying implementation so I think it's better to start from the reader mode where we can predict the structure of the page.

> I agree with :Dolske that this will make a great add-on that can be applied
> to every text. Either full text, or a selection. On any website or in reader
> view.

FWIW, a high quality implementation where we would for example highlight the text being spoken is actually very difficult to do as an add-on because we would need a fast way to highlight the text in Gecko, and doing it in an add-on would make us resort to hacks.

Comment 11

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

(In reply to Justin Dolske [:Dolske] from comment #7)
> I think we need to think carefully about if this should be part of the
> built-in UI or not. Feels like add-on material to me, especially since
> Reader View is intended to be minimal UI to focus on the article.

The current UI doesn't seem to complicate things and would be a great addition to the mode, IMO

Comment 12

[Eitan Isaacson [:eeejay]]

Here is a build that works, not perfect.

Some features I think are missing:
1. Auto language detection, the lang attribute cannot always be trusted.
2. Voice selection, user should be able to choose voice.
3. Change voice/rate on the fly, or at least in the next paragraph.
4. Change the icon..
5. More Nice Things.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a132a409cc2

Mac:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6a132a409cc2/try-macosx64/

Windows:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6a132a409cc2/try-win64/

Linux:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6a132a409cc2/try-linux64/

Comment 13

[Eitan Isaacson [:eeejay]]

Made another prototype. You could soon download binaries from this try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f9b68db10f

Comment 14

[Eitan Isaacson [:eeejay]]

Attachment: Screenshot

Here is an updated screenshot.

Here is a description of each control and its behavior:
1. Play button: Speaks the article, starting from the first visible paragraph.
2. Stop button: the pla button turns into the stop button when playing.
3. Skip next button: Only enabled when speaking. Skips to the next paragraph. If at last paragraph, ends speaking.
4. Skip previous button: Speak the previous parahraph. If at first paragraph, start speaking it again.
5. Speed slider: Default is center (0), max is 4 (4 times the normal rate), min is -4 (4 times slower than the normal rate). When the slider is adjusted while speaking, the paragraph will current paragraph will be re-read from the start.
6. Voice select: Default is "automatic", it automatically detects the article language and uses the most appropriate voice. User could also explicitly choose a voice in the pulldown menu. When this is changed, the current paragraph is re-spoken.

Comment 15

[Markus Jaritz [:designakt] (UX)]

(In reply to Eitan Isaacson [:eeejay] from comment #13)
> Made another prototype. You could soon download binaries from this try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f9b68db10f

Wow. Great to be able to have articles read to me. First impression on clicking play: Feels great!
After using it a bit, some questions occurred to me:
- I would have expected the previous button to jump to the beginning of the paragraph, and only on a second click to the previous paragraph. This is what I am used to from audio players.
- Using the previous, next button hides the control-panel, which made it complicated for me to jump to the paragraph I wanted to continue reading from when coming back to the article after having closed the browser.
- Scrolling to a paragraph I want to read and starting to play did not work if the paragraph is longer than the viewport.
- Clicking stop, and play again I expected the reading to continue on the position, or paragraph I had stopped it at. (But it started on the top of the page, in that case 2 paragraphs before the one I was at.)
- Great that the page jumps with the articles that I read, and that the paragraph is marked. With that I can follow the text if I want. However, for me it was contrary to my usual reading habits to always read the last paragraph on the page. Usually I read the top, or the middle one, and then scroll to the next.
(I wonder how this might be handles for a paragraph that is longer than the viewport.)
- On my system (Win10) changing the voice or speed during reading does not result in a change. I have to stop and start again to hear a change.

Could the page - after reaching the end of the viewport scroll the next paragraph to the top of the viewport (maybe minus 2 lines), and continue reading from there. This would minimize jumps and make use of the full page for reading as it does for the first part of an article.

Comment 16

[Eitan Isaacson [:eeejay]]

Thanks, Markus for all the feedback! Some answers inline.

I would love to also get some input/ideas for the visual design.

(In reply to Markus Jaritz [:maritz] (UX) from comment #15)
> (In reply to Eitan Isaacson [:eeejay] from comment #13)
> > Made another prototype. You could soon download binaries from this try run:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f9b68db10f
> 
> Wow. Great to be able to have articles read to me. First impression on
> clicking play: Feels great!
> After using it a bit, some questions occurred to me:
> - I would have expected the previous button to jump to the beginning of the
> paragraph, and only on a second click to the previous paragraph. This is
> what I am used to from audio players.

I considered that behavior, but decided against it. I never encountered double clicking a software button before. It poses a few accessibility challenges. I agree that re-speaking the current paragraph would be a useful function. But maybe a dedicated button would be better?

> - Using the previous, next button hides the control-panel, which made it
> complicated for me to jump to the paragraph I wanted to continue reading
> from when coming back to the article after having closed the browser.

It will hide it if the previous/next initiates a scroll. That is the default behavior for popups in reader mode. Ultimately, I think the speaking controls should always remain visible when toggled. But the popup would need to have a different geometry that does not obscure the content and remains in the margin.

> - Scrolling to a paragraph I want to read and starting to play did not work
> if the paragraph is longer than the viewport.

Ah, good catch. Right now, it reads the topmost paragraph when its top is not clipped. I think large paragraphs are an important edge case, it should scroll to the beginning of the paragraph and start reading.

> - Clicking stop, and play again I expected the reading to continue on the
> position, or paragraph I had stopped it at. (But it started on the top of
> the page, in that case 2 paragraphs before the one I was at.)

Unfortunately, pause/resume is not supported in all desktop platforms. So I tried to make the behavior accommodate for that and not depend on it by allowing finer granularity by paragraph..

> - Great that the page jumps with the articles that I read, and that the
> paragraph is marked. With that I can follow the text if I want. However, for
> me it was contrary to my usual reading habits to always read the last
> paragraph on the page. Usually I read the top, or the middle one, and then
> scroll to the next.
> (I wonder how this might be handles for a paragraph that is longer than the
> viewport.)

I think scrolling habits could be subjective. I scroll the minimal amount necessary to read the next block of text. I think in this case, scrolling the minimal amount makes sense because it is the least disruptive, and the user does not lose their bearings.

> - On my system (Win10) changing the voice or speed during reading does not
> result in a change. I have to stop and start again to hear a change.
> 

Ah, that was a bug. You could get a fixed build here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feadf9f8a52a

> Could the page - after reaching the end of the viewport scroll the next
> paragraph to the top of the viewport (maybe minus 2 lines), and continue
> reading from there. This would minimize jumps and make use of the full page
> for reading as it does for the first part of an article.

I see what you mean now.. I could play around with that. Yes.

Comment 17

[Markus Jaritz [:designakt] (UX)]

Some replies to your answers to show possible solutions.
(There is a lot of logic in this feature.)

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Thanks, Markus for all the feedback! Some answers inline.
> 
> I would love to also get some input/ideas for the visual design.

I think the visual design is fine, it aligns very well with the Type controls.
The voice dropdown could maybe use an arrow indicating it is a dropdown.
If you want more detailed visual design feedback ask :mmaslaney or :shorlander


> (In reply to Markus Jaritz [:maritz] (UX) from comment #15)
> > - I would have expected the previous button to jump to the beginning of the
> > paragraph, and only on a second click to the previous paragraph. This is
> > what I am used to from audio players.
> 
> I considered that behavior, but decided against it. I never encountered
> double clicking a software button before. It poses a few accessibility
> challenges. I agree that re-speaking the current paragraph would be a useful
> function. But maybe a dedicated button would be better?

Try iTunes or MusicBee and you'll see they implement the behavior I described.
It is not so much a double click as 2 clicks, as it requires far less speed.
Both players will jump to the previous track in the first few seconds, 
but after a few seconds jump to the beginning of the track first.
If you want to experience the difference it makes, compare with VLC which 
does not do the jump to the beginning of the track.

Having a dedicated button for it might pose a challenge as it is not a common 
button used in time-based controls.

> > - Using the previous, next button hides the control-panel, which made it
> > complicated for me to jump to the paragraph I wanted to continue reading
> > from when coming back to the article after having closed the browser.
> 
> It will hide it if the previous/next initiates a scroll. That is the default
> behavior for popups in reader mode. Ultimately, I think the speaking
> controls should always remain visible when toggled. But the popup would need
> to have a different geometry that does not obscure the content and remains
> in the margin.

Having the popup stay visible while navigating will be an improvement, 
even if the UI is sometimes overlaying some content. 
(As the content will be listened to, and not looked at most of the time.)

> > - Clicking stop, and play again I expected the reading to continue on the
> > position, or paragraph I had stopped it at. (But it started on the top of
> > the page, in that case 2 paragraphs before the one I was at.)
> 
> Unfortunately, pause/resume is not supported in all desktop platforms. So I
> tried to make the behavior accommodate for that and not depend on it by
> allowing finer granularity by paragraph..

The finer granularity by paragraph is very helpful. What confused me was that 
after stop, it didn't start the beginning of the same paragraph it stopped at.
(but rather started on the top of the page again.)
This could be solved by remembering the paragraph it stopped at, to start 
from there. (At least until one would scroll. Then it would again start on top)
Or by showing an indicator which paragraph will be read, even if on stop.
Or by moving the latest paragraph to the top on stop, so that it will be the 
one read on start.

Comment 18

[Eitan Isaacson [:eeejay]]

Attachment: WIP Reader mode speech

Sorry for the long delay, other projects abound. I think I implemented all Markus's feedback. Now, it would be good to get some code feedback before I continue. Gijs, does this implementation look reasonable?

Comment 19

[Eitan Isaacson [:eeejay]]

Michael, would you mind providing some visual feedback? I know, for one the icon should not look like the Hello one. Maybe a megaphone?

Comment 20

[:Gijs (he/him)]

(In reply to Eitan Isaacson [:eeejay] from comment #18)
> Created attachment 8693935 [details] [diff] [review]
> WIP Reader mode speech
> 
> Sorry for the long delay, other projects abound. I think I implemented all
> Markus's feedback. Now, it would be good to get some code feedback before I
> continue. Gijs, does this implementation look reasonable?

I'll try to look at this tomorrow. Too many other reviews today. If you could upload the patch to mozreview instead of splinter that would also help.

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - WIP Reader mode speech r?Gijs

Bug 1166365 - WIP Reader mode speech r?Gijs

Comment 22

[:Gijs (he/him)]

Comment on attachment 8693935 [details] [diff] [review]
WIP Reader mode speech

Clearing this in favour of the mozreview one.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8694293 [details]
MozReview Request: Bug 1166365 - WIP Reader mode speech r?Gijs

https://reviewboard.mozilla.org/r/26717/#review24287

Not a thorough review, but I left nits where I noticed things.

Overall, this looks sane-ish to me, except for one kind of major thing: we should try to make about:reader more extendable and not hardcode this stuff into it.

For the pocket button, Shane is already doing some work in bug 1215694. You should be able to use that to add the speech control things, though you probably will need to do a bit more work so you can introduce more complex markup into the page via the messages that Shane's adding. I imagine you could add another message to support loading an extra JSM into the window, which the buttons would then be able to interact with.

It's not really clear to me how this patch is currently supposed to play with android. It omits Android style changes, but it always adds the Narrator stuff if you have speechSynthesis on the content window. It looks like bug 1184142 is adding that to android, and there's quite an old bug, bug 792363, about having essentially this feature on android. It'll likely need different UI there, but this code doesn't seem to make a distinction.

So yeah, I would prefer a less tight integration, even if that is slightly more work in the short term. That'll help with maintainability and ensuring the code could in theory be a system add-on as well.

::: toolkit/components/reader/AboutReader.jsm:1080
(Diff revision 1)
> -      return;
> -    }
> +    Array.prototype.forEach.call(openDropdowns,
> +      (dropdown) => dropdown.classList.remove("open"));

Nit:

for (let dropdown of openDropdowns) {
  dropdown.classList.remove("open");
}

::: toolkit/themes/shared/aboutReaderControls.css:412
(Diff revision 1)
> +#speech-control-buttons > button {

It'd be neater to use a class selector and then just use the shorthand for background: and override the image with the ID selectors.

::: toolkit/themes/shared/aboutReaderControls.css:463
(Diff revision 1)
> +  padding: 0.25rem 0.5rem;

Everything else here uses px, so your added CSS should probably do the same.

::: toolkit/themes/shared/aboutReaderControls.css:478
(Diff revision 1)
> +#speech-rate-controls input[type="range"]::-moz-range-thumb {
> +    background-color: #808080;
> +    height: 0.75rem;
> +    width: 0.75rem;
> +    border-color: #808080;
> +  }

Some wonky indenting here.

::: toolkit/themes/shared/jar.inc.mn:48
(Diff revision 1)
> -
> +  skin/classic/global/reader/RM-Stop-24x24.svg            (../../shared/reader/RM-Stop-24x24.svg)

Nit: line up (

Comment 24

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: [e-mail]

Design for the Reader View "Speak" function

Updates to include:

• Iconography instead of labels for the speed function, to avoid localization issues

• Combining the label and the button for the Voice function

Comment 25

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: [e-mail]

Behaviors and Spec

Comment 26

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Speak-Controls.zip

Assets

Comment 27

[Eitan Isaacson [:eeejay]]

Very cool! Excited to implement.

Comment 28

[Marco Zehe (:MarcoZ)]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #24)
> • Iconography instead of labels for the speed function, to avoid
> localization issues

Note that these icons will require text alternatives for accessibility, so there will be localization involved in any case.

Comment 29

[Michael Maslaney [:mmaslaney] ([e-mail])]

Yes, that's understandable and thanks for the heads up Marco. On that note, I believe having tool tips for each would be beneficial.

Comment 30

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: FX_RM-Toolbar-Speak-Controls.svg

Toolbar Icon

Comment 31

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Review commit: https://reviewboard.mozilla.org/r/34443/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34443/

Comment 32

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/34445/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34445/

Comment 33

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/34447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34447/

Comment 34

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - Add tests narrator tests.

Review commit: https://reviewboard.mozilla.org/r/34449/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34449/

Comment 35

[:Gijs (he/him)]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

https://reviewboard.mozilla.org/r/34443/#review31181

r=me but please get review from :margaret or someone else from the Mobile team as well.

::: toolkit/components/reader/AboutReader.jsm:286
(Diff revision 1)
> +            this._updatePopupPosition(dropdown);

This used to wait for a reflow, and now is being run immediately. Seems like that should keep its existing behaviour?

::: toolkit/components/reader/AboutReader.jsm:957
(Diff revision 1)
> -        // Wait for reflow before calculating the new position of the popup.
> +  _toggleDropdownClicked: function(aEvent) {

convention in browser frontend is no longer to use aFoo for arguments, and the code already used to just use 'event', and the other things in this file use 'dropdown' etc, so please just use 'event' as the argument name.

Comment 36

[:Gijs (he/him)]

Comment on attachment 8718168 [details]
MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

https://reviewboard.mozilla.org/r/34445/#review31185

Comment 37

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review31187

::: browser/extensions/narrate/.eslintrc:3
(Diff revision 1)
> +    "../../../devtools/.eslintrc"

Why not just extend the default .eslintrc file? That would make more sense, IMHO. Now, if devtools want to change their file, they'll have to update your code to obey whatever changes they make. That doesn't seem like it makes sense.

::: browser/extensions/narrate/bootstrap.js:50
(Diff revision 1)
> +  Services.mm.loadFrameScript("chrome://narrate/content/content.js", true);

It seems to me this should be a process script, so you don't load a new instance into every single tab / content-process-browser ? I think it should be possible to listen for reader mode being initialized - that's all using messge passing already - from there, and add the controls and style sheets from there, too.

::: browser/extensions/narrate/content/Controls.jsm:20
(Diff revision 1)
> +  this._docRef = Cu.getWeakReference(win.document);

Why do you need both of these, and why are these weak refs but not the message manager one?

::: browser/extensions/narrate/content/Controls.jsm:22
(Diff revision 1)
> +    Cu.reportError("Narrate controls already inserted.");

But the caller in the process/frame script is still going to stick this dead one into its Map(), overwriting the one that was already there, and register a second unload handler? That doesn't seem right...

Also, when does this happen?

::: browser/extensions/narrate/content/Controls.jsm:50
(Diff revision 1)
> +      "</li>";

Please use a template string to make this readable. You can lose all the escaping and concatenation that way.

::: browser/extensions/narrate/content/Controls.jsm:55
(Diff revision 1)
> +    this.voiceSelect = new VoiceSelect(
> +      this.win,
> +      bundle.GetStringFromName("selectvoicelabel"), [
> +        { label: bundle.GetStringFromName("automaticvoice"),
> +          value: "automatic" }]);

If you do want to keep the argument passing here, please make this use temp variables so we don't use 4 levels of indenting in a single statement. It's very hard to read, especially with the array and object brackets being spaced out on one end but not the other.

::: browser/extensions/narrate/content/Controls.jsm:63
(Diff revision 1)
> +    this.voiceSelect.value = Services.prefs.getCharPref(
> +      "extensions.narrate.voice");

Does devtools require a maximum of 80 characters per line or something? Personally, I'd find a single line easier to read here.

::: browser/extensions/narrate/content/Controls.jsm:67
(Diff revision 1)
> +    this.voiceSelect.element.id = "voice-select";

This seems like something VoiceSelect should be doing itself.

::: browser/extensions/narrate/content/Controls.jsm:72
(Diff revision 1)
> +    this._setupButton(dropdown, "#narrate-toggle", "narrate", true);

All of these have IDs, so might as well drop the '#' and use getElementById instead of querySelector.

::: browser/extensions/narrate/content/Controls.jsm:79
(Diff revision 1)
> +    rateRange.value = Services.prefs.getCharPref("extensions.narrate.rate");

Why is this a char pref? Seems like it should be a number?

::: browser/extensions/narrate/content/Controls.jsm:81
(Diff revision 1)
> +    rateRange.addEventListener("change", this._onChange.bind(this));

I would prefer to use an input event with a backup/throttle over a combination of change/keyup. That way we don't have to manually deal with the keyboard changes (and potentially miss other ways of manipulating the slider that don't trigger those events, or alternative keys with which to manipulate the slider).

::: browser/extensions/narrate/content/Controls.jsm:100
(Diff revision 1)
> +        this._mm.sendAsyncMessage("Reader:SetCharPref", {

I have been r-'d before on having generic setCharPref messages. Don't do that; use a message specific to the pref and don't make the pref be an argument. This avoids this API becoming a sandbox-escaping-vector.

::: browser/extensions/narrate/content/Controls.jsm:145
(Diff revision 1)
> +          this.narrator.start(
> +            { rate: this.rate, voice: this.voice }).then(() => {
> +              this._updateSpeechControls(false);
> +            });

This too is really hard to read, because the inner promise callback and the closing braces for the callback and the `then()` call are misindented. It would be simpler if the arguments to `start` were a temp:

```
let options = {rate: this.rate, voice: this.voice};
this.narrator.start(options).then(() => {
  // whatever
});
```

or if you prefer:

```
let {rate, voice} = this;
this.narrator.start({rate, voice}).then(() => {
  // whatever
});
```

::: browser/extensions/narrate/content/Controls.jsm:159
(Diff revision 1)
> +          // _updateSpeechControls will be called async, and we need this
> +          // class removed.
> +          dropdown.classList.remove("keep-open");

I don't understand this comment. The only callers for `_updateSpeechControls` are in this method. Under what circumstances do they get called and does that conflict with narrate-toggle being clicked? Saying 'we need this class removed' also doesn't explain *why* that needs to happen, so I'm lost. Please update the comment and/or remove this line if it's no longer necessary.

::: browser/extensions/narrate/content/Narrator.jsm:48
(Diff revision 1)
> +      // filter out zero sized paragraphs.
> +      let paragraphs = Array.prototype.filter.call(
> +        this._doc.querySelectorAll(queryString), p => {
> +          let bb = p.getBoundingClientRect();
> +          return bb.width && bb.height;
> +        });
> +

Nit:

```
let paragraphs = this._doc.querySelectorAll(queryString);
paragraphs = paragraphs.filter(p => {
  // stuff
});
```

Note that you're now flushing layout for every single paragraph. It seems more likely to be efficient to use the same technique as https://dxr.mozilla.org/mozilla-central/rev/ac39fba33c6daf95b2cda71e588ca18e2eb752ab/toolkit/components/reader/ReaderMode.jsm#131-141 .

::: browser/extensions/narrate/content/Narrator.jsm:94
(Diff revision 1)
> +      if (result.confident) {
> +        this._speechOptions.lang = result.language;
> +      }

What happens if we don't set the language here?

::: browser/extensions/narrate/content/Narrator.jsm:115
(Diff revision 1)
> +    return new Promise(resolve => {

You're chaining a load of promises here, but it doesn't seem anybody actually uses those?

::: browser/extensions/narrate/content/Narrator.jsm:124
(Diff revision 1)
> +      utterance.addEventListener("end", () => {

Does this get called when the page unloads? I would assume so, in which case, `this._win` will start being null and `_speakInner` will throw. You use that weak reference a lot, and it seems like right now, if it ever does go away, the code here wouldn't deal well.

::: browser/extensions/narrate/content/Narrator.jsm:140
(Diff revision 1)
> +    let sortedVoices = Array.from(this._voiceMap.values()).sort(
> +      (a, b) => a.lang > b.lang ? 1 : 0);

`a.lang.localeCompare(b.lang)` ?

If `lang` is just a language code, for say, an English speaker, it does not necessarily make sense that English comes before Spanish comes before French. Can we sort on the localized names of the language?

::: browser/extensions/narrate/content/Narrator.jsm:146
(Diff revision 1)
> +        label: voice.name + " (" + voice.lang + ")",

This should be a localized string with the name and language code as in-string-replaced things (%S).

Please use https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/languageNames.properties to obtain a human-readable equivalent for the language code.

::: browser/extensions/narrate/content/Narrator.jsm:172
(Diff revision 1)
> +    this._canceled = true;
> +    this._win.speechSynthesis.cancel();

Maybe call `_canceled` `_stopped` instead? Because right now you call `speechSynthesis.cancel()` all over the place, but `_canceled` only gets set from `stop()` and so really it means `stopped`. More generally, it would help to have a comment here somewhere that indicated that cancel() fires the `end` event and that will proceed to speak the next paragraph unless `_canceled` (which I would like to be called `_stopped`) was set by calling `stop()`.

::: browser/extensions/narrate/content/Narrator.jsm:199
(Diff revision 1)
> +  reset: function() {
> +    delete this._paragraphsInner;
> +    delete this._voiceMapInner;
> +    this._speechOptions = {};
> +    this._startTime = 0;
> +    this._canceled = false;
> +    this._win.speechSynthesis.cancel();

Nothing seems to ever call this?

::: browser/extensions/narrate/content/VoiceSelect.jsm:11
(Diff revision 1)
> +this.EXPORTED_SYMBOLS = ["VoiceSelect"];

I don't understand why this is a custom control and not just a `<select size=5>` or whatever.

::: browser/extensions/narrate/content/VoiceSelect.jsm:13
(Diff revision 1)
> +function VoiceSelect(win, label, options = [], selected = 0) {

This only ever gets called with 1 option, which is also always the same. Why does it need to take an array? Why does it need to take an option at all - can't it just get the string stuff it needs itself?

::: browser/extensions/narrate/content/content.js:6
(Diff revision 1)
> +/* globals Components, content, Services, removeMessageListener,
> +   addMessageListener, Controls  */

You should already get the Services and Controls one off the lazy module getter; it shouldn't be necessary to add those here.

We should probably define an environment for frame scripts at some point.

::: browser/extensions/narrate/content/content.js:87
(Diff revision 1)
> +

Nit: don't need this newline.

::: browser/extensions/narrate/jar.mn:6
(Diff revision 1)
> +% content narrate %content/ contentaccessible=yes

Please do not make all of this contentaccessible. Register resource URLs for the things you need to use from the unprivileged page.

::: browser/extensions/narrate/moz.build:9
(Diff revision 1)
> +FINAL_TARGET_FILES.features['[e-mail]'] += [

You should get :glandium's (or some other build peer's) review for the build fu here. Probably check with Axel about the l10n side of things (see bug 1240628)

::: browser/extensions/narrate/skin/shared/narrate-controls.css:1
(Diff revision 1)
> +#narrate-dropdown *, #narrate-dropdown *::before, #narrate-dropdown *::after {

Please can we not use `*` as the last part of a descendant selector? The performance is basically horrible, as CSS has to check every element and then walk every element's ancestry tree to see if it can maybe find the earlier stuff in the selector. Seems like there's only a couple of tags here, or maybe we can use a class?

::: browser/extensions/narrate/skin/shared/narrate-controls.css:6
(Diff revision 1)
> +  background-image: url("chrome://narrate/content/img/narrate.svg");

The images should live in skin/ as well.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:138
(Diff revision 1)
> +  height: 40px;

This is confusing, because the ::after is sized in em.

In general, the content that has text here should be sized in em. I don't think I follow why the ::after has a different sizing.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:160
(Diff revision 1)
> +  overflow-y: auto;

Hm, what happens if a voice label is really long?

"Darth Vader ((c) George Lucas) (English)" or whatever. Because IIRC if you set overflow-y to auto, overflow-x will also end up with a scrollbar? Well, sometimes it behaves in unwanted ways. It would be good to check.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8718170 [details]
MozReview Request: Bug 1166365 - Add tests narrator tests.

https://reviewboard.mozilla.org/r/34449/#review31195

Going to clear review here pending some of the stuff from part 3.

Comment 39

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review31187

> But the caller in the process/frame script is still going to stick this dead one into its Map(), overwriting the one that was already there, and register a second unload handler? That doesn't seem right...
> 
> Also, when does this happen?

We now don't keep track of insterted narrate controls anymore, so it doesn't matter.

> This seems like something VoiceSelect should be doing itself.

I'm treating VoiceSelect like a custom element, perhaps one day implement it as a web component. Being so, the actual ID assigned to the top-level element should not be encapsulated, IMHO.

> All of these have IDs, so might as well drop the '#' and use getElementById instead of querySelector.

They aren't inserted in the doc yet, so that doesn't work. I replaced the setup method with a top-level click listener instead. And localization happens in the template.

> Why is this a char pref? Seems like it should be a number?

It is a float. Prefs can't be floats.

> I would prefer to use an input event with a backup/throttle over a combination of change/keyup. That way we don't have to manually deal with the keyboard changes (and potentially miss other ways of manipulating the slider that don't trigger those events, or alternative keys with which to manipulate the slider).

Good idea! Instead of throttling, I only listen to input events that are not sandwiched in mousedown/mouseup.

> What happens if we don't set the language here?

The speech synthesis will rely on the document's lang tag, which is not always present.

> You're chaining a load of promises here, but it doesn't seem anybody actually uses those?

The chain gets resolved when the speech completely ends (ie. doesn't continue to the next paragraph). We use it in Controls.jsm when we call narrator.start(..).then(..).

> Does this get called when the page unloads? I would assume so, in which case, `this._win` will start being null and `_speakInner` will throw. You use that weak reference a lot, and it seems like right now, if it ever does go away, the code here wouldn't deal well.

Good catch!

> `a.lang.localeCompare(b.lang)` ?
> 
> If `lang` is just a language code, for say, an English speaker, it does not necessarily make sense that English comes before Spanish comes before French. Can we sort on the localized names of the language?

Good idea, so on the voice names.

> This should be a localized string with the name and language code as in-string-replaced things (%S).
> 
> Please use https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/languageNames.properties to obtain a human-readable equivalent for the language code.

I don't think that will be enough. We need to have locale as well (eg. British English vs. U.S. English).

> I don't understand why this is a custom control and not just a `<select size=5>` or whatever.

Because select elements are almost impossible to style, specifically the child options. Please tell me I am wrong?

> This only ever gets called with 1 option, which is also always the same. Why does it need to take an array? Why does it need to take an option at all - can't it just get the string stuff it needs itself?

I like keeping localization out of here and treating this like a generic custom element. I'll clean up the arguments, though.

> Nit: don't need this newline.

This file was pretty much rewritten.

> The images should live in skin/ as well.

I did my best. Not really familar with how jar files work.

> This is confusing, because the ::after is sized in em.
> 
> In general, the content that has text here should be sized in em. I don't think I follow why the ::after has a different sizing.

The specs are all in px for the size of the different widgets. ::after is an inline child of the button and participates in the flow of the text, so it should be em.

Comment 40

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34443/diff/1-2/

Comment 41

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/35191/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35191/

Comment 42

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718168 [details]
MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34445/diff/1-2/

Comment 43

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/1-2/

Comment 44

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718170 [details]
MozReview Request: Bug 1166365 - Add tests narrator tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34449/diff/1-2/

Comment 45

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Hi Axel and Pike.

I am not sure I am doing the right thing in regard to bundled extensions. I mimicked the pocket one. Can you guys sign off? Especially in regards to l10n because of bug 1240628.

Comment 46

[Axel Hecht [:Pike]]

How do I denote an r- in mozreview?

I strongly believe that this should not be a system addon.

This should either not be part of Firefox and an external addon, or it should be code like anything else that's designed to ride the trains.

System addons are in an early place, and they're an incredibly uncomfortable place for a project to be in right now. Ask Ian. You need to have a project that has a lot of carrot to take all that stick. Things like AB testing, frequent updates, etc. If you have a lot of work, it's worth to invest in being a system addon. I don't see that here?

As far as l10n of system addons goes, there's no proven way at this point. Hello does what they do, in many ways. Pocket is the same way. So there's no "right thing" as far as I'm concerned, yet. We'll actually need to learn things first.

One more comment, please make sure the player controls are LTR for RTL locales, as that's what it should be.

Comment 47

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #46)
> How do I denote an r- in mozreview?

You can't, right now. Clear review there and r- on bugzilla if so inclined.

> I strongly believe that this should not be a system addon.
> 
> This should either not be part of Firefox and an external addon, or it
> should be code like anything else that's designed to ride the trains.
> 
> System addons are in an early place, and they're an incredibly uncomfortable
> place for a project to be in right now. Ask Ian. You need to have a project
> that has a lot of carrot to take all that stick. Things like AB testing,
> frequent updates, etc.

Why is this an issue if you have no plans to expedite shipping the addon? Doesn't that get rid of all of the 'stick'?

The whole carrot here is modularization and how easily we can ship without it (if, for instance, we hold back speech synthesis a train for some reason). Besides that, there is also no carrot for Pocket, AFAICT - to the best of my knowledge we aren't investing in it at all at this point, never mind updating out-of-band or faster-than-the-train. Patches are still going through the regular approval process for aurora. The only difference is technical.

Comment 48

[:Gijs (he/him)]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

https://reviewboard.mozilla.org/r/35191/#review31887

Comment 49

[:Gijs (he/him)]

(In reply to Eitan Isaacson [:eeejay] from comment #39)
> > Why is this a char pref? Seems like it should be a number?
> 
> It is a float. Prefs can't be floats.

Sure, but the range is [-4, 4] with .25 increments. Seems like it could just be -400 to 400 with 25 increments, and be stored as an int?

> > I would prefer to use an input event with a backup/throttle over a combination of change/keyup. That way we don't have to manually deal with the keyboard changes (and potentially miss other ways of manipulating the slider that don't trigger those events, or alternative keys with which to manipulate the slider).
> 
> Good idea! Instead of throttling, I only listen to input events that are not
> sandwiched in mousedown/mouseup.

Does this work if you down on the element, and then drag off the element?

> > What happens if we don't set the language here?
> 
> The speech synthesis will rely on the document's lang tag, which is not
> always present.

And failing that, fall back to English? Or...?

> > This should be a localized string with the name and language code as in-string-replaced things (%S).
> > 
> > Please use https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/languageNames.properties to obtain a human-readable equivalent for the language code.
> 
> I don't think that will be enough. We need to have locale as well (eg.
> British English vs. U.S. English).

OK, but this should still be a localized string rather than having " (" and ")" hardcoded, ie you want a string like "%S (%S)".

> > I don't understand why this is a custom control and not just a `<select size=5>` or whatever.
> 
> Because select elements are almost impossible to style, specifically the
> child options. Please tell me I am wrong?

They can be styled but there are limits, yes. You could change the fonts used, and the default background color used. I haven't found an incantation for the selected background color. Is the custom styling really that important here?

Note also that this is causing windows-style focus rectangles on OS X for the dropdown, which doesn't seem right (in fact, there should be no focus rectangles like this at all for mouse clicks, only when using keyboard navigation, IIRC).

Comment 50

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review31891

::: browser/extensions/narrate/content/Narrator.jsm:147
(Diff revisions 1 - 2)
> -    let sortedVoices = Array.from(this._voiceMap.values()).sort(
> -      (a, b) => a.lang > b.lang ? 1 : 0);
> +    let sortedVoices = Array.from(this._voiceMap.values()).sort((a, b) =>
> +      a.name.toLocaleLowerCase() > b.name.toLocaleLowerCase() ? 1 : 0);

Same nit:

(a, b) => a.name.localeCompare(b.name)

Seems like it should work here (potentially with .toLocaleLowerCase() if necessary).

::: browser/extensions/narrate/content/Controls.jsm:27
(Diff revision 2)
> +    function localize(pieces) {
> +      let result = pieces[0];
> +      let substitutions = [].slice.call(arguments, 1);

Nit:
```
function localize(pieces, ...substitutions) {
  let result = pieces.shift();
  for (let substitution of substitutions) {
    result += bundle.GetStringFromName(substitution) + pieces.shift();
  }
  return result;
}
```

::: browser/extensions/narrate/jar.mn:7
(Diff revision 2)
> +% skin narrate classic/1.0 %skin/linux/ os=Linux
> +% skin narrate classic/1.0 %skin/osx/ os=Darwin
> +% skin narrate classic/1.0 %skin/windows/ os=WINNT
> +% skin narrate-shared classic/1.0 %skin/shared/

As noted later, because there's no platform-specific CSS we can just use:

% skin narrate classic/1.0 skin/

and put 2 stylesheets under there.

::: browser/extensions/narrate/skin/linux/narrate.css:1
(Diff revision 2)
> +@import url("chrome://narrate-shared/skin/narrate.css");

Considering there is no platform-specific CSS at all, let's just get rid of these and use a single stylesheet on all OSes.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:13
(Diff revision 2)
> +#narrate-dropdown .keep-open .dropdown-popup {
> +  /* Don't obscure other popups */
> +  z-index: 999;
> +}

I don't really understand the comment here in relation to what the rule does, but also, it seems like the DOM structure looks like:

#narrate-dropdown > .dropdown-popup

which means this rule can never match as written.

Can it just be omitted? What were you trying to do here?

::: browser/extensions/narrate/skin/shared/narrate-controls.css:18
(Diff revision 2)
> +#narrate-dropdown .narrate-row {

Nit: you don't need the ID selector here, I think.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:20
(Diff revision 2)
> +  align-items: center;
> +  justify-content: space-between;

This seems like it's not necessary / doing anything?

::: browser/extensions/narrate/skin/shared/narrate-controls.css:40
(Diff revision 2)
> +  font-size: 16px;

For font-sizes we should really use em, even if the original design used px for simplicity.  It also seems like this property actually has no effect anyway (that is, there is no text that is affected by it).

::: browser/extensions/narrate/skin/shared/narrate-controls.css:98
(Diff revision 2)
> +#narrate-rate input[type="range"]::-moz-range-track {

Do all of these need to have the [type="range"] specified? I wouldn't expect so.

It also seems like this element has an id, and is a direct child, so please use the ID selector or, if you prefer not to for some reason, at least a child selector instead of a descendant selector.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:109
(Diff revision 2)
> +    background-color: #808080;
> +    height: 16px;
> +    width: 16px;
> +    border-radius: 8px;
> +    border-width: 0;

Nit: changed indentation?

::: browser/extensions/narrate/skin/shared/narrate-controls.css:120
(Diff revision 2)
> +#narrate-rate span, #narrate-voice label {

There is no span descendant of #narrate-rate, afaict. #narrate-voice doesn't exist, nor are there any <label> documents in the entire tree. :-\

::: browser/extensions/narrate/skin/shared/narrate-controls.css:132
(Diff revision 2)
> +#narrate-dropdown .voiceselect button {

Here too, please use child selectors.

It seems like you could just be using:

.voiceselect > .select-toggle,
.voiceselect > .options > .option {
  ...
}

::: browser/extensions/narrate/skin/shared/narrate-controls.css:140
(Diff revision 2)
> +#narrate-dropdown .voiceselect > button::after {

Here too, please use a class or ID to indicate what element this specifically is (ie, not all the 'button' elements that are in the 'dropdown'), and the #narrate-dropdown seems superfluous.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:152
(Diff revision 2)
> +#narrate-dropdown .voiceselect button.option {

Nit: .voiceselect > .options > .option

and similarly for the remaining selectors relating to the #narrate-dropdown.

::: browser/extensions/narrate/skin/shared/narrate-controls.css:170
(Diff revision 2)
> +#narrate-dropdown .voiceselect:not(.open) > button, .option:last-child {
> +    border-radius: 0 0 3px 3px;
> +}

Nit: you don't need the #id selector here, and the indenting is off, and please put each comma-separated selector on its own line for readability's sake.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8718170 [details]
MozReview Request: Bug 1166365 - Add tests narrator tests.

https://reviewboard.mozilla.org/r/34449/#review31897

::: browser/extensions/narrate/content/Narrator.jsm:126
(Diff revision 2)
> +        if (this._inTest) {
> +          Services.cpmm.sendAsyncMessage("Narrate:StartParagraph",
> +            { voice: utterance.chosenVoiceURI,
> +              rate: utterance.rate,
> +              paragraph: this._index });
> +        }

I haven't reviewed these in detail and will do that when we've nailed down how the code itself works.

However, it seems that here it might be simpler if we exposed the currently-read paragraph promise on the narrator object (assuming it is possible to get to that object somehow).

Generally, the tests should use a ContentTask to run code in the content process, rather than the content-side code sending messages up to the parent (and the manual message passing done in head.js). Then the majority of the testing could just happen inside the ContentTask using similar primitives as we use for chrome-side testing, which will be a lot simpler than the current implementation using manual message passing.

Comment 52

[Axel Hecht [:Pike]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review31907

Reading Gijs' comment, this might apply elsewhere, didn't do a full intl code review.

::: browser/extensions/narrate/content/Narrator.jsm:147
(Diff revision 2)
> +    let sortedVoices = Array.from(this._voiceMap.values()).sort((a, b) =>
> +      a.name.toLocaleLowerCase() > b.name.toLocaleLowerCase() ? 1 : 0);

I think this should use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator.

The languages to pass in might be best to take via navigator.languages.

Comment 53

[Axel Hecht [:Pike]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

(In reply to :Gijs Kruitbosch from comment #47)
<...>
> > I strongly believe that this should not be a system addon.
> > 
> > This should either not be part of Firefox and an external addon, or it
> > should be code like anything else that's designed to ride the trains.
> > 
> > System addons are in an early place, and they're an incredibly uncomfortable
> > place for a project to be in right now. Ask Ian. You need to have a project
> > that has a lot of carrot to take all that stick. Things like AB testing,
> > frequent updates, etc.
> 
> Why is this an issue if you have no plans to expedite shipping the addon?
> Doesn't that get rid of all of the 'stick'?

With the conversation we had on the gofaster call yesterday, we're still a good deal away from having well-understood tree rules, at least for hello, aka, loop in that directory.

> The whole carrot here is modularization and how easily we can ship without
> it (if, for instance, we hold back speech synthesis a train for some
> reason). Besides that, there is also no carrot for Pocket, AFAICT - to the
> best of my knowledge we aren't investing in it at all at this point, never
> mind updating out-of-band or faster-than-the-train. Patches are still going
> through the regular approval process for aurora. The only difference is
> technical.

I think that the modularization can happen as regular code.

The thing is, we have only talked about technical stuff in the past few weeks, as now, as hello is actually uplifting, we start the conversations around policies, and what a system addon might actually turn out to be in practice.

I don't think that this code wants to be subject to the outcome of those conversations, and I don't think that this code should have an impact on those conversations.

Thus I think that this is the wrong place in the repo to put this code.

Comment 54

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #53)
> Comment on attachment 8718169 [details]
> MozReview Request: Bug 1166365 - Introduce Narrate extension. r?Gijs
> 
> (In reply to :Gijs Kruitbosch from comment #47)
> <...>
> > > I strongly believe that this should not be a system addon.
> > > 
> > > This should either not be part of Firefox and an external addon, or it
> > > should be code like anything else that's designed to ride the trains.
> > > 
> > > System addons are in an early place, and they're an incredibly uncomfortable
> > > place for a project to be in right now. Ask Ian. You need to have a project
> > > that has a lot of carrot to take all that stick. Things like AB testing,
> > > frequent updates, etc.
> > 
> > Why is this an issue if you have no plans to expedite shipping the addon?
> > Doesn't that get rid of all of the 'stick'?
> 
> With the conversation we had on the gofaster call yesterday, we're still a
> good deal away from having well-understood tree rules, at least for hello,
> aka, loop in that directory.

I was not on that call, so I don't know what this means. As I said earlier, Pocket is being developed in the same way as it was before it moved into the extensions/ directory, with the same standards of review and approval, AFAICT.

> > The whole carrot here is modularization and how easily we can ship without
> > it (if, for instance, we hold back speech synthesis a train for some
> > reason). Besides that, there is also no carrot for Pocket, AFAICT - to the
> > best of my knowledge we aren't investing in it at all at this point, never
> > mind updating out-of-band or faster-than-the-train. Patches are still going
> > through the regular approval process for aurora. The only difference is
> > technical.
> 
> I think that the modularization can happen as regular code.

This *is* regular code... What are you proposing instead and why is that different?

> The thing is, we have only talked about technical stuff in the past few
> weeks, as now, as hello is actually uplifting, we start the conversations
> around policies, and what a system addon might actually turn out to be in
> practice.
> 
> I don't think that this code wants to be subject to the outcome of those
> conversations, and I don't think that this code should have an impact on
> those conversations.
> 
> Thus I think that this is the wrong place in the repo to put this code.

This makes very little sense to me, maybe because I wasn't in the conference calls... Why does purely the location of the code impact whether this is a "go faster" add-on? PDF.js has lived in this space for years now, and AFAICT nobody has talked about removing it or moving it elsewhere. Same for Shumway.

Comment 55

[Axel Hecht [:Pike]]

(In reply to :Gijs Kruitbosch from comment #54)
> (In reply to Axel Hecht [:Pike] from comment #53)
> > Comment on attachment 8718169 [details]
> > MozReview Request: Bug 1166365 - Introduce Narrate extension. r?Gijs
> > 
> > (In reply to :Gijs Kruitbosch from comment #47)
> > <...>
> > > > I strongly believe that this should not be a system addon.
> > > > 
> > > > This should either not be part of Firefox and an external addon, or it
> > > > should be code like anything else that's designed to ride the trains.
> > > > 
> > > > System addons are in an early place, and they're an incredibly uncomfortable
> > > > place for a project to be in right now. Ask Ian. You need to have a project
> > > > that has a lot of carrot to take all that stick. Things like AB testing,
> > > > frequent updates, etc.
> > > 
> > > Why is this an issue if you have no plans to expedite shipping the addon?
> > > Doesn't that get rid of all of the 'stick'?
> > 
> > With the conversation we had on the gofaster call yesterday, we're still a
> > good deal away from having well-understood tree rules, at least for hello,
> > aka, loop in that directory.
> 
> I was not on that call, so I don't know what this means. As I said earlier,
> Pocket is being developed in the same way as it was before it moved into the
> extensions/ directory, with the same standards of review and approval,
> AFAICT.

pocket hasn't yet shown what it really wants to do. I know that there's conversations that are gonna happen soon, and there's been name-dropping of Github.

> > > The whole carrot here is modularization and how easily we can ship without
> > > it (if, for instance, we hold back speech synthesis a train for some
> > > reason). Besides that, there is also no carrot for Pocket, AFAICT - to the
> > > best of my knowledge we aren't investing in it at all at this point, never
> > > mind updating out-of-band or faster-than-the-train. Patches are still going
> > > through the regular approval process for aurora. The only difference is
> > > technical.
> > 
> > I think that the modularization can happen as regular code.
> 
> This *is* regular code... What are you proposing instead and why is that
> different?

I'd put this code into browser/components. Or even toolkit, if there's a decent chance that any of this code will work on Android.

> > The thing is, we have only talked about technical stuff in the past few
> > weeks, as now, as hello is actually uplifting, we start the conversations
> > around policies, and what a system addon might actually turn out to be in
> > practice.
> > 
> > I don't think that this code wants to be subject to the outcome of those
> > conversations, and I don't think that this code should have an impact on
> > those conversations.
> > 
> > Thus I think that this is the wrong place in the repo to put this code.
> 
> This makes very little sense to me, maybe because I wasn't in the conference
> calls... Why does purely the location of the code impact whether this is a
> "go faster" add-on? PDF.js has lived in this space for years now, and AFAICT
> nobody has talked about removing it or moving it elsewhere. Same for Shumway.

pdf.js and shumway are also external code drops. Not that I think that makes a good argument for them being that.

Comment 56

[Axel Hecht [:Pike]]

Further random comments:

https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1646 says that 
pref("media.webspeech.synth.enabled", true);
is only true for Nightly.

I didn't find code ad-hoc that checks that the speech is actually enabled? Probably should be runtime, though?

Also, I saw an innerHTML = 'localized content' fly by, that's generally a security concern. It'd be better to generate the DOM and then localize that with textContent, or setAttribute.

Comment 57

[Eitan Isaacson [:eeejay]]

(In reply to Axel Hecht [:Pike] from comment #56)
> Further random comments:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.
> js#1646 says that 
> pref("media.webspeech.synth.enabled", true);
> is only true for Nightly.
> 
> I didn't find code ad-hoc that checks that the speech is actually enabled?
> Probably should be runtime, though?

Thanks! That was in my original patch, need to re-introduce it.

Comment 58

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #56)
> Also, I saw an innerHTML = 'localized content' fly by, that's generally a
> security concern. It'd be better to generate the DOM and then localize that
> with textContent, or setAttribute.

We use DTDs all over the place, which have the exact same issue. I don't think it's worth worrying about locales doing this unless we're going to get serious about getting rid of DTDs.

Comment 59

[Axel Hecht [:Pike]]

(In reply to :Gijs Kruitbosch from comment #58)
> (In reply to Axel Hecht [:Pike] from comment #56)
> > Also, I saw an innerHTML = 'localized content' fly by, that's generally a
> > security concern. It'd be better to generate the DOM and then localize that
> > with textContent, or setAttribute.
> 
> We use DTDs all over the place, which have the exact same issue. I don't
> think it's worth worrying about locales doing this unless we're going to get
> serious about getting rid of DTDs.

We have extensive checks for parser footguns in our tools and dashboards and builds for DTDs. Same for the different set of footguns that are in the stringbundle code paths.

There's nothing for xml/html parsing after stringbundle. Anything like & or " goes straight into runtime without any tool or human bothering, really.

Comment 60

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #49)
> (In reply to Eitan Isaacson [:eeejay] from comment #39)
> > > I don't understand why this is a custom control and not just a `<select size=5>` or whatever.
> > 
> > Because select elements are almost impossible to style, specifically the
> > child options. Please tell me I am wrong?
> 
> They can be styled but there are limits, yes. You could change the fonts
> used, and the default background color used. I haven't found an incantation
> for the selected background color. Is the custom styling really that
> important here?
> 

It is if we want it to look good. I'm going to drop this issue..

> Note also that this is causing windows-style focus rectangles on OS X for
> the dropdown, which doesn't seem right (in fact, there should be no focus
> rectangles like this at all for mouse clicks, only when using keyboard
> navigation, IIRC).

I need to look at it in OS X. For a11y purposes I decided to go with simple focus management and keyboard interactions instead of a custom selected style and aria-activedescendent. I guess I could go there..

Comment 61

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #59)
> (In reply to :Gijs Kruitbosch from comment #58)
> > (In reply to Axel Hecht [:Pike] from comment #56)
> > > Also, I saw an innerHTML = 'localized content' fly by, that's generally a
> > > security concern. It'd be better to generate the DOM and then localize that
> > > with textContent, or setAttribute.
> > 
> > We use DTDs all over the place, which have the exact same issue. I don't
> > think it's worth worrying about locales doing this unless we're going to get
> > serious about getting rid of DTDs.
> 
> We have extensive checks for parser footguns in our tools and dashboards and
> builds for DTDs. Same for the different set of footguns that are in the
> stringbundle code paths.
> 
> There's nothing for xml/html parsing after stringbundle. Anything like & or
> " goes straight into runtime without any tool or human bothering, really.

Yes, but for this to be a security issue you'd need to assume malice, and for the tools you describe to be effective you are assuming these aren't external lang-packs distributed on AMO or such. The intersection between those two requirements should be empty. If our localizers are actively trying to hack Firefox through their locales, we shouldn't be distributing those locales and their commit access should be revoked. :-\

It sounds like you really mean "correctness issue", in which case: point taken.

Comment 62

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #55)
> I'd put this code into browser/components. Or even toolkit, if there's a
> decent chance that any of this code will work on Android.

Fine, let's put it in either of those places, then. It's probably more work to get this UI to work correctly on Android; I don't know if Eitan wants to do that in this bug or not.

> > > The thing is, we have only talked about technical stuff in the past few
> > > weeks, as now, as hello is actually uplifting, we start the conversations
> > > around policies, and what a system addon might actually turn out to be in
> > > practice.
> > > 
> > > I don't think that this code wants to be subject to the outcome of those
> > > conversations, and I don't think that this code should have an impact on
> > > those conversations.
> > > 
> > > Thus I think that this is the wrong place in the repo to put this code.
> > 
> > This makes very little sense to me, maybe because I wasn't in the conference
> > calls... Why does purely the location of the code impact whether this is a
> > "go faster" add-on? PDF.js has lived in this space for years now, and AFAICT
> > nobody has talked about removing it or moving it elsewhere. Same for Shumway.
> 
> pdf.js and shumway are also external code drops. Not that I think that makes
> a good argument for them being that.

Let the record show that I am intensely displeased with the fact that we first go and say "significant new features should be implemented as add-ons", start converting existing stuff, and then when people follow that guideline, suddenly it turns out that that isn't quite what we meant - for reasons which are not publicly discussed or clear for people not intimately involved with Hello. It's deeply worrying that I apparently reviewed almost all the "pocket as an add-on" patches but nobody bothered to keep me in the loop about this discussion, longer term goals to which you alluded, or the decisions that are now being made (whatever those are, which is still completely not clear!). Worse, when people then drop the conclusions of those discussions in the bug and I press for reasoning, I get no answers - I still don't know what the current issues with system add-ons are, what things do and don't qualify to be system add-ons, and why this code is different from pdfjs/shumway (besides the 'external code drop', which you in the same comment said isn't why this should be treated differently... so what is?).

All the go faster updates in our desktop meetings are about hello, and no mention was made about general issues with system add-ons - all I heard this Tuesday was that there were expectation issues with relman (which went unspecified) and that those are being worked on. From my perspective, this code does not need treating differently from other Firefox code as far as relman are concerned, so there is no issue.

I don't understand how, as a Firefox peer, I'm supposed to effectively review patches when people then drop in and r- (or r+ if I r-'d, which would be just as confusing) based on information I literally do not have access to. This situation is ridiculous.

Comment 63

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review31891

> I don't really understand the comment here in relation to what the rule does, but also, it seems like the DOM structure looks like:
> 
> #narrate-dropdown > .dropdown-popup
> 
> which means this rule can never match as written.
> 
> Can it just be omitted? What were you trying to do here?

Oops. that was broken, but it is crucial. When a popup is "permanently" opened, in this case when narrate is talking, other popups should open above it.
STR:
1. Open narrate popup and press start.
2. Click on font style popup.

Expected: Font dialog should appear above narrate dialog.
Result: It appears below the narrate one because their z-index is identical and narrate is later in markup order.

Even after fixing the rule it looks like I can't override a scoped style unless I use !important. Instead, I am going to add this rule to aboutReaderControls.css

> Nit: you don't need the ID selector here, I think.

I'll remove unncessary ancestors in selectors.

But for the record, I put them there because this stylesheet is not scoped and it affects the entire document. I thought it was good form to have all the rules be explicit to elements under #narrate-dropdown.

> This seems like it's not necessary / doing anything?

Oh yeah. I guess not.

Comment 64

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review31907

> I think this should use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator.
> 
> The languages to pass in might be best to take via navigator.languages.

Not sure what advantage Collator gives over localeCompare in this case?

navigator.languages only has the user's preferred languages. In reality, the system can provide many other language voices.

Comment 65

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review31891

> I'll remove unncessary ancestors in selectors.
> 
> But for the record, I put them there because this stylesheet is not scoped and it affects the entire document. I thought it was good form to have all the rules be explicit to elements under #narrate-dropdown.

Oh, right, I forgot about how much we use scoped styles for reader mode. There's a bug to put the readerized stuff in an iframe.

Can we move the narrate styles into a scoped sheet as well? You could potentially just dynamically add an @import rule to the existing controls stylesheet. Likewise for the overriding of the earlier rule.

Comment 66

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review31891

> Oh, right, I forgot about how much we use scoped styles for reader mode. There's a bug to put the readerized stuff in an iframe.
> 
> Can we move the narrate styles into a scoped sheet as well? You could potentially just dynamically add an @import rule to the existing controls stylesheet. Likewise for the overriding of the earlier rule.

I had that for a while, and then some change in the mixed content policy happened that somehow blocked those styles. I decided to drop it at that point. If this isn't an addon we won't have that be a problem.

Comment 67

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34443/diff/2-3/

Comment 68

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/1-2/

Comment 69

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718168 [details]
MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34445/diff/2-3/

Comment 70

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/2-3/

Comment 71

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718170 [details]
MozReview Request: Bug 1166365 - Add tests narrator tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34449/diff/2-3/

Comment 72

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Reflagging Gijs for review because of the dropdown z-index addition.

Comment 73

[:Gijs (he/him)]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

https://reviewboard.mozilla.org/r/34443/#review31961

Comment 74

[Axel Hecht [:Pike]]

(In reply to Eitan Isaacson [:eeejay] from comment #64)
> https://reviewboard.mozilla.org/r/34447/#review31907
> 
> > I think this should use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator.
> > 
> > The languages to pass in might be best to take via navigator.languages.
> 
> Not sure what advantage Collator gives over localeCompare in this case?
> 
> navigator.languages only has the user's preferred languages. In reality, the
> system can provide many other language voices.

Reread the docs, as you asked. Shouldn't be much difference, just speed, looking at https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare#Performance.

The languages to pass in to Collator (or localeCompare, to that extent), don't affect the choice of voices, just the way things are ordered.

Given that the .name entries come from the OS, it might even make sense to specify the OS language instead of the UI or web content languages?

Note, these comments only apply to desktop, not Android, as Android doesn't support Intl yet, bug 1215247

Comment 75

[Axel Hecht [:Pike]]

(In reply to :Gijs Kruitbosch from comment #62)

I feel your frustration. I'm in a slightly different boat, but it has the same color.

The way that you perceived that we're all good to do this, I perceived that we're in a land of one-offs.

I've shared some of my thoughts on irc, and I admit that they're not in a tone to put into a bug.

Comment 76

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review31981

Clearing review for now based on IRC conversation with Eitan.

Comment 77

[:Gijs (he/him)]

Comment on attachment 8718170 [details]
MozReview Request: Bug 1166365 - Add tests narrator tests.

https://reviewboard.mozilla.org/r/34449/#review31983

Clearing review for now based on IRC conversation with Eitan.

Comment 78

[:Margaret Leibovic]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

https://reviewboard.mozilla.org/r/34443/#review32281

I built and tested on Android, and this works for me.

Comment 79

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34443/diff/3-4/

Comment 80

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718168 [details]
MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34445/diff/3-4/

Comment 81

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/3-4/

Comment 82

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/2-3/

Comment 83

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

I think MozReview is making a mess of this process..

From what I can see there are two patches that are r+, and two that are still pending review from Gijs.

I just transitioned this feature out of an addon, so a lot of files moved..

Comment 84

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review33129

The tests are orange, so I'm going to clear review on the tests for now.

::: browser/app/profile/firefox.js:1662
(Diff revision 4)
> +pref("browser.narrate.enabled", true);
> +pref("browser.narrate.test", false);
> +pref("browser.narrate.rate", 0);
> +pref("browser.narrate.voice", "automatic");

These prefs should live in something corresponding to toolkit. Because we don't really have such a thing, they should probably live in all.js .

If this is in toolkit then the pref prefix being browser.narrate is also not quite right. It seems the other reader prefs simply use "reader.foo", so we can probably use "reader.narrate.foo".

::: toolkit/components/narrate/NarrateControls.jsm:20
(Diff revision 4)
> +  if (win.document.getElementById("narrate-dropdown")) {
> +    Cu.reportError("Narrate controls already inserted.");

AIUI this shouldn't happen, right? Can we just nuke this and move all of the contents of create() into the constructor?

::: toolkit/components/narrate/NarrateControls.jsm:50
(Diff revision 4)
> +      <li>
> +         <button class="dropdown-toggle button"
> +                 id="narrate-toggle" title="${"narrate"}"></button>
> +       </li>
> +       <li class="dropdown-popup">

Nit: The indenting here doesn't match up.

::: toolkit/components/narrate/NarrateControls.jsm:70
(Diff revision 4)
> +    let branch = Services.prefs.getBranch("browser.narrate.");

Nit: declare this right before you use it.

::: toolkit/components/narrate/NarrateControls.jsm:80
(Diff revision 4)
> +    options.unshift(
> +      { label: gStrings.GetStringFromName("automaticvoice"),
> +        value: "automatic" });

Nit: prevailing style would be:
```
options.unshift({
  label: ...,
  value: ...,
});
```

::: toolkit/components/narrate/NarrateControls.jsm:85
(Diff revision 4)
> +    this.voiceSelect.element.addEventListener(
> +      "change", this._onVoiceChange.bind(this));

The typical way we do this is:

```
element.addEventListener("change", this);
```

and then have a `handleEvent` method on the object that gets called with the event, in which there's a switch statement based on event.type and you delegate things to the right methods (or, for small responses to events, handle them in the case statement itself).

That avoids having to .bind() everything (`handleEvent` is always called with the 'right' value of `this`) and puts all the event handling in one place.

::: toolkit/components/narrate/NarrateControls.jsm:126
(Diff revision 4)
> +    this._mm.sendAsyncMessage("Reader:SetCharPref",
> +      { name: "browser.narrate.voice", value: voice });

This patch seems to have no listeners for this message (or the SetIntPref version) at all, and it doesn't seem to match the changes we made earlier that restricted what could be set to the specific prefs...

Maybe this is why the tests are failing on try?

::: toolkit/components/narrate/NarrateControls.jsm:189
(Diff revision 4)
> +  get _rate() {
> +    return this._doc.querySelector("#narrate-rate > input").value;
> +  },

Seems like both this and `_voice` could legitimately be non-private, esp. as they're only getters?

::: toolkit/components/narrate/Narrator.jsm:81
(Diff revision 4)
> +    if (!paragraph || (paragraph.get && !paragraph.get())) {

Nit: I would just do:

```
paragraph = paragraph && paragraph.get && paragraph.get();

if (!paragraph) {
  return false;
}

let bb = paragraph.getBoundingClientRect();
```

::: toolkit/components/narrate/Narrator.jsm:199
(Diff revision 4)
> +    this._index -= this._index > 0 && this._timeIntoParagraph < 2000 ? 2 : 1;

Nit: make the 2000 a const at the top of the file with a comment indicating what we use it for.

::: toolkit/components/narrate/VoiceSelect.jsm:13
(Diff revision 4)
> +  this.create(label);
> +
> +  for (let option of options) {
> +    this.add(option.label, option.value);
> +  }
> +
> +  this.selectedIndex = 0;

It's not clear to me why the contents of `create()` aren't part of the constructor, or, conversely, why the contents of the constructor after the `create()` call aren't in `create()`.

::: toolkit/components/narrate/VoiceSelect.jsm:33
(Diff revision 4)
> +    button.addEventListener("click", this._clickedButton.bind(this));

Same deal here with the event handlers.

::: toolkit/components/narrate/VoiceSelect.jsm:44
(Diff revision 4)
> +    this._elementRef = Cu.getWeakReference(element);

If you move this up to right after you create the element, then you can use the `this.listbox` getters and friends in this method, instead of having to duplicate the selector.

::: toolkit/components/narrate/VoiceSelect.jsm:53
(Diff revision 4)
> +    option.textContent = label;

You replied to an earlier comment about these labels that you want e.g. "British English" rather than just "English". That's understandable, but it seems like just "English" would be better than "en-US" for the average user, right? Am I missing something?

::: toolkit/components/narrate/VoiceSelect.jsm:63
(Diff revision 4)
> +      this.element.querySelector(".select-toggle").focus();

This leaves focus outlines after clicks, which shouldn't happen. Can we fix this with some custom CSS (maybe using -moz-focusring)?

::: toolkit/components/narrate/VoiceSelect.jsm:68
(Diff revision 4)
> +  close: function() {
> +    this.element.classList.remove("open");
> +    this.listbox.setAttribute("aria-expanded", false);
> +  },

Feels like this is duplicated with toggleList - couldn't callers (there's only 1, AFAICT?) call toggleList(false) ?

::: toolkit/components/narrate/VoiceSelect.jsm:92
(Diff revision 4)
> +          this.selected.previousElementSibling :
> +          this.selected.nextElementSibling;

ctrl/cmd (accel) plus down or up arrow normally opens a select box, AIUI. So this should probably check that modifier key and open the box if it is used.

::: toolkit/components/narrate/VoiceSelect.jsm:104
(Diff revision 4)
> +      case "ArrowUp":
> +      case "ArrowDown":

This should also handle home/end and pageup/pagedn. Even with that it will lack first-letter-based navigation.

This kind of stuff is why I would really prefer we used a native select box. Can you talk to UX and see if we can reconsider?

::: toolkit/components/narrate/VoiceSelect.jsm:123
(Diff revision 4)
> +      oldSelected.removeAttribute("aria-selected");

FWIW, I don't feel qualified to vouch for the aria stuff in here. It would probably be good to get Marco to give this a look, if we do stick with the custom widget.

::: toolkit/components/narrate/VoiceSelect.jsm:149
(Diff revision 4)
> +    } else {
> +      this._updatedDropdown = false;
> +      this._win.requestAnimationFrame(() => {
> +        if (!this._updatedDropdown) {
> +          updateInner();
> +        }
> +
> +        this._updatedDropdown = true;
> +      });
> +    }

This would be shorter and easier to read if it used:

```
} else if (!this._pendingDropdownUpdate) {
  this._pendingDropdownUpdate = true;
  this._win.requestAnimationFrame(() => {
    updateInner();
    delete this._pendingDropdownUpdate;
  });
}
```

::: toolkit/components/narrate/VoiceSelect.jsm:202
(Diff revision 4)
> +  get value() {
> +    let selected = this.selected;
> +    if (selected) {
> +      return selected.dataset.value;
> +    }
> +
> +    return "";

nit: return selected ? selected.dataset.value : "";

::: toolkit/themes/shared/narrateControls.css:5
(Diff revision 4)
> +#narrate-dropdown button {
> +  background-color: transparent;
> +}
> +
> +#narrate-dropdown button:hover:not(:disabled) {
> +  background-color: #eaeaea;
> +}

Now that this is scoped, do we need either of the `#id` selectors?

::: toolkit/themes/shared/narrateControls.css:119
(Diff revision 4)
> +.voiceselect > button.select-toggle,
> +.voiceselect > .options > button.option {

Seems like there should be a bottom border here somewhere:

http://imgur.com/aIa1sLB

doesn't look good.

::: toolkit/themes/shared/narrateControls.css:141
(Diff revision 4)
> +  border-top: 1px solid #e5e5e5;

We use this color 3 times, could make that a CSS variable defined on the `#narrate-dropdown` ? If it's the same as the color we use elsewhere in the reader mode controls (which I kind of assume), then it might make sense to set it up there.

::: toolkit/themes/shared/narrateControls.css:158
(Diff revision 4)
> +.voiceselect:not(.open) > button, .option:last-child {

Nit: Please split this selector across 2 lines.

Comment 85

[:Gijs (he/him)]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

https://reviewboard.mozilla.org/r/35191/#review33145

Comment 86

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review33129

> These prefs should live in something corresponding to toolkit. Because we don't really have such a thing, they should probably live in all.js .
> 
> If this is in toolkit then the pref prefix being browser.narrate is also not quite right. It seems the other reader prefs simply use "reader.foo", so we can probably use "reader.narrate.foo".

Good points. I'm not going to prefix it with "reader." because this feature may break out of reader mode some day(?).

> This patch seems to have no listeners for this message (or the SetIntPref version) at all, and it doesn't seem to match the changes we made earlier that restricted what could be set to the specific prefs...
> 
> Maybe this is why the tests are failing on try?

I forgot to ask about this: In the non-extension setup we would need to go out of our way to setup listeners in the parent process for those prefs. I opted to go with the already existing reader listeners. Should we go through the trouble of putting custom listeners up? I feel like this functionality is useful beyond narrate. Or, why is the prefs API not e10s friendly..

> You replied to an earlier comment about these labels that you want e.g. "British English" rather than just "English". That's understandable, but it seems like just "English" would be better than "en-US" for the average user, right? Am I missing something?

This is a tough issue. I am not sure what the best answer is. The names of the voices are specific to each platform. In most default Linux setups it will be the unlocalized eSpeak voice name which incedentally is also the language, for example "english-us". I checked on Windows, and names are something like "Microsoft David Desktop - English (United States)". Which includes both a voice "name" like David, and the language/region. I am assuming it is localized to the system, but it might not be. In Mac, I need to check, but from memory it is just a name, like Carmit for Hebrew.

It would be nice if we offered some consistency and gaurentee as much localization as possible. Here is the name/lang pair on an en-US voice for Linux and Windows respectively:

* "english-us","en-US"
* "Microsoft David Desktop - English (United States)","en-US"

> This leaves focus outlines after clicks, which shouldn't happen. Can we fix this with some custom CSS (maybe using -moz-focusring)?

After clicks on what? The focus remains on this widget, which I think is similar to the behavior of a stock select element.

> Feels like this is duplicated with toggleList - couldn't callers (there's only 1, AFAICT?) call toggleList(false) ?

It's important here to not touch focus. It is used when the user closes the narrate popup. If focus() is called on the select-toggle then the focus is reset in the doc instead of staying on the narrate toggle button. I could add an optional argument to toggleList..

> This should also handle home/end and pageup/pagedn. Even with that it will lack first-letter-based navigation.
> 
> This kind of stuff is why I would really prefer we used a native select box. Can you talk to UX and see if we can reconsider?

We can.. I kind of like this visual design. I added home/end and pageup/pagedn. Incremental search select would be nice, but I think it may be overkill and add too much complexity.

> FWIW, I don't feel qualified to vouch for the aria stuff in here. It would probably be good to get Marco to give this a look, if we do stick with the custom widget.

I tested in NVDA. I'll ask Marco to give feedback on this feature in general.

Comment 87

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/4-5/

Comment 88

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/3-4/

Comment 89

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718167 [details]
MozReview Request: Bug 1166365 - Pluralize dropdown logic. r=Gijs r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34443/diff/4-5/

Comment 90

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718168 [details]
MozReview Request: Bug 1166365 - Add some padding around paragraphs for nice highlighting. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34445/diff/4-5/

Comment 91

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/5-6/

Comment 92

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/4-5/

Comment 93

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review34115

Almost there. This would probably have been r+ except for 3 things:
- the pref listener stuff
- android. This now gets initialized in AboutReader.jsm which is loaded in both desktop and android, and is behind a pref that is globally set to true. That means it'll now get turned on for android immediately. If we don't want that (right now, straight away), we should `#ifdef` out the enabling pref for android in all.js
- the focus stuff in the select box. I'll reply to your comment.

::: toolkit/components/narrate/NarrateControls.jsm:191
(Diff revisions 4 - 6)
> +      return gLangStrings.GetStringFromName(lang.split(/\W/)[0]);

What is the split() here for? I haven't really looked into your earlier comments in detail, but \W will match non-ascii 'word' characters, like accented latin, cyrillic and other scripts, see the note on MDN's Regular Expression page about \b:

 JavaScript's regular expression engine defines a specific set of characters to be "word" characters. Any character not in that set is considered a word break. This set of characters is fairly limited: it consists solely of the Roman alphabet in both upper- and lower-case, decimal digits, and the underscore character. Accented characters, such as "é" or "ü" are, unfortunately, treated as word breaks.
 
So I doubt this is what you want because it won't work properly in e.g. French and many other languages that use non-ascii-latin characters for 'words'.

What are you trying to do? Maybe splitting on \s will work better? :-)

::: toolkit/components/narrate/VoiceSelect.jsm:120
(Diff revisions 4 - 6)
> +    for (let i = 0; i < pageSize; i++) {
> +      let sibling = up ? next.previousElementSibling : next.nextElementSibling;
> +      if (!sibling) {
> +        break;
> +      }
> +
> +      next = sibling;
> +    }

I believe you can avoid the loop with something like:

```
let container = option.parentNode;
let index = Array.prototype.indexOf.call(container.children, option);
index += up ? -pageSize : pageSize;
if (index < 0 || index >= container.childElementCount) {
  return null;
}
return container.children[index];
```

In fact, this is still suboptimal because if some of the text in the options wraps, their height will not be the same everywhere, so we might be off by a little bit, but I don't see a way around that in the "going up" case. We could cache all the heights as they don't change, but that doesn't seem worth it.

::: toolkit/components/narrate/VoiceSelect.jsm:190
(Diff revisions 4 - 6)
> +    this._win.console.log(toFocus);

Nit: stray console log.

::: toolkit/components/narrate/NarrateControls.jsm:134
(Diff revision 6)
> +    this._mm.sendAsyncMessage("Reader:SetCharPref",
> +      { name: "narrate.voice", value: voice });

There's still no listeners for this, but the tests pass? That's surprising...

I'm fairly sure we're deliberately not letting you "easily" change arbitrary prefs from the content window. Sorry.

So yes, you really do need something like `"Narrate:ChangeSetting"` as a topic that takes an object like `{"voice": voice}` here, and the rate as an object elsewhere, and a listener elsewhere that lets you set exactly those two (I think it's just 2, right?) prefs.

As for where to add the listeners in the parent, I think you should add some code into browser-content.js that listens for the topic of choice, which then sets those prefs.

Comment 94

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review33129

> I forgot to ask about this: In the non-extension setup we would need to go out of our way to setup listeners in the parent process for those prefs. I opted to go with the already existing reader listeners. Should we go through the trouble of putting custom listeners up? I feel like this functionality is useful beyond narrate. Or, why is the prefs API not e10s friendly..

Oh, reader:setcharpref is already a thing. I didn't realize that. Umm. I don't think that's OK. I'll ping someone about that?

> After clicks on what? The focus remains on this widget, which I think is similar to the behavior of a stock select element.

I mean that I click the select widget or its options, and it starts drawing focus outlines around items. Because I'm not using my keyboard on this page, there shouldn't be focus outlines. I expect this is the result of the explicit focus() calls but it's hard to be sure.

Comment 95

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review33129

> Oh, reader:setcharpref is already a thing. I didn't realize that. Umm. I don't think that's OK. I'll ping someone about that?

I filed bug 1252855. For now, let's just keep using the reader mode thing and we can re-evaluate based on the outcome in bug 1252855 in a followup bug or whatever.

Comment 96

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review34115

> There's still no listeners for this, but the tests pass? That's surprising...
> 
> I'm fairly sure we're deliberately not letting you "easily" change arbitrary prefs from the content window. Sorry.
> 
> So yes, you really do need something like `"Narrate:ChangeSetting"` as a topic that takes an object like `{"voice": voice}` here, and the rate as an object elsewhere, and a listener elsewhere that lets you set exactly those two (I think it's just 2, right?) prefs.
> 
> As for where to add the listeners in the parent, I think you should add some code into browser-content.js that listens for the topic of choice, which then sets those prefs.

Dropping this issue as well.

Comment 97

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review33129

> This is a tough issue. I am not sure what the best answer is. The names of the voices are specific to each platform. In most default Linux setups it will be the unlocalized eSpeak voice name which incedentally is also the language, for example "english-us". I checked on Windows, and names are something like "Microsoft David Desktop - English (United States)". Which includes both a voice "name" like David, and the language/region. I am assuming it is localized to the system, but it might not be. In Mac, I need to check, but from memory it is just a name, like Carmit for Hebrew.
> 
> It would be nice if we offered some consistency and gaurentee as much localization as possible. Here is the name/lang pair on an en-US voice for Linux and Windows respectively:
> 
> * "english-us","en-US"
> * "Microsoft David Desktop - English (United States)","en-US"

Yes, on OS X this is just a proper name (like "Agnes") and a language code for the language ("en-US").

It's a little bit evil, but it almost sounds like we'd want to:
- show name + localized version of lang on OS X;
- show only localized version of lang on Linux, if the name is not any more useful (which is what it sounds like);
- show only name on Windows (? I wonder if the MS desktop voice name styling is used by third party voices from JAWS/NVDA...);

Would that make sense?

Comment 98

[:Gijs (he/him)]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

https://reviewboard.mozilla.org/r/35191/#review34127

::: toolkit/components/narrate/test/NarrateTestUtils.jsm:5
(Diff revision 5)
> +/*
> + * This module implements a number of utilities useful for testing.
> + *
> + * Additions to this module should be generic and useful to testing multiple
> + * features. Utilities only useful to a sepcific testing framework should live
> + * in a module specific to that framework, such as BrowserTestUtils.jsm in the
> + * case of mochitest-browser-chrome.
> + */

This is copied from TestUtils.jsm and doesn't seem like it's useful here.

::: toolkit/components/narrate/test/NarrateTestUtils.jsm:52
(Diff revision 5)
> +  isStoppedState: function(window) {

For this and isStartedState, the annoying thing is going to be that if this fails (intermittently) we won't know which of these didn't match expectations.

Can we make this not return a boolean and call is() (which we probably have to pass in...) repeatedly with a description, something like:

is(!window.document.querySelector(this.STOP), false, "Stop button is not available.");

?

::: toolkit/components/narrate/test/NarrateTestUtils.jsm:96
(Diff revision 5)
> +    let deferred = Promise.defer();

Please do not use Promise.jsm in new code. You should have access to the Promise constructor in this scope, and can just do:

return new Promise(resolve => {
  // code goes here.
});

::: toolkit/components/narrate/test/browser_narrate.js:64
(Diff revision 5)
> +    isnot(newspeechinfo.rate, speechinfo.rate, "rate changed");

Might be good to check that the rate pref changed?

::: toolkit/components/narrate/test/browser_voiceselect.js:50
(Diff revision 5)
> +    ok(NarrateTestUtils.selectVoice(content, "urn:moz-tts:fake-direct:lenny"),
> +      "voice selected");

Again, might be useful to check that the pref changed, too.

::: toolkit/components/narrate/test/browser_voiceselect.js:62
(Diff revision 5)
> +  registerCleanupFunction(teardown);

I'm fairly sure you can just add this in the global scope, and right now you're just going to call the teardown function N times, for N add_task'd tests in the file. :-)

::: toolkit/components/narrate/test/head.js:25
(Diff revision 5)
> +  ["browser.reader.detectedFirstArticle", true],

You shouldn't need to set this to true; it's set in the generic testing prefs.

Comment 99

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review33129

> Yes, on OS X this is just a proper name (like "Agnes") and a language code for the language ("en-US").
> 
> It's a little bit evil, but it almost sounds like we'd want to:
> - show name + localized version of lang on OS X;
> - show only localized version of lang on Linux, if the name is not any more useful (which is what it sounds like);
> - show only name on Windows (? I wonder if the MS desktop voice name styling is used by third party voices from JAWS/NVDA...);
> 
> Would that make sense?

I think so. It is far from perfect. I think your question on 3rd party voice names is relevant on Linux as well since the language-as-name scheme may be specific to espeak voices.

> I mean that I click the select widget or its options, and it starts drawing focus outlines around items. Because I'm not using my keyboard on this page, there shouldn't be focus outlines. I expect this is the result of the explicit focus() calls but it's hard to be sure.

I'll draw inspiration from the awesomebar: Highlight either the item the mouse is hovering on or the item that is arrowed to. If arrowing through items, and then switching to mouse, go back to hover styling.

Comment 100

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/6-7/

Comment 101

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/5-6/

Comment 102

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34115

> What is the split() here for? I haven't really looked into your earlier comments in detail, but \W will match non-ascii 'word' characters, like accented latin, cyrillic and other scripts, see the note on MDN's Regular Expression page about \b:
> 
>  JavaScript's regular expression engine defines a specific set of characters to be "word" characters. Any character not in that set is considered a word break. This set of characters is fairly limited: it consists solely of the Roman alphabet in both upper- and lower-case, decimal digits, and the underscore character. Accented characters, such as "é" or "ü" are, unfortunately, treated as word breaks.
>  
> So I doubt this is what you want because it won't work properly in e.g. French and many other languages that use non-ascii-latin characters for 'words'.
> 
> What are you trying to do? Maybe splitting on \s will work better? :-)

This splits lang tags like 'fr-CA'. So it should always be ascii-ish. The delimiter by convention should be a hyphen, but I decided to generalize it a bit in the rare case where an underscore, or some other non-standard delimiter is used.

> I believe you can avoid the loop with something like:
> 
> ```
> let container = option.parentNode;
> let index = Array.prototype.indexOf.call(container.children, option);
> index += up ? -pageSize : pageSize;
> if (index < 0 || index >= container.childElementCount) {
>   return null;
> }
> return container.children[index];
> ```
> 
> In fact, this is still suboptimal because if some of the text in the options wraps, their height will not be the same everywhere, so we might be off by a little bit, but I don't see a way around that in the "going up" case. We could cache all the heights as they don't change, but that doesn't seem worth it.

I chose to not do what you suggest because getting indexOf like that scares me. The amount of node retrievals goes up the farther down you are on a potentially very large list (this is probably not ever an issue, especially here, but the pattern worries me). OTOH, getting the sibling will always be a constant number of node retrieval calls (this is assuming the DOM implementation doesn't do indexOf + 1 :)

You are right that this will not be accurate if a few options are higher than others. I don't think precision in paging is neccessarily important, but you challanged me, so now I will make it more correct ;)

Comment 103

[Axel Hecht [:Pike]]

(In reply to Eitan Isaacson [:eeejay] from comment #102)
> https://reviewboard.mozilla.org/r/34447/#review34115
> 
> > What is the split() here for? I haven't really looked into your earlier comments in detail, but \W will match non-ascii 'word' characters, like accented latin, cyrillic and other scripts, see the note on MDN's Regular Expression page about \b:
> > 
> >  JavaScript's regular expression engine defines a specific set of characters to be "word" characters. Any character not in that set is considered a word break. This set of characters is fairly limited: it consists solely of the Roman alphabet in both upper- and lower-case, decimal digits, and the underscore character. Accented characters, such as "é" or "ü" are, unfortunately, treated as word breaks.
> >  
> > So I doubt this is what you want because it won't work properly in e.g. French and many other languages that use non-ascii-latin characters for 'words'.
> > 
> > What are you trying to do? Maybe splitting on \s will work better? :-)
> 
> This splits lang tags like 'fr-CA'. So it should always be ascii-ish. The
> delimiter by convention should be a hyphen, but I decided to generalize it a
> bit in the rare case where an underscore, or some other non-standard
> delimiter is used.
> 

Can you give me a link to the code paths? I tried to get through reviewboard, and I just fail badly.

I'm kinda confident that I could be helpful here, if I could just find the code.

Comment 104

[Eitan Isaacson [:eeejay]]

(In reply to Axel Hecht [:Pike] from comment #103)
> (In reply to Eitan Isaacson [:eeejay] from comment #102)
> > https://reviewboard.mozilla.org/r/34447/#review34115
> > 
> > > What is the split() here for? I haven't really looked into your earlier comments in detail, but \W will match non-ascii 'word' characters, like accented latin, cyrillic and other scripts, see the note on MDN's Regular Expression page about \b:
> > > 
> > >  JavaScript's regular expression engine defines a specific set of characters to be "word" characters. Any character not in that set is considered a word break. This set of characters is fairly limited: it consists solely of the Roman alphabet in both upper- and lower-case, decimal digits, and the underscore character. Accented characters, such as "é" or "ü" are, unfortunately, treated as word breaks.
> > >  
> > > So I doubt this is what you want because it won't work properly in e.g. French and many other languages that use non-ascii-latin characters for 'words'.
> > > 
> > > What are you trying to do? Maybe splitting on \s will work better? :-)
> > 
> > This splits lang tags like 'fr-CA'. So it should always be ascii-ish. The
> > delimiter by convention should be a hyphen, but I decided to generalize it a
> > bit in the rare case where an underscore, or some other non-standard
> > delimiter is used.
> > 
> 
> Can you give me a link to the code paths? I tried to get through
> reviewboard, and I just fail badly.
> 
> I'm kinda confident that I could be helpful here, if I could just find the
> code.

https://hg.mozilla.org/try/file/da5963fce3d3/toolkit/components/narrate/NarrateControls.jsm#l219

Comment 105

[Axel Hecht [:Pike]]

https://reviewboard.mozilla.org/r/34447/#review34267

Thanks for the link. Found a few things.

::: toolkit/components/narrate/NarrateControls.jsm:219
(Diff revision 7)
> +      return this._langStrings.GetStringFromName(lang.split(/\W/)[0]);

I recommend to make this code actually restrict the match to something that's in languageNames.properties.

[a-z]{2,3} is the things we'll ever add to languageNames.properties.

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/lang specs that we're returning BCP47 language tags, so splitting on '-' should be good enough.

::: toolkit/locales/en-US/chrome/global/narrate.properties:5
(Diff revision 7)
> +narrate = Narrate

This could use a localization note with tips on how to translate this feature name.

"Narrate" might in some languages be more "make up a story" than "read a story".

::: toolkit/locales/en-US/chrome/global/narrate.properties:12
(Diff revision 7)
> +automaticvoice = Automatic

Is there a better name for "Automatic"? Reading comment 14, it's not an automatic voice, but a default by some algorithm?

Either way, this could use a localization note describing in detail what this is.

::: toolkit/locales/en-US/chrome/global/narrate.properties:16
(Diff revision 7)
> +voiceLabel = %S (%S)

The comment should just say "English", and not "U.S. English" here.

Comment 106

[Francesco Lodolo [:flod]]

https://reviewboard.mozilla.org/r/34447/#review34339

::: toolkit/locales/en-US/chrome/global/narrate.properties:20
(Diff revision 7)
> +rateFaster = %S times faster

I wonder about this couple of strings. If I read the code correctly, rate goes from -4 to 4 with a 0.25 step.

In that case we need a proper plural form, assuming that's not a technical issue within an add-on (it also fails with English, "1 times faster"). I confess I've also never seen negative numbers used in playback speed, they usually go from 0.25x to 4x or similar.

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#List_of_Plural_Rules

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

Comment 107

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/34447/#review34339

> I wonder about this couple of strings. If I read the code correctly, rate goes from -4 to 4 with a 0.25 step.
> 
> In that case we need a proper plural form, assuming that's not a technical issue within an add-on (it also fails with English, "1 times faster"). I confess I've also never seen negative numbers used in playback speed, they usually go from 0.25x to 4x or similar.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#List_of_Plural_Rules
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

We only use this string for values greater than 0; values smaller than 0 use rateSlower, so negative numbers aren't used/shown, and 0 uses rateNormal.

I think the simplest solution might be to use:

%S%% faster

and use rate multiplied by 100, so that you end up with:

25% faster

100% faster

200% faster

etc.

It's pretty tricky to come up with anything here that is both understandable without having to think about it too much for every value, and accurately reflects what happens to the narration speed from a maths perspective.

Comment 108

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review34393

With this and the l10n comments addressed, I think we're good here.

::: toolkit/themes/shared/narrateControls.css:9
(Diff revisions 6 - 7)
> -button {
> +.dropdown-popup button {

Can this be a child rather than a descendant selector?

Comment 109

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34339

> We only use this string for values greater than 0; values smaller than 0 use rateSlower, so negative numbers aren't used/shown, and 0 uses rateNormal.
> 
> I think the simplest solution might be to use:
> 
> %S%% faster
> 
> and use rate multiplied by 100, so that you end up with:
> 
> 25% faster
> 
> 100% faster
> 
> 200% faster
> 
> etc.
> 
> It's pretty tricky to come up with anything here that is both understandable without having to think about it too much for every value, and accurately reflects what happens to the narration speed from a maths perspective.

I don't think the percentage language works for slower "100% slower"? I think it would need to be "50% slower".
What about quarter speed? If we simply multiply the speechsynth rate by 10 it would be wrong, it is not "25% slower", it is "75% slower" which is a really obfuscated way of saying quarter speed.

Even before localization, there are two rate representations that need to be used:
1. Speech synth rate: this is dictated by the spec. 1 is normal speed, 2 is twice as fast, 0.5 is half the speed. (is this a geometric progression?)
2. Rate slider value: this needs to have equal steps for similar increases/decreases (ie. and arithmetic progression?). This is also the value we store in prefs.

I think the current language makes sense to use with the first representation:
* If equal to one: "Normal speed"
* If smaller than one: "0.5 times slower"
* If larger than one: "2 times faster"

Comment 110

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34267

> Is there a better name for "Automatic"? Reading comment 14, it's not an automatic voice, but a default by some algorithm?
> 
> Either way, this could use a localization note describing in detail what this is.

Yeah, I'll change it to "Default" in English as well.

Comment 111

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34393

> Can this be a child rather than a descendant selector?

This is so the button style is not applied on the narrate popup toggle button, but only on buttons in the dropdown. So a descendant selector does the trick here.

Comment 112

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/7-8/

Comment 113

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/6-7/

Comment 114

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Reflagging Gijs for review. Changes since last patch are non trivial, specifically:
1. Close voice dropdown on blur.
2. Rate value normalization and localization.

Comment 115

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review34655

Not ticking ship it because I guess for the pluralform.jsm stuff I'd need to review again.

::: toolkit/components/narrate/NarrateControls.jsm
(Diff revisions 7 - 8)
> -          this.voiceSelect.close();

Why was this removed? Can we now not close the dropdown by clicking the anchor a second time?

::: toolkit/components/narrate/NarrateControls.jsm:216
(Diff revisions 7 - 8)
> -      return this._langStrings.GetStringFromName(lang.split(/\W/)[0]);
> +      // country tags will be lower case ascii between 2 and 3 characters long.

Nit: language tags - the country is the trailing part.

::: toolkit/locales/en-US/chrome/global/narrate.properties:5
(Diff revisions 7 - 8)
> +# Narrate, meaning "read the article out loud". This is the name of the feature

Esp. if we start using this more generally, maybe label this "read the page out loud", rather than implying it's always an article?

::: toolkit/locales/en-US/chrome/global/narrate.properties:24
(Diff revisions 7 - 8)
>  rateFaster = %S times faster

Based on https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#List_of_Plural_Rules this should still use PluralForm.jsm because some languages will need changes to the "times faster" bit of "2 times faster" vs. "3 times faster", even though English is fine because now all numbers are going to be > 1.

It doesn't look like PluralForm knows about non-integer numbers so we can't use it for `rateSlower`, and I don't know if/how it works for things like "2.25 times faster", which is possible with the current implementation. Maybe Axel or Francesco can clarify?

That said... if something is playing at half speed, I would actually find "2 times slower" more natural than "0.5 times slower", but maybe that's just me?

Comment 116

[Axel Hecht [:Pike]]

Sadly, floats use different plural forms than ints for some languages. Even trailing zeros matter for some, fwiw.

Looking at cldr, and guessing at the code, we have a 1.25 factor? That'd trigger an issue for French :-/

http://www.unicode.org/cldr/charts/28/supplemental/language_plural_rules.html#fr has the details.

I'm tempted to say "we need to create this". Bug 1215247 being a pain in the behind here. Like, gandalf and stas work with the standards committee to get exactly this into an Intl api :-(

Comment 117

[Axel Hecht [:Pike]]

Actually. This might be neither of the above. It's not like we do 3.5 speeds, or are we?

TBH, I know 2.5 stuffs, and 5th stuff, but 2.5 faster, I don't. Can you ask for people's opinion on .l10n or #l10n?

Comment 118

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #117)
> Actually. This might be neither of the above. It's not like we do 3.5
> speeds, or are we?
> 
> TBH, I know 2.5 stuffs, and 5th stuff, but 2.5 faster, I don't. Can you ask
> for people's opinion on .l10n or #l10n?

I don't understand the penultimate sentence, or who "you" refers to in the last sentence.

Comment 119

[Axel Hecht [:Pike]]

That I don't have an answer for you, because I just don't know what this specific case is.

You could be you or Eitan?

(Had to look up penultimate, too)

Comment 120

[:Gijs (he/him)]

(In reply to Axel Hecht [:Pike] from comment #119)
> That I don't have an answer for you, because I just don't know what this
> specific case is.
> 
> You could be you or Eitan?
> 
> (Had to look up penultimate, too)

In that case, I really don't want to hold this up any more. "5 times faster" is pretty well-accepted English - there is no "5 faster" string, so I don't really understand the objection.

If PluralForm.jsm doesn't deal with non-int values, it sounds like we should just round before passing the number in question to PluralForm, and we can change that if/when PluralForm.jsm grows the necessary support. It's not OK to hold up a feature because we do not have the technology to localize 1 of the relevant strings 100% correctly into 100% of locales.

(In reply to Axel Hecht [:Pike] from comment #116)
> Sadly, floats use different plural forms than ints for some languages. Even
> trailing zeros matter for some, fwiw.
> 
> Looking at cldr, and guessing at the code, we have a 1.25 factor? That'd
> trigger an issue for French :-/
> 
> http://www.unicode.org/cldr/charts/28/supplemental/language_plural_rules.
> html#fr has the details.

I don't understand what this link means or how it explains that "2.5" would need something other than "2" or "3". It has a table but I do not understand how to read the table. Is this just saying that it should be "1.25 jour" and "2.25 jours" ? It's not clear what "1.75" should be - "jour" or "jours" ? In fact, according to the explanation at the top of the table, the 0.0~1.5 range does not include 1.25 because it is specific to the number of digits, so behaviour is undefined for anything with more than 2 digits after the decimal separator. In fact, there also seems to be no entry for anything > 3.5 that has a non-integer part. So much for that specification. :-\

It seems like either rounding or rounding down should work for pluralform in the meantime and give approximately accurate renderings in most if not all locales. If someone comes up with something better, we can deal with it in a dep bug.

Comment 121

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34655

> Why was this removed? Can we now not close the dropdown by clicking the anchor a second time?

We now rely on focus to close the dropdown. This happens automatically in this case. Clicking outside of the dropdown will close it.

Comment 122

[Eitan Isaacson [:eeejay]]

I'm going to remove the text representation of the slider value. That will put to rest this debate. It really is only useful for screen reader users, and it seems like not all screen readers support it anyway.

I'll keep the refactor, we can re-add it in a followup bug.

Comment 123

[Zibi Braniecki [:zbraniecki][:gandalf]]

Almost a fly-by comments:

> if something is playing at half speed, I would actually find "2 times slower" more natural than "0.5 times slower", but maybe that's just me?

Unless English has very different rules than other languages I know, "0.5 times slower" === "2 times faster".
If you want to get "2 times slower" as a fraction, you'd have to go for "0.5 times faster".

Obviously for that reason I'd vote for "2 times slower".

Alternatively, to avoid the whole problem, you can tweak the UI the way VLC did (for similar reasons):

Playback speed: 1.0x (0.5x, 2.0x etc.)

And I would recommend going for that solution because with my localizer hat on, I just spent 10 minutes figuring out the proper plural form for "0.5x" and "1.7x" and it just feels awkward and with my developer hat on, I wish PluralForm used ICU already but it doesn't so we don't have good story for floats.

Comment 124

[Eitan Isaacson [:eeejay]]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34447/diff/8-9/

Comment 125

[Eitan Isaacson [:eeejay]]

Comment on attachment 8720046 [details]
MozReview Request: Bug 1166365 - Add tests for narrate. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35191/diff/7-8/

Comment 126

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/34447/#review34655

> Based on https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#List_of_Plural_Rules this should still use PluralForm.jsm because some languages will need changes to the "times faster" bit of "2 times faster" vs. "3 times faster", even though English is fine because now all numbers are going to be > 1.
> 
> It doesn't look like PluralForm knows about non-integer numbers so we can't use it for `rateSlower`, and I don't know if/how it works for things like "2.25 times faster", which is possible with the current implementation. Maybe Axel or Francesco can clarify?
> 
> That said... if something is playing at half speed, I would actually find "2 times slower" more natural than "0.5 times slower", but maybe that's just me?

Got rid of all this..

Comment 127

[:Gijs (he/him)]

Comment on attachment 8718169 [details]
MozReview Request: Bug 1166365 - Introduce Narrate feature in reader mode. r=Gijs

https://reviewboard.mozilla.org/r/34447/#review34825

Let's do it!

Comment 128

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51459032ccce
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3447f8edd06
https://hg.mozilla.org/integration/mozilla-inbound/rev/df591274c1d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94c22d14ce7

Comment 129

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51459032ccce
https://hg.mozilla.org/mozilla-central/rev/e3447f8edd06
https://hg.mozilla.org/mozilla-central/rev/df591274c1d7
https://hg.mozilla.org/mozilla-central/rev/c94c22d14ce7

Comment 130

[Barbara Bermes [:barbara] - NI please!]

Have you added some probes to measure this?

Comment 131

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Barbara Bermes [:barbara] from comment #130)
> Have you added some probes to measure this?

Probes for narrate usage telemetry? (I don't think we have but it would be interesting)

Comment 132

[:Gijs (he/him)]

(In reply to David Bolter [:davidb] from comment #131)
> (In reply to Barbara Bermes [:barbara] from comment #130)
> > Have you added some probes to measure this?
> 
> Probes for narrate usage telemetry? (I don't think we have but it would be
> interesting)

It would be appropriate to check for usage, yes. Barbara, can you file a followup bug indicating what measures you think are useful? At a guess, some combination/subset of:

- number of times people narrate a page / number of times people view a page in reading mode
- total narrate time / total time reading mode is open
- number of times people pick a different voice / narrated page count
- number of times people change the reading speed / narrated page count

Comment 133

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #132)
> (In reply to David Bolter [:davidb] from comment #131)
> > (In reply to Barbara Bermes [:barbara] from comment #130)
> > > Have you added some probes to measure this?
> > 
> > Probes for narrate usage telemetry? (I don't think we have but it would be
> > interesting)
> 
> It would be appropriate to check for usage, yes. Barbara, can you file a
> followup bug indicating what measures you think are useful? At a guess, some
> combination/subset of:
> 
> - number of times people narrate a page / number of times people view a page
> in reading mode
> - total narrate time / total time reading mode is open
> - number of times people pick a different voice / narrated page count
> - number of times people change the reading speed / narrated page count

- number of times people complete narrating the entire article (expect interruptions, just count times when last paragraph is completed).

Comment 134

[Barbara Bermes [:barbara] - NI please!]

(In reply to David Bolter [:davidb] from comment #131)
> (In reply to Barbara Bermes [:barbara] from comment #130)
> > Have you added some probes to measure this?
> 
> Probes for narrate usage telemetry? (I don't think we have but it would be
> interesting)

Created a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1252617

Comment 135

[Alfred Kayser]

The padding/margin patch to show nice selection frame doesn't work, as the "narrating" selection is applied to the whole "entry-content" instead of each paragraph.
I think that the "narrating" class should be applied to the specific paragraph being read.

Comment 136

[:Gijs (he/him)]

(In reply to Alfred Kayser from comment #135)
> The padding/margin patch to show nice selection frame doesn't work, as the
> "narrating" selection is applied to the whole "entry-content" instead of
> each paragraph.
> I think that the "narrating" class should be applied to the specific
> paragraph being read.

It looks fine to me (tested on a BBC news article), and I'm fairly sure it would have looked worse before that particular patch. Additionally, your "narrating class" comment seems misplaced considering that change is not in the patch you're referencing.

In other words, this comment is not actionable. So, please file a followup bug depending on this bug, with more specifics (like what article you're using to test, on which platform, with which build, and if it reproduces in the default theme). It kind of sounds like it might get fixed by the changes in bug 1254232 but that's impossible to know for sure without more specifics.

Comment 137

[Eitan Isaacson [:eeejay]]

(In reply to Alfred Kayser from comment #135)
> The padding/margin patch to show nice selection frame doesn't work, as the
> "narrating" selection is applied to the whole "entry-content" instead of
> each paragraph.
> I think that the "narrating" class should be applied to the specific
> paragraph being read.

I think that should be solved when bug 1254232 lands.

Comment 138

[David Bolter [:davidb] (NeedInfo me for attention)]

Mark please let me know if we need to (and how to) make sure SUMO, release notes people etc are aware of this new feature as it rides the trains.

Comment 139

[Jared Wein [:jaws] (please needinfo? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Reader Mode can read the article out loud, good for everyone but also good for users with accessibility needs
[Suggested wording]: Reader Mode can now read articles out loud
[Links (documentation, blog post, etc)]: tbd

Comment 140

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #139)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: Reader Mode can read the article out loud, good for
> everyone but also good for users with accessibility needs
> [Suggested wording]: Reader Mode can now read articles out loud
> [Links (documentation, blog post, etc)]: tbd

This isn't riding release trains because the speech synthesis stuff isn't riding release trains, so I don't think we can relnote it.

Comment 141

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #140)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #139)
> > Release Note Request (optional, but appreciated)
> > [Why is this notable]: Reader Mode can read the article out loud, good for
> > everyone but also good for users with accessibility needs
> > [Suggested wording]: Reader Mode can now read articles out loud
> > [Links (documentation, blog post, etc)]: tbd
> 
> This isn't riding release trains because the speech synthesis stuff isn't
> riding release trains, so I don't think we can relnote it.

I plan to have speech enabled for the next release. I think we should relnote it!!

Comment 142

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 release notes.

Comment 143

[Eitan Isaacson [:eeejay]]

(In reply to Ritu Kothari (:ritu) from comment #142)
> Added to Fx47 release notes.

This is going to be released in 49 when speech synth is enabled.

Comment 144

[Jared Wein [:jaws] (please needinfo? me)]

Release Note Request (optional, but appreciated)
This should be relnote: 49+, not 47+

Comment 145

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Marcia, fyi. I moved this relnote in nucleus from 47 to 49 Aurora release notes.

Comment 146

[Liz Henry (:lizzard) (relman/hg->git project)]

Jaws or Eitan, please let me know if you have a post or other document I can link to from the 49 aurora relnotes.

Comment 147

[Jared Wein [:jaws] (please needinfo? me)]

I don't but I hope that Eitan does as it is his feature ;)

I'm just a lowly cheerleader of the feature who wants to make sure that people find out about it :)

Comment 148

[Eitan Isaacson [:eeejay]]

I have a post from the Nightly release. The icon art changed a bit. Let me know if a more recent post would be worthwhile.

https://blog.monotonous.org/2016/03/07/narrate-a-new-feature-in-firefox-nightly/

Comment 149

[Andrei Vaida [:avaida]]

[Test Plan]:
https://wiki.mozilla.org/QA/Speak_the_article