Closed Bug 1169019 Opened 7 years ago Closed 4 years ago

Update conditions when the screen reader should be speaking.

Categories

(Firefox OS Graveyard :: Gaia::System::Accessibility, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: yzen, Assigned: obara.justin)

References

Details

(Keywords: access, leave-open)

Attachments

(2 files, 2 obsolete files)

Screen reader should:
* Only speak when its setting is set
* Have a 'force' parameter when we want the speech to happen (screen reader off, volume up/down, ...?)
* Get rid of cancel speech logic if possible.
Depends on: 1171184
Attachment #8654615 - Flags: review?(yzenevich)
Is there a way to write a unit test that ensure that the underlying speechSynthesizer's speak function was called or not?

Also, could you remind me why we want to remove the the cancel speech logic. I'm worried that it will keep speaking any queued items even after the screenreader has been disabled without this, but I want to make sure I fully understand the issue before continuing on with it.
Flags: needinfo?(yzenevich)
(In reply to jobara from comment #2)
> Is there a way to write a unit test that ensure that the underlying
> speechSynthesizer's speak function was called or not?

Yes you should be able to use the mock like this test does:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/accessibility_test.js#L6

You can also stub functions that you expect to be called and verify that they indeed were, and with the arguments you expected.

> 
> Also, could you remind me why we want to remove the the cancel speech logic.
> I'm worried that it will keep speaking any queued items even after the
> screenreader has been disabled without this, but I want to make sure I fully
> understand the issue before continuing on with it.

So I think we can also check that the screen is enabled when speaking (see logic https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.js#L397) but that depends on the shade accessibility settings that we talked about.

Also I think logic around isSpeakingHints might be unnecessary.

This is also unnecessary https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.js#L317 since I think cancel hints is called always before this function.

So i think we can simplify but not get rid of cancel hints and only keep: removal of the timeout + cancel speech part.
Flags: needinfo?(yzenevich)
Comment on attachment 8654615 [details] [review]
[gaia] jobara:1169019 > mozilla-b2g:master

Looks good! see comments above, Thanks!
Attachment #8654615 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #3)
> (In reply to jobara from comment #2)
> > Is there a way to write a unit test that ensure that the underlying
> > speechSynthesizer's speak function was called or not?
> 
> Yes you should be able to use the mock like this test does:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> accessibility_test.js#L6
> 
> You can also stub functions that you expect to be called and verify that
> they indeed were, and with the arguments you expected.

Thanks, I've updated the tests to use the mocks and stubs.
> 
> > 
> > Also, could you remind me why we want to remove the the cancel speech logic.
> > I'm worried that it will keep speaking any queued items even after the
> > screenreader has been disabled without this, but I want to make sure I fully
> > understand the issue before continuing on with it.
> 
> So I think we can also check that the screen is enabled when speaking (see
> logic
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> js#L397) but that depends on the shade accessibility settings that we talked
> about.

It seems to already stop speaking when the screen is disabled. I don't know how to enable the shade though. Is there a gesture for that?

> 
> Also I think logic around isSpeakingHints might be unnecessary.
> 
> This is also unnecessary
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> js#L317 since I think cancel hints is called always before this function.
> 
> So i think we can simplify but not get rid of cancel hints and only keep:
> removal of the timeout + cancel speech part.

I've removed the isSpeakingHints variable and simplified the cancelHints function.
Flags: needinfo?(yzenevich)
(In reply to jobara from comment #5)
> (In reply to Yura Zenevich [:yzen] from comment #3)
> > (In reply to jobara from comment #2)
> > > Is there a way to write a unit test that ensure that the underlying
> > > speechSynthesizer's speak function was called or not?
> > 
> > Yes you should be able to use the mock like this test does:
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> > accessibility_test.js#L6
> > 
> > You can also stub functions that you expect to be called and verify that
> > they indeed were, and with the arguments you expected.
> 
> Thanks, I've updated the tests to use the mocks and stubs.
> > 
> > > 
> > > Also, could you remind me why we want to remove the the cancel speech logic.
> > > I'm worried that it will keep speaking any queued items even after the
> > > screenreader has been disabled without this, but I want to make sure I fully
> > > understand the issue before continuing on with it.
> > 
> > So I think we can also check that the screen is enabled when speaking (see
> > logic
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > js#L397) but that depends on the shade accessibility settings that we talked
> > about.
> 
> It seems to already stop speaking when the screen is disabled. I don't know
> how to enable the shade though. Is there a gesture for that?

I think it's in the accessibility settings menu.

> 
> > 
> > Also I think logic around isSpeakingHints might be unnecessary.
> > 
> > This is also unnecessary
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > js#L317 since I think cancel hints is called always before this function.
> > 
> > So i think we can simplify but not get rid of cancel hints and only keep:
> > removal of the timeout + cancel speech part.
> 
> I've removed the isSpeakingHints variable and simplified the cancelHints
> function.
Flags: needinfo?(yzenevich)
The PR looks good, once you ensure that nothing weird happens on shading, feel free to mark me for review. Thanks!
(In reply to Yura Zenevich [:yzen] from comment #6)
> (In reply to jobara from comment #5)
> > (In reply to Yura Zenevich [:yzen] from comment #3)
> > > (In reply to jobara from comment #2)
> > > > Is there a way to write a unit test that ensure that the underlying
> > > > speechSynthesizer's speak function was called or not?
> > > 
> > > Yes you should be able to use the mock like this test does:
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> > > accessibility_test.js#L6
> > > 
> > > You can also stub functions that you expect to be called and verify that
> > > they indeed were, and with the arguments you expected.
> > 
> > Thanks, I've updated the tests to use the mocks and stubs.
> > > 
> > > > 
> > > > Also, could you remind me why we want to remove the the cancel speech logic.
> > > > I'm worried that it will keep speaking any queued items even after the
> > > > screenreader has been disabled without this, but I want to make sure I fully
> > > > understand the issue before continuing on with it.
> > > 
> > > So I think we can also check that the screen is enabled when speaking (see
> > > logic
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > > js#L397) but that depends on the shade accessibility settings that we talked
> > > about.
> > 
> > It seems to already stop speaking when the screen is disabled. I don't know
> > how to enable the shade though. Is there a gesture for that?
> 
> I think it's in the accessibility settings menu.

I only have the "color filter" and "screen reader" items under the accessibility settings. And the "screen reader" subitem doesn't have an option for shade either. Could it be somewhere else, or is my version some how out of date?

> 
> > 
> > > 
> > > Also I think logic around isSpeakingHints might be unnecessary.
> > > 
> > > This is also unnecessary
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > > js#L317 since I think cancel hints is called always before this function.
> > > 
> > > So i think we can simplify but not get rid of cancel hints and only keep:
> > > removal of the timeout + cancel speech part.
> > 
> > I've removed the isSpeakingHints variable and simplified the cancelHints
> > function.
Attachment #8654615 - Flags: review?(yzenevich)
(In reply to jobara from comment #8)
> (In reply to Yura Zenevich [:yzen] from comment #6)
> > (In reply to jobara from comment #5)
> > > (In reply to Yura Zenevich [:yzen] from comment #3)
> > > > (In reply to jobara from comment #2)
> > > > > Is there a way to write a unit test that ensure that the underlying
> > > > > speechSynthesizer's speak function was called or not?
> > > > 
> > > > Yes you should be able to use the mock like this test does:
> > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> > > > accessibility_test.js#L6
> > > > 
> > > > You can also stub functions that you expect to be called and verify that
> > > > they indeed were, and with the arguments you expected.
> > > 
> > > Thanks, I've updated the tests to use the mocks and stubs.
> > > > 
> > > > > 
> > > > > Also, could you remind me why we want to remove the the cancel speech logic.
> > > > > I'm worried that it will keep speaking any queued items even after the
> > > > > screenreader has been disabled without this, but I want to make sure I fully
> > > > > understand the issue before continuing on with it.
> > > > 
> > > > So I think we can also check that the screen is enabled when speaking (see
> > > > logic
> > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > > > js#L397) but that depends on the shade accessibility settings that we talked
> > > > about.
> > > 
> > > It seems to already stop speaking when the screen is disabled. I don't know
> > > how to enable the shade though. Is there a gesture for that?
> > 
> > I think it's in the accessibility settings menu.
> 
> I only have the "color filter" and "screen reader" items under the
> accessibility settings. And the "screen reader" subitem doesn't have an
> option for shade either. Could it be somewhere else, or is my version some
> how out of date?
> 

I updated the device to the latest build, which provided the shade option. It seemed to be working okay with the changes I've made. I've now marked the pull for review on this ticket.
> > 
> > > 
> > > > 
> > > > Also I think logic around isSpeakingHints might be unnecessary.
> > > > 
> > > > This is also unnecessary
> > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.
> > > > js#L317 since I think cancel hints is called always before this function.
> > > > 
> > > > So i think we can simplify but not get rid of cancel hints and only keep:
> > > > removal of the timeout + cancel speech part.
> > > 
> > > I've removed the isSpeakingHints variable and simplified the cancelHints
> > > function.
Comment on attachment 8654615 [details] [review]
[gaia] jobara:1169019 > mozilla-b2g:master

Looks good, just removing r? for the last time to address one regression:

We also need to force speach on the first instruction when turning the screen reader on (the one that says: "please press blabla 3 more times". It is currently not happening because of no force.
Attachment #8654615 - Flags: review?(yzenevich)
^ Oh and the unit test for that case too :). Thanks, Justin!
Attachment #8654615 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #10)
> Comment on attachment 8654615 [details] [review]
> [gaia] jobara:1169019 > mozilla-b2g:master
> 
> Looks good, just removing r? for the last time to address one regression:
> 
> We also need to force speach on the first instruction when turning the
> screen reader on (the one that says: "please press blabla 3 more times". It
> is currently not happening because of no force.

I've made the change you suggested and added a new unit test as well.
(In reply to jobara from comment #12)
> (In reply to Yura Zenevich [:yzen] from comment #10)
> > Comment on attachment 8654615 [details] [review]
> > [gaia] jobara:1169019 > mozilla-b2g:master
> > 
> > Looks good, just removing r? for the last time to address one regression:
> > 
> > We also need to force speach on the first instruction when turning the
> > screen reader on (the one that says: "please press blabla 3 more times". It
> > is currently not happening because of no force.
> 
> I've made the change you suggested and added a new unit test as well.

It looks good, though I think we are still swallowing "Screen reader started" announcement.
Could you take a look when you have time, Justin ^
Flags: needinfo?(obara.justin)
Comment on attachment 8654615 [details] [review]
[gaia] jobara:1169019 > mozilla-b2g:master

removing for now , while you're taking care of the, hopefully, final issue.
Attachment #8654615 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #15)
> Comment on attachment 8654615 [details] [review]
> [gaia] jobara:1169019 > mozilla-b2g:master
> 
> removing for now , while you're taking care of the, hopefully, final issue.

I've been trying to look into this for the past couple days, but I keep getting an error when I try to run make on gaia. Is there something wrong with building gaia right now, something changed, or is it just something in my setup?
Flags: needinfo?(obara.justin)
(In reply to jobara from comment #16)
> (In reply to Yura Zenevich [:yzen] from comment #15)
> > Comment on attachment 8654615 [details] [review]
> > [gaia] jobara:1169019 > mozilla-b2g:master
> > 
> > removing for now , while you're taking care of the, hopefully, final issue.
> 
> I've been trying to look into this for the past couple days, but I keep
> getting an error when I try to run make on gaia. Is there something wrong
> with building gaia right now, something changed, or is it just something in
> my setup?

This was an issue with having run make with sudo before. Did "make really-clean" to remove those files.
Spoke with yzen today about the outstanding regression. It seems that the screenreader enabled announcements are being swallowed because it is happening before the screenreader enabled setting is flipped on. The solution is to handle the screenreader announcements in Gaia instead of passing the event from gecko. The first step will be to remove the announcement event triggers from gecko. Remove the following:

https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/AccessFu.jsm#149-151
https://dxr.mozilla.org/mozilla-central/source/accessible/jsat/AccessFu.jsm#168-170
and any tests.

Add trigger the speech handler in gaia in this case:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/accessibility.js#L152
Attached patch 1169019.patch (obsolete) — Splinter Review
Removes the announcement of screenreader on/off from gecko. This will be handled by Gaia instead.
Attachment #8673440 - Flags: review?(yzenevich)
Comment on attachment 8673440 [details] [diff] [review]
1169019.patch

Review of attachment 8673440 [details] [diff] [review]:
-----------------------------------------------------------------

See my comment, feel free to re-flag once you address it.

::: accessible/tests/mochitest/jsat/test_alive.html
@@ -23,4 @@
>      function settingsStart() {
>        ok(true, "EventManager was stopped.");
>        isnot(AccessFu._enabled, true, "AccessFu was disabled.");
> -      AccessFuTest.once({

So we need to make sure that the access fu is enabled. What I noticed is that we already have 
Logger.info('Enabled'); and Logger.info('Disabled'); for the two functions. Lets move them to the end. Then in the test, instead of the current AccessFuTest.once we should use AccessFuTest.once_log and only continue when the logging happens.
Attachment #8673440 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #20)
> Comment on attachment 8673440 [details] [diff] [review]
> 1169019.patch
> 
> Review of attachment 8673440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See my comment, feel free to re-flag once you address it.
> 
> ::: accessible/tests/mochitest/jsat/test_alive.html
> @@ -23,4 @@
> >      function settingsStart() {
> >        ok(true, "EventManager was stopped.");
> >        isnot(AccessFu._enabled, true, "AccessFu was disabled.");
> > -      AccessFuTest.once({
> 
> So we need to make sure that the access fu is enabled. What I noticed is
> that we already have 
> Logger.info('Enabled'); and Logger.info('Disabled'); for the two functions.
> Lets move them to the end. Then in the test, instead of the current
> AccessFuTest.once we should use AccessFuTest.once_log and only continue when
> the logging happens.

How do you make use of the Logger? I've tried AccessFuTest.once_log("Enabled", AccessFuTest.nextTest); and AccessFuTest.once_log("Logger.Enabled", AccessFuTest.nextTest); But neither seems to work in the test.
Flags: needinfo?(yzenevich)
(In reply to jobara from comment #21)
> (In reply to Yura Zenevich [:yzen] from comment #20)
> > Comment on attachment 8673440 [details] [diff] [review]
> > 1169019.patch
> > 
> > Review of attachment 8673440 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > See my comment, feel free to re-flag once you address it.
> > 
> > ::: accessible/tests/mochitest/jsat/test_alive.html
> > @@ -23,4 @@
> > >      function settingsStart() {
> > >        ok(true, "EventManager was stopped.");
> > >        isnot(AccessFu._enabled, true, "AccessFu was disabled.");
> > > -      AccessFuTest.once({
> > 
> > So we need to make sure that the access fu is enabled. What I noticed is
> > that we already have 
> > Logger.info('Enabled'); and Logger.info('Disabled'); for the two functions.
> > Lets move them to the end. Then in the test, instead of the current
> > AccessFuTest.once we should use AccessFuTest.once_log and only continue when
> > the logging happens.
> 
> How do you make use of the Logger? I've tried
> AccessFuTest.once_log("Enabled", AccessFuTest.nextTest); and
> AccessFuTest.once_log("Logger.Enabled", AccessFuTest.nextTest); But neither
> seems to work in the test.

I would suggest installing our screen reader emulator add-on for your build. After that if you look at it in dev tools panel, you can change the log level and try enabling/disabling the screen reader to see what the log prints out.
Flags: needinfo?(yzenevich)
Attached patch 1169019-b.patch (obsolete) — Splinter Review
Unit tests now verify that _enabled was changed
Attachment #8673440 - Attachment is obsolete: true
Attachment #8678392 - Flags: review?(yzenevich)
Comment on attachment 8678392 [details] [diff] [review]
1169019-b.patch

Looks good, Justin, could you remove the changes to the strings file. We don nor remove old value from there.
Attachment #8678392 - Flags: review?(yzenevich) → review+
Ping me when you want me to land this.
Comment on attachment 8684661 [details] [diff] [review]
Removes screenreader announcement from Gecko. Does not modify the Accessfu.properties file.

Looks good thanks!
Attachment #8684661 - Flags: review?(yzenevich) → review+
I tried to update my Gaia PR with master the other night. I had a merge conflict related to the speak function. It seems that it now returns a promise. The plan had been to ignore the speak request if the screen reader wasn't enabled. However, now that it is returning a promise I'm not sure what the appropriate course of action should be. Should it ignore the request, or should it return a promise and reject it. Any thoughts?
Flags: needinfo?(yzenevich)
(In reply to jobara from comment #31)
> I tried to update my Gaia PR with master the other night. I had a merge
> conflict related to the speak function. It seems that it now returns a
> promise. The plan had been to ignore the speak request if the screen reader
> wasn't enabled. However, now that it is returning a promise I'm not sure
> what the appropriate course of action should be. Should it ignore the
> request, or should it return a promise and reject it. Any thoughts?

Yes, I think in Accessibility.speak we would do the same check and if screen reader is disabled we just return Promise.reject() object instead of calling speak and returning its return value promise.
Flags: needinfo?(yzenevich)
I updated the gaia pull request, although I'm not sure it's been added here by the autolander.

https://github.com/mozilla-b2g/gaia/pull/31592
Flags: needinfo?(yzenevich)
(In reply to jobara from comment #33)
> I updated the gaia pull request, although I'm not sure it's been added here
> by the autolander.
> 
> https://github.com/mozilla-b2g/gaia/pull/31592

Looks good Justin, just the comments in Github and it will be ready to land
Flags: needinfo?(yzenevich)
Flags: a11y-review?
Added a comment in the PR. What do you think? You can then mark the attachment itself for feedback? or a11y-review? Thanks!
Flags: a11y-review?
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.