Make cvox2 output feedback more robust when focusing a text field.

Make cvox2 output feedback more robust when focusing a text field.

To make feedback robust and make the test reliable:

1. Construct the editable text handler on focus, not just on text changed.

2. Add an event queue, like on classic - that way we don't have a focus
   event immediately followed by an event that selects some of the text.

3. Stop MockTTS from skipping over "extra" speech once it's matched the
   first matching utterance - otherwise a test can't assert that only
   the expected speech is generated and not anything else

4. Force ChromeVox Next on in cvox2 tests, otherwise we're getting mixed
   feedback from Classic sometimes, introducing flakiness.

BUG=none

[dmazzoni, 2015-08-19 16:13:22]

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

[dmazzoni, 2015-08-19 16:13:22]

I wanted to get this in before landing earcon changes, because without it I was
having a hard time making robust tests.

[David Tseng, 2015-08-19 17:19:25]

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:83:
this.EVENT_QUEUE_DELAY_MS = 10;
nit: make private.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:427:
* Add an event to the event queue so that we can avoid ignore
avoid or ignore

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:433:
*/
nit: @private

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:446:
* to wake up again.
nit: @private

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:453:
}
nit: remove braces.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:462:
return evt.enqueueTime == lastTimeForEventType[evt.type];
Any issues doing this generically? For example, doesn't editable text base
require processing all text changed / selected text changed events to generate
proper output?

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:534:
if (evt.target.role == 'textField' || evt.target.role == 'textBox') {
nit: please use RoleType.textField and RoleType.textBox

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:768:
* @param {Object} node
s/Object/AutomationNode

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:769:
*/
@private

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:269:
}), 100);
Can you do this without a timeout (e.g. by adding a focus event handler)?

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_tts.js:33: * so that we
can assert that no extra unrelated speech is generated.
I think this is called a strict mock.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_tts.js:34: */
nit: @private

[Peter Lundblad, 2015-08-20 08:30:06]

Hi,

A few suggestions below.  FYI, I am working on a combine mock speech
and braille feedback class that will superseed MockTts.  I'll incorporate
more strictness there according to what we decide in this thread.

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:69:
this.eventQueue_ = [];
I suggest putting the event queue into its own class outside of background
that can be unit tested separately.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:76:
this.eventQueueTimerId_ = null;
Since your code already relies on timer ids being non-zero, perhaps slightly
simplify and use 0 for no id?

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:83:
this.EVENT_QUEUE_DELAY_MS = 10;
Put constants outside of the constructor, directly on the object.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:110:
* @type {?cvox.ChromeVoxEditableTextBase}
Document when this is null.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:435:
evt.enqueueTime = new Date();
Should we extend the automation api with a time field?  The closer to the
source, the closer to reality the time will be.  In any case, I suggest wrapping
the event and time in a new object instead of extending the AutomationEvent,
allowing the type of the queue elements to be correct.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:448:
executeNextEventFromQueue_: function() {
nit: s/execute/handle/ ?

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:457:
var lastTimeForEventType = {};
Why not just store the object reference instead of the the time in this
object. I think it has the same result, but is easier to understand.

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:26:
global.backgroundObj.forceChromeVoxNextActive();
ChromeVoxE2ETest.runWithTab already adds chromevox_next_test to the url which
should cause next to be enabled. If that logic is not working, we should
probably clean it
up whn adding this line, making sure that it was the cause of the flakiness.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:269:
}), 100);
I bet this will flake on some memmory bot. We shouldn't use timeouts for this,
but instead arrange for this to be done in a callback after the previous
expectation
is met.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:271:
}.bind(this));
Superflous bind.

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_tts.js:35:
skipNonMatchingUtterances_: true,
I support this in principal, but it has the potential to be flaky.  In end
to end tests like this there are some things we don't have control over.  When
running these tests locally, I always get a system alert about 'connected
to eth1' and I don't think we can be sure those happen first.  Perhaps we need
to whitelist a few unrelated utterances to be able to enable more strictness.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/testing/mock_tts.js:135: // anymore.
nit: any more, anymore

[David Tseng, 2015-08-20 17:52:47]

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

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:26:
global.backgroundObj.forceChromeVoxNextActive();
On 2015/08/20 08:30:06, Peter Lundblad wrote:
> ChromeVoxE2ETest.runWithTab already adds chromevox_next_test to the url which
> should cause next to be enabled. If that logic is not working, we should
> probably clean it
> up whn adding this line, making sure that it was the cause of the flakiness.

That logic depends on a load complete event to be observed, so there's a
possibility that classic gets injected first. With force next (different from
just the #chromevox_next_test), classic doesn't get started at all.

https://codereview.chromium.org/1295213003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:269:
}), 100);
On 2015/08/20 08:30:06, Peter Lundblad wrote:
> I bet this will flake on some memmory bot. We shouldn't use timeouts for this,
> but instead arrange for this to be done in a callback after the previous
> expectation
> is met.

See my suggestion above (about focus event handler).

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

Closing in favor of: https://codereview.chromium.org/1318683002/