1254526 - Narrate stops when it gets to a figure inside the article

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[(no longer active)]

STR: Narrate this page: <http://haakonsk.blogg.no/1456259429_if_youre_alive_in_30_.html>

The narration stops at "remaining life expectancy can be illustrated like this" right before we get to the first figure in the article.

Comment 1

[(no longer active)]

Seems like pressing stop will make the narration continue...

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1254526 - Don't let Narrate get into bad state after encountering a synth error. r=Gijs

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

Comment 3

[Eitan Isaacson [:eeejay]]

The other problem here is that we are getting that error in osx because we are sending an empty string to synthesis. I think that is not good. If anything, because of fingerprinting.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8728053 [details]
MozReview Request: Bug 1254526 - Don't let Narrate get into bad state after encountering a synth error. r=Gijs

https://reviewboard.mozilla.org/r/38769/#review35455

I'm a little uncomfortable with the assumption here that for every error we should just try reading the next paragraph. Under what circumstances should we throw the error event per spec? Could it be temporary, in which case, shouldn't we be restarting where we left off?

It seems to me like we could/should use the treewalker to filter out the images and potentially other empty bits of paragraph or whatever?

If we do have to (partially?) rely on the error handler here, I wonder if it needs some kind of backoff/limit. If narration is somehow busted because platform support is missing or causing errors (say, because you stored a voice in the prefs and that voice is now no longer available, or because whatever espeak-type API we use on linux is not installed, or just because software is sometimes awful) then right now it will try to speak every paragraph in turn until it hits the end, all the while not emitting noise (well, or a series of beeps - we don't really know in the general case), and ending up with the page scrolled to the bottom and the user wondering what on earth just happened. That seems unfortunate as well. :-)

Comment 5

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/38769/#review35455

Yup, all legitimate concerns.. I'll revise the patch to reject the promise on error. And for this specific case, I'll have the tree filter remove empty nodes.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 8728053 [details]
MozReview Request: Bug 1254526 - Don't let Narrate get into bad state after encountering a synth error. r=Gijs

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

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1254526 - Filter out empty text nodes from narrate queue. r=Gijs

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8728053 [details]
MozReview Request: Bug 1254526 - Don't let Narrate get into bad state after encountering a synth error. r=Gijs

https://reviewboard.mozilla.org/r/38769/#review35807

Comment 9

[:Gijs (he/him)]

Comment on attachment 8728591 [details]
MozReview Request: Bug 1254526 - Filter out empty text nodes from narrate queue. r=Gijs

https://reviewboard.mozilla.org/r/38983/#review35809

Nice, thanks for the extra comment!

::: toolkit/components/narrate/Narrator.jsm:69
(Diff revision 1)
> +          if (!node.textContent.match(/\S/)) {

Nit:
```
!/\S/.test(node.textContent)
```

is slightly cheaper than `.match`

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 8728053 [details]
MozReview Request: Bug 1254526 - Don't let Narrate get into bad state after encountering a synth error. r=Gijs

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

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8728591 [details]
MozReview Request: Bug 1254526 - Filter out empty text nodes from narrate queue. r=Gijs

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1daaba023b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6bcc1fec08

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa1daaba023b
https://hg.mozilla.org/mozilla-central/rev/5b6bcc1fec08

Comment 14

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Aurora 48.0a2 (buildID: 20160504004015).