1291839 - [Narrate] Changing speech rate does restart paragraph

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Eitan Isaacson [:eeejay]]

STR:
1. Load an article in readermode.
2. Press play in narrate.
3. Change rate slider with mouse.

Result: Article continues to be read with old rate
Expected: Currently read paragraph should restart with new rate.

We relied on a certain DOM event order when user drags the range slider. Specifically, we relied on an "input" event after "mouseup". It seems like that is not the case anymore, at least in nightly.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1291839 - Fix setting rate in narrate.

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

Comment 2

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

Comment on attachment 8777500 [details]
Bug 1291839 - Fix setting rate in narrate.

https://reviewboard.mozilla.org/r/69028/#review66798

::: toolkit/components/narrate/NarrateControls.jsm:144
(Diff revision 1)
> -      case "mousedown":
> +        case "mousedown":
> -        this._rateMousedown = true;
> +          this._rateMousedown = true;
> -        break;
> +          break;
> -      case "mouseup":
> +        case "mouseup":
> -        this._rateMousedown = false;
> +          this._rateMousedown = false;

Actually, this bug will be fixed by removing the rateMouseDown variable.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1287321 which changed this behavior.

To note, I used mozregression to find which bug changed this behavior. It was pretty easy and only took me about 5 minutes. http://mozilla.github.io/mozregression/

Comment 3

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/69028/#review66798

> Actually, this bug will be fixed by removing the rateMouseDown variable.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1287321 which changed this behavior.
> 
> To note, I used mozregression to find which bug changed this behavior. It was pretty easy and only took me about 5 minutes. http://mozilla.github.io/mozregression/

Simply removing `_rateMouseDown` is not functionally the same. We don't want to get intermediate `input` events, only one after the user lets go of the slider.

Thanks for the tip on mozregression!

Comment 4

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

I assume that's because the input event would restart the speech synthesis? Can we not change rate without restarting the speech?

Comment 5

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

So the fix here is wrong though, and the reason why is because we are getting 'input' events, but we're ignoring them due to rateMouseDown. 

So we're faking the last one by falling through the case statement because the last 'input' event isn't received after the mouseup because it was fired when the range value changed already.

Ideally we would be able to change the rate of the speech without requiring it to restart.

Comment 6

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

[Tracking Requested - why for this release]: The Narrate slider is broken when adjusted via the mouse.

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> So the fix here is wrong though, and the reason why is because we are
> getting 'input' events, but we're ignoring them due to rateMouseDown. 
> 
> So we're faking the last one by falling through the case statement because
> the last 'input' event isn't received after the mouseup because it was fired
> when the range value changed already.
> 

Good points. I'll work another patch.

> Ideally we would be able to change the rate of the speech without requiring
> it to restart.

It is not possible technically, and even if it was I am not sure I agree. If the slider had instant affect on the rate the user would experience a delayed feedback loop as they move the slider and wait to hear changes.

I think we may have discussed this in the original feature bug.

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> So the fix here is wrong though, and the reason why is because we are
> getting 'input' events, but we're ignoring them due to rateMouseDown. 
> 
> So we're faking the last one by falling through the case statement because
> the last 'input' event isn't received after the mouseup because it was fired
> when the range value changed already.
> 
> Ideally we would be able to change the rate of the speech without requiring
> it to restart.

Actually on second thought, I don't really understand your feedback.

The other case we need to account for is keyboard input. When the user presses arrows or pgup/pgdown/home/end, we receive an input event. In the keyboard case we want to apply the rate change on each keypress.

To reiterate: with pointer input we only want to trigger a rate change on mouseup, with keyboard input we want to trigger a rate change on each keypress. The current patch accommodates those two cases.

Was there something else that was wrong in the patch that I missed?

Comment 10

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

Attachment: Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

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

Comment 11

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

Eitan, what do you think of this patch? It may be rough, I threw it together as a proof of concept but it worked fine for me in quick testing. 'change' events are only dispatched when the mouse is released, and are dispatched on keyup as well.

Comment 12

[Eitan Isaacson [:eeejay]]

Comment on attachment 8781363 [details]
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

https://reviewboard.mozilla.org/r/71804/#review69510

