Make cvox2 feedback more robust when focusing a text field.

Make cvox2 feedback more robust when focusing a text field.

The editable text handler was speaking unnecessarily on focus.

Modifies MockFeedback to allow us to assert that a certain utterance is not
spoken at a given place in the sequence.

Depends on: https://codereview.chromium.org/1302763002/

BUG=none

Committed: https://crrev.com/5a266cee66909121cc4cfc535ddfed24c0838f7c
Cr-Commit-Position: refs/heads/master@{#348466}

[dmazzoni, 2015-08-26 05:54:51]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, plundblad@chromium.org

[Peter Lundblad, 2015-08-26 16:56:13]

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:479:
if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
Use chrome.automation.RoleType.
no braces around on-line substatements.

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:756:
node.state['protected'],
Why do you use square brackets around 'protected' here?

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:149:
expectNoSpeech: function() {
I think this only works if the unexpected utterance comes first in the queue.
Perhaps that's good enough, but would be good to document.

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:246:
console.log('*** ADDUTTERANCE ' + textString);
Intentional?

[dmazzoni, 2015-08-26 18:18:23]

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:479:
if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
On 2015/08/26 16:56:13, Peter Lundblad wrote:
> Use chrome.automation.RoleType.
> no braces around on-line substatements.

Done.

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:756:
node.state['protected'],
On 2015/08/26 16:56:13, Peter Lundblad wrote:
> Why do you use square brackets around 'protected' here?

I was thinking of it as a set/map. Changed to node.state.protected to match the
rest of the code.

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:149:
expectNoSpeech: function() {
On 2015/08/26 16:56:13, Peter Lundblad wrote:
> I think this only works if the unexpected utterance comes first in the queue.
> Perhaps that's good enough, but would be good to document.

Good point. How about expectNextSpeechUtteranceIsNot()

https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:246:
console.log('*** ADDUTTERANCE ' + textString);
On 2015/08/26 16:56:13, Peter Lundblad wrote:
> Intentional?

Oops, meant to remove that. Is there a way to add a VLOG from JS that I could
turn on temporarily while debugging, like in C++? When a test fails sometimes
it's nice to see exactly what MockFeedback got.

[Peter Lundblad, 2015-08-26 18:34:27]

dmazzoni@chromium.org writes:
> 
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
> (right):
> 
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:479:
> if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
> On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > Use chrome.automation.RoleType.
> > no braces around on-line substatements.
> 
> Done.
> 
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:756:
> node.state['protected'],
> On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > Why do you use square brackets around 'protected' here?
> 
> I was thinking of it as a set/map. Changed to node.state.protected to
> match the rest of the code.
> 
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> File
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
> (right):
> 
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:149:
> expectNoSpeech: function() {
> On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > I think this only works if the unexpected utterance comes first in the
> queue.
> > Perhaps that's good enough, but would be good to document.
> 
> Good point. How about expectNextSpeechUtteranceIsNot()

I think that's a better name because it stands out, addressing a concern I had
but didn't mention before that the two letter difference in name was hard to
me.
Still worth pinting the caveat about this only working for immediately
follwing utterance. I'm concerned that this expectation will easily pass when
it
shouldn't over time, but it is better than nothing and we can try to come
up with something more robust later.

>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:246:
> console.log('*** ADDUTTERANCE ' + textString);
> On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > Intentional?
> 
> Oops, meant to remove that. Is there a way to add a VLOG from JS that I
> could turn on temporarily while debugging, like in C++? When a test
> fails sometimes it's nice to see exactly what MockFeedback got.

Not that I know of.  I've also had this need (it's a mracle that I managed
to remove all instance of logging before sending the code out for review;)
Perhaps we could keep all utterances and log on failure.  something to think
about for a future improvement.

> https://codereview.chromium.org/1318683002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-08-26 18:40:53]

I added a specific caveat to the comment, does that look good/

On Wed, Aug 26, 2015 at 11:34 AM <plundblad@chromium.org> wrote:

> dmazzoni@chromium.org writes:
> >
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> > File
> >
> chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
> > (right):
> >
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:479:
> > if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
> > On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > > Use chrome.automation.RoleType.
> > > no braces around on-line substatements.
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:756:
> > node.state['protected'],
> > On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > > Why do you use square brackets around 'protected' here?
> >
> > I was thinking of it as a set/map. Changed to node.state.protected to
> > match the rest of the code.
> >
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> > File
> > chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
> > (right):
> >
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> > chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:149:
> > expectNoSpeech: function() {
> > On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > > I think this only works if the unexpected utterance comes first in the
> > queue.
> > > Perhaps that's good enough, but would be good to document.
> >
> > Good point. How about expectNextSpeechUtteranceIsNot()
>
> I think that's a better name because it stands out, addressing a concern I
> had
> but didn't mention before that the two letter difference in name was hard
> to me.
> Still worth pinting the caveat about this only working for immediately
> follwing utterance. I'm concerned that this expectation will easily pass
> when it
> shouldn't over time, but it is better than nothing and we can try to come
> up with something more robust later.
>
> >
>
https://codereview.chromium.org/1318683002/diff/1/chrome/browser/resources/ch...
> > chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:246:
> > console.log('*** ADDUTTERANCE ' + textString);
> > On 2015/08/26 16:56:13, Peter Lundblad wrote:
> > > Intentional?
> >
> > Oops, meant to remove that. Is there a way to add a VLOG from JS that I
> > could turn on temporarily while debugging, like in C++? When a test
> > fails sometimes it's nice to see exactly what MockFeedback got.
>
> Not that I know of.  I've also had this need (it's a mracle that I managed
> to remove all instance of logging before sending the code out for review;)
> Perhaps we could keep all utterances and log on failure.  something to
> think
> about for a future improvement.
>
> > https://codereview.chromium.org/1318683002/
>
> --
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2015-08-26 18:42:37]

lgtm

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:479:
if (evt.target.role == RoleType.textField)
Should we have an else here where we clear the editable text handler if some
node with a non-text field role gets focused?  It seems like we could otherwise
keep
state between different fields.

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:746:
* @param {Object} node
chrome.automation.AutomationNode instead of Object.

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:747:
*/
@private

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1318683002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:146: *
@param {...(string|RegExp)} var_args One or more utterance to add as
One or more utterances.

[Peter Lundblad, 2015-08-26 18:45:14]

Dominic Mazzoni writes:
> I added a specific caveat to the comment, does that look good/

Yup, thanks,
//Peter

> On Wed, Aug 26, 2015 at 11:34 AM <plundblad@chromium.org> wrote:
> 
>     dmazzoni@chromium.org writes:
>     >
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/cvox2/background/background.js
>     > File
>     > chrome/browser/resources/chromeos/chromevox/cvox2/background/
>     background.js
>     > (right):
>     >
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/cvox2/background/background.js#newcode479
>     > chrome/browser/resources/chromeos/chromevox/cvox2/background/
>     background.js:479:
>     > if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
>     > On 2015/08/26 16:56:13, Peter Lundblad wrote:
>     > > Use chrome.automation.RoleType.
>     > > no braces around on-line substatements.
>     >
>     > Done.
>     >
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/cvox2/background/background.js#newcode756
>     > chrome/browser/resources/chromeos/chromevox/cvox2/background/
>     background.js:756:
>     > node.state['protected'],
>     > On 2015/08/26 16:56:13, Peter Lundblad wrote:
>     > > Why do you use square brackets around 'protected' here?
>     >
>     > I was thinking of it as a set/map. Changed to node.state.protected to
>     > match the rest of the code.
>     >
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/testing/mock_feedback.js
>     > File
>     > chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
>     > (right):
>     >
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/testing/mock_feedback.js#newcode149
>     > chrome/browser/resources/chromeos/chromevox/testing/
>     mock_feedback.js:149:
>     > expectNoSpeech: function() {
>     > On 2015/08/26 16:56:13, Peter Lundblad wrote:
>     > > I think this only works if the unexpected utterance comes first in
the
>     > queue.
>     > > Perhaps that's good enough, but would be good to document.
>     >
>     > Good point. How about expectNextSpeechUtteranceIsNot()
>    
>     I think that's a better name because it stands out, addressing a concern
I
>     had
>     but didn't mention before that the two letter difference in name was hard
>     to me.
>     Still worth pinting the caveat about this only working for immediately
>     follwing utterance. I'm concerned that this expectation will easily pass
>     when it
>     shouldn't over time, but it is better than nothing and we can try to come
>     up with something more robust later.
>    
>     > https://codereview.chromium.org/1318683002/diff/1/chrome/browser/
>     resources/chromeos/chromevox/testing/mock_feedback.js#newcode246
>     > chrome/browser/resources/chromeos/chromevox/testing/
>     mock_feedback.js:246:
>     > console.log('*** ADDUTTERANCE ' + textString);
>     > On 2015/08/26 16:56:13, Peter Lundblad wrote:
>     > > Intentional?
>     >
>     > Oops, meant to remove that. Is there a way to add a VLOG from JS that I
>     > could turn on temporarily while debugging, like in C++? When a test
>     > fails sometimes it's nice to see exactly what MockFeedback got.
>    
>     Not that I know of.  I've also had this need (it's a mracle that I
managed
>     to remove all instance of logging before sending the code out for
review;)
>     Perhaps we could keep all utterances and log on failure.  something to
>     think
>     about for a future improvement.
>    
>     > https://codereview.chromium.org/1318683002/
>    
>     --
> 

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-08-28 21:08:59]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-08-28 21:08:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1318683002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-28 21:09:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318683002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318683002/60001

[commit-bot: I haz the power, 2015-08-28 22:04:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 22:04:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2015-09-11 19:25:00]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-09-11 19:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1318683002/#ps80001
(title: "EditableTextBase expects start < end")

[commit-bot: I haz the power, 2015-09-11 19:25:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1318683002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1318683002/80001

[commit-bot: I haz the power, 2015-09-11 19:51:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-11 19:52:50]

Patchset 5 (id:??) landed as
https://crrev.com/5a266cee66909121cc4cfc535ddfed24c0838f7c
Cr-Commit-Position: refs/heads/master@{#348466}

[commit-bot: I haz the power, 2015-09-23 12:25:27]

Patchset 5 (id:??) landed as
https://crrev.com/5a266cee66909121cc4cfc535ddfed24c0838f7c
Cr-Commit-Position: refs/heads/master@{#348466}