Closed Bug 1291839 Opened 3 years ago Closed 3 years ago

[Narrate] Changing speech rate does restart paragraph

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox49 --- unaffected
firefox50 + verified
firefox51 --- verified

People

(Reporter: eeejay, Assigned: jaws)

References

Details

Attachments

(3 files)

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 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/
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!
I assume that's because the input event would restart the speech synthesis? Can we not change rate without restarting the speech?
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.
[Tracking Requested - why for this release]: The Narrate slider is broken when adjusted via the mouse.
(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.
(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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
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".
Attachment #8781363 - Flags: review?(eitan) → review+
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.
"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.
Depends on: 1295719
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
https://hg.mozilla.org/mozilla-central/rev/e90c59d60ded
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Eitan, should we uplift the fix to Aurora50?
Flags: needinfo?(eitan)
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.
Flags: needinfo?(eitan)
Attachment #8781363 - Flags: approval-mozilla-aurora?
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+
Attachment #8781363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(rkothari)
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!
Flags: needinfo?(rkothari)
Attachment #8781363 - Flags: approval-mozilla-aurora+
Hi Jared, should we uplift the fix from this bug and from bug 1295719 to Beta50?
Flags: needinfo?(jaws)
Yes, it looks like this can get uplifted to Beta50.
Flags: needinfo?(jaws)
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
Attachment #8799520 - Flags: approval-mozilla-beta?
(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 on attachment 8799520 [details] [diff] [review]
Patch for beta50 uplift

Reader mode shipped in Fx49, we should be incrementally improving its quality, Beta50+
Attachment #8799520 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: camelia.badau
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.