1255261 - [Narrate] Don't skip to next section by stop and start

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P5)

Description

[magicp]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160309030419

Steps to reproduce:

1. Start Nightly
2. Go to "https://developer.mozilla.org/Firefox"
3. Switch to reader mode
4. Open narrate-control
5. Click "Start"
6. Click "Stop" and "Start"



Actual results:

In step 6, skip to next section. 


Expected results:

Start from stopped position.

Comment 1

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

Eitan, is this on your radar? How difficult would it be to fix this?

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1255261 - Read current paragraph after pressing stop.

We always had to make sure we were one paragraph back from where we wanted
to start. That seems broken. I made it so that we start from the current
paragraph.

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

Comment 4

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

Comment on attachment 8777503 [details]
Bug 1255261 - Read current paragraph after pressing stop.

https://reviewboard.mozilla.org/r/69040/#review66804

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8777503 [details]
Bug 1255261 - Read current paragraph after pressing stop.

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

Comment 6

[:Gijs (he/him)]

Eitan, seems this was never autolanded, mozreview says:

 We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rewritecommitdescriptions --descriptions=/tmp/tmpx7ttLo 4c30cda8d1a9a8a6c2deff370a4c84fe28191bed: rebasing 339664:489ce1e6d656 "Bug 1255261 - Read current paragraph after pressing stop. r=jaws" (tip)

merging toolkit/components/narrate/Narrator.jsm merging toolkit/components/narrate/test/browser_narrate.js
warning: conflicts while merging toolkit/components/narrate/test/browser_narrate.js!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29c0af9420d
Read current paragraph after pressing stop. r=jaws

Comment 8

[Eitan Isaacson [:eeejay]]

Weird, I rebased before I repushed to mozreview. Anyway, pushed manually to inbound now..

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2069e3662c13
Followup - fix eslint error. CLOSED TREE

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/d29c0af9420d
https://hg.mozilla.org/mozilla-central/rev/2069e3662c13

Comment 11

[ithompson4]

Testday 12th August 2016 reference 1. Text to Speech in Reader Mode Test case 4.1.4. - FAIL.  I encountered this bug during testing. 

This bug is NOT resolved in Firefox 49.0b3 . While for theose with normal vision its a mere inconvenience, it could be a nasty and confusing bug from the point of view of the seriously visually impaired, who may have difficulty finding their way back to where the text breaks take place. Personally I'd rate this bug as Serious.

Comment 12

[:Gijs (he/him)]

(In reply to ithompson4 from comment #11)
> Testday 12th August 2016 reference 1. Text to Speech in Reader Mode Test
> case 4.1.4. - FAIL.  I encountered this bug during testing. 
> 
> This bug is NOT resolved in Firefox 49.0b3 . While for theose with normal
> vision its a mere inconvenience, it could be a nasty and confusing bug from
> the point of view of the seriously visually impaired, who may have
> difficulty finding their way back to where the text breaks take place.
> Personally I'd rate this bug as Serious.

It isn't marked as fixed in 49. It has been addressed in 51, and is marked as such in the bug. In other words, what you're seeing is expected. In order to be addressed for 49, it would need uplifting. Eitan, want to uplift this?

Comment 13

[Eitan Isaacson [:eeejay]]

Comment on attachment 8777503 [details]
Bug 1255261 - Read current paragraph after pressing stop.

Approval Request Comment
[Feature/regressing bug #]: 1255261
[User impact if declined]: Pressing stop narrate and then start again will no read from the last paragraph.
[Describe test coverage new/current, TreeHerder]: Added browser test.
[Risks and why]: A bit of a reworked narrate logic, so there is some risk but it is not high.
[String/UUID change made/needed]: None.

Comment 15

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

Hello, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 16

[magicp]

I have verified this issue was fixed as expected (Read current paragraph after pressing stop) on Nightly build (20160815030201)

Comment 17

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

(In reply to magicp from comment #16)
> I have verified this issue was fixed as expected (Read current paragraph
> after pressing stop) on Nightly build (20160815030201)

Fantastic! Thank you. :)

Comment 18

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

Comment on attachment 8777503 [details]
Bug 1255261 - Read current paragraph after pressing stop.

Fix was verified on Nightly, let's uplift to Aurora50.

Comment 19

[Eitan Isaacson [:eeejay]]

This feature is not available in the release channel. So changing to "unaffected".

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: Read current paragraph after pressing stop (rebased for aurora).

This is a rebased patch for aurora.

Comment 22

[Eitan Isaacson [:eeejay]]

Comment on attachment 8782536 [details] [diff] [review]
Read current paragraph after pressing stop (rebased for aurora).

Same patch that was already approved, just rebased.

Sorry. Don't know how to do this within mozreview.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8782536 [details] [diff] [review]
Read current paragraph after pressing stop (rebased for aurora).

You don't need to re-request approval on simple rebases.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84b11aee0b43

Comment 25

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

Comment on attachment 8777503 [details]
Bug 1255261 - Read current paragraph after pressing stop.

Fix for accessibility reader mode regression in 49, let's uplift this for beta 6.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e6d6eb0ce3c4

Comment 27

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 x64, Mac OSX 10.9.5 and Ubuntu 16.04 x64 using latest Nightly 51.0a1 (buildID: 20160830030201), latest Aurora 50.0a2 (buildID: 20160831004001) and Firefox 49 Beta 8 (buildID: 20160829102229)