Looks good! I don't think this was possible when the original code was written (or I really messed up). I think "change" events would only get emitted after the input was blurred, at least when adjusting with the keyboard.

::: toolkit/components/narrate/NarrateControls.jsm:143
(Diff revision 1)
> -      case "input":
> -        this._onRateInput(evt);
> -        break;
>        case "change":
> +        let rateRange = this._doc.querySelector("#narrate-rate > input");
> +        if (evt.target == rateRange) {

Seems cheaper just to check target id. Should be "narrate-rate-input".

Comment 13

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

Comment on attachment 8781363 [details]
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

https://reviewboard.mozilla.org/r/71804/#review69510

> Seems cheaper just to check target id. Should be "narrate-rate-input".

Cool, much better. I didn't know there was an ID on this.

Comment 14

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

"change" events aren't being fired for PAGE_UP, PAGE_DOWN, Shift+Left, or Shift+Right. I filed bug 1295719 and attached a test case so that should get fixed soon.

Comment 15

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

https://hg.mozilla.org/integration/fx-team/rev/e90c59d60ded2bf4b2d2a29d0aa1e912f5bacf72
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider. r=eeejay

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e90c59d60ded

Comment 17

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

Hi Eitan, should we uplift the fix to Aurora50?

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 8781363 [details]
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

Sorry for the late response.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Moving the rate slider will not change the speech rate.
[Describe test coverage new/current, TreeHerder]: No new coverage was added. This is hard to test in a mochitest.
[Risks and why]: Little risk.
[String/UUID change made/needed]: None.

Comment 19

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

Comment on attachment 8781363 [details]
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

Fixes a bug in reader mode that is manifested when playback rate is changed, stabilized in Nightly for ~10 days, Aurora50+

Comment 20

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

note that the functionality in this bug depends on the changes being made in bug 1295719 which just got r+ and would also need uplift to aurora50 if this gets uplifted.

Comment 21

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

Comment on attachment 8781363 [details]
Bug 1291839 - Change the Narrate input[type=range] to use 'change' events instead of 'input' events because we are not interested in intermediate values of the slider.

It seems the fix from bug 1295719 isn't ready for Aurora. Please re-nominate both fixes when it's ready for landing. Thanks!

Comment 22

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

Hi Jared, should we uplift the fix from this bug and from bug 1295719 to Beta50?

Comment 23

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

Yes, it looks like this can get uplifted to Beta50.

Comment 24

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

Attachment: Patch for beta50 uplift

Approval Request Comment
[Feature/regressing bug #]: this bug, Narrate Mode released in 49, nice to have fix in follow-up release
[User impact if declined]: using the keyboard to change narration speed in Reader Mode has no effect
[Describe test coverage new/current, TreeHerder]: automated tests and baked on central and aurora
[Risks and why]: low risk, baked on nightly and aurora
[String/UUID change made/needed]: none

Comment 25

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

(In reply to Jared Wein [:jaws] (overloaded with reviews / please needinfo? me) from comment #24)
> Created attachment 8799520 [details] [diff] [review]
> Patch for beta50 uplift
> 
> Approval Request Comment
> [Feature/regressing bug #]: this bug, Narrate Mode released in 49, nice to
> have fix in follow-up release
> [User impact if declined]: using the keyboard to change narration speed in
> Reader Mode has no effect
> [Describe test coverage new/current, TreeHerder]: automated tests and baked
> on central and aurora
> [Risks and why]: low risk, baked on nightly and aurora
> [String/UUID change made/needed]: none

Depends on the patches in bug 1286509 and bug 1295719.

Comment 26

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

Comment on attachment 8799520 [details] [diff] [review]
Patch for beta50 uplift

Reader mode shipped in Fx49, we should be incrementally improving its quality, Beta50+

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/61f5a6000c75

Comment 28

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

Reproduced the initial issue on Windows 7 x64 using Nightly 51.0a1 (2016-08-03).

Verified fixed on Windows 7 x64, Ubuntu 16.04 x64 and Mac OS X 10.11 using latest Aurora 51.0a2 (buildID: 20161025004017) and Firefox 50 Beta 10 (build ID: 20161024172922).