jobara Assignee
	Description • 9 years ago

For Bug 1169019 a force option will be added to the speak method, which will allow for speech synthesis when the screenreader is disabled. (e.g. to announce that the screenreader has been disabled, instructions for enabling/disabling). To make use of this new flow, the announcement event from gecko needs to pass along the options to ensure that the disabled state is announced.

	jobara Assignee
	Comment 1 • 9 years ago

I'm having some trouble with the unit test. I'm trying to extend the existing one by verifying that the options are passed down. However, the test is failing with a rather unhelpful error messages.

Data is correct -     Structures begin differing at:
got['options'] = [object Object]
expected['options'] = [object Object]

Is there a way to debug this? I tried to add a console log into the _addObserver method of jsatcommon.js but it didn't seem to output.

Changes: https://github.com/mozilla/gecko-dev/compare/master...jobara:1171184

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 2 • 9 years ago

(In reply to jobara from comment #1)
> I'm having some trouble with the unit test. I'm trying to extend the
> existing one by verifying that the options are passed down. However, the
> test is failing with a rather unhelpful error messages.
> 
> Data is correct -     Structures begin differing at:
> got['options'] = [object Object]
> expected['options'] = [object Object]
> 
> Is there a way to debug this? I tried to add a console log into the
> _addObserver method of jsatcommon.js but it didn't seem to output.
> 
> Changes: https://github.com/mozilla/gecko-dev/compare/master...jobara:1171184

So it looks like the options block content is somehow different, I would suggest taking a look at jsatcommon.js and seeing where we log things and trying to stringify the diff (instead of getting [object Object]). In fact that can be part of the patch. I think to log in test we use built in info() from mochitest framework itself.

	jobara Assignee
	Comment 3 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #2)
> (In reply to jobara from comment #1)
> > I'm having some trouble with the unit test. I'm trying to extend the
> > existing one by verifying that the options are passed down. However, the
> > test is failing with a rather unhelpful error messages.
> > 
> > Data is correct -     Structures begin differing at:
> > got['options'] = [object Object]
> > expected['options'] = [object Object]
> > 
> > Is there a way to debug this? I tried to add a console log into the
> > _addObserver method of jsatcommon.js but it didn't seem to output.
> > 
> > Changes: https://github.com/mozilla/gecko-dev/compare/master...jobara:1171184
> 
> So it looks like the options block content is somehow different, I would
> suggest taking a look at jsatcommon.js and seeing where we log things and
> trying to stringify the diff (instead of getting [object Object]). In fact
> that can be part of the patch. I think to log in test we use built in info()
> from mochitest framework itself.

It looks like Logger is used. I'm using Logger.debug('message') to log the stringified versions of the objects. They are indeed different, as data.details is missing options: {force: true}

	jobara Assignee
	Comment 5 • 9 years ago

Add Bug-1171184.patch which adds the ability to pass options along to the announce method.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 6 • 9 years ago

Comment on attachment 8632597 [details] [diff] [review]
Bug-1171184.patch

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

Looks good with my comment. Note I think the patch might be malformed as I can't see all the changes in Presentation.jsm (though in raw view they look good). Just flag me for r? once the above comment is addressed.

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +78,4 @@
>        if (!data) {
>          return;
>        }
> +      Logger.debug('data.details:', JSON.stringify(data.details), "aWaitForData:", JSON.stringify(aWaitForData));

This should probably be removed.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 8 • 9 years ago

(In reply to jobara from comment #7)
> Created attachment 8633784 [details] [diff] [review]
> Bug-1171184-b.patch - removes logger call

hmm still looks like the same file attached, Justin, could you double check that it looks as expected?

	jobara Assignee
	Comment 9 • 9 years ago

I'm not really sure why, but the logger call keeps showing up in my patch despite the fact that it is no longer in the code. I've manually removed the line from the patch. Hopefully it still applies okay.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 10 • 9 years ago

(In reply to jobara from comment #9)
> Created attachment 8634020 [details] [diff] [review]
> Bug-1171184-c.patch - removes logger call
> 
> I'm not really sure why, but the logger call keeps showing up in my patch
> despite the fact that it is no longer in the code. I've manually removed the
> line from the patch. Hopefully it still applies okay.

So based on the patch message I think you have more than 1 commit in the branch. Could you ensure there's only one (by squashing) before you call git format patch and git patch to hg patch

	jobara Assignee
	Comment 11 • 9 years ago

Sorry, forgot to squash all the commits down. I've done that with Bug-1171184-d.patch.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 12 • 9 years ago

Comment on attachment 8634469 [details] [diff] [review]
Bug-1171184-d.patch: squashed commits

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

r=me with one final nit addressed. Also add r=yzen in the commit message and mark for checkin-needed. Thanks, Justin!

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +78,4 @@
>        if (!data) {
>          return;
>        }
> +      isDeeply(data.details, aWaitForData, 'Data is correct');

Nit: not part of this patch, lets keep this out.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 13 • 9 years ago

Actually, Justin: Eitan and I talked about it more right now and it makes more sense to move announcements about screen reader on and off from Gecko to Gaia and only do it there. Perhaps you would be OK to rename this bug to something close to that. Then the dependent bug for Gaia will be about adding screen reader on/off notifications + simplifying speak logic.

	jobara Assignee
	Comment 14 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #13)
> Actually, Justin: Eitan and I talked about it more right now and it makes
> more sense to move announcements about screen reader on and off from Gecko
> to Gaia and only do it there. Perhaps you would be OK to rename this bug to
> something close to that. Then the dependent bug for Gaia will be about
> adding screen reader on/off notifications + simplifying speak logic.

There is a Gaia one already related to this. https://bugzilla.mozilla.org/show_bug.cgi?id=994715
Should this one just be closed?

	jobara Assignee
	Comment 15 • 9 years ago

After discussion with Yura and Eitan it was decided that the screen reader announcements should remain in Gaia. https://bugzilla.mozilla.org/show_bug.cgi?id=994715 already covers this work.