Entering / exiting dialog notification should not interrupt live region.

Entering / exiting dialog notification should not interrupt live region.

The key to the fix is adding a category of 'nav' to the entering/exiting
dialog notifications.

BUG=408821
R=dtseng@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/97c41bc2d0e367a9a3a28c221bf9bd052128061e

[dmazzoni, 2014-09-23 19:50:07]

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

[David Tseng, 2014-09-24 22:38:42]

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js:1329:
cvox.ChromeVox.msgs.getMsg('exiting_dialog'), 'nav');
Is the category defined as an enum somewhere?

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs:781:
assertEquals('Button', ulist[3].text);
It's not really obvious what's actually being interrupted or not here?

[dmazzoni, 2014-09-25 19:42:11]

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js:1329:
cvox.ChromeVox.msgs.getMsg('exiting_dialog'), 'nav');
On 2014/09/24 22:38:41, David Tseng wrote:
> Is the category defined as an enum somewhere?

It's intended to be ad-hoc rather than having a small number of predefined
categories. The important thing is that all speech from the same subsystem
(events, live regions, typing) should use the same category, if any.

I'll move this to a constant/enum, but if it's okay with you, not defined in
abstract_tts but defined here in chromevox/injected.

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs:781:
assertEquals('Button', ulist[3].text);
On 2014/09/24 22:38:41, David Tseng wrote:
> It's not really obvious what's actually being interrupted or not here?

Nothing will interrupt. Previously the speech arguments to 'Exited dialog.'
flushed everything, rather than flushing only navigations.

[David Tseng, 2014-09-25 21:03:00]

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js:1329:
cvox.ChromeVox.msgs.getMsg('exiting_dialog'), 'nav');
On 2014/09/25 19:42:10, dmazzoni wrote:
> On 2014/09/24 22:38:41, David Tseng wrote:
> > Is the category defined as an enum somewhere?
> 
> It's intended to be ad-hoc rather than having a small number of predefined
> categories. The important thing is that all speech from the same subsystem
> (events, live regions, typing) should use the same category, if any.
> 
> I'll move this to a constant/enum, but if it's okay with you, not defined in
> abstract_tts but defined here in chromevox/injected.

It's going to be nice to have a place to reference what categories are available
and whenever we want a new category, we should think about its necessity given
the existing ones. Also, if you define it using @enum, you can use it for
compile-time checks and @param {ENUMNAME} it.

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs
(right):

https://codereview.chromium.org/596033002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher_test.unitjs:781:
assertEquals('Button', ulist[3].text);
On 2014/09/25 19:42:10, dmazzoni wrote:
> On 2014/09/24 22:38:41, David Tseng wrote:
> > It's not really obvious what's actually being interrupted or not here?
> 
> Nothing will interrupt. Previously the speech arguments to 'Exited dialog.'
> flushed everything, rather than flushing only navigations.

Hmm...ok, I'm just not really seeing how this test verifies nothing interrupts.
Above, there's a category flush followed by another category flush. I understand
that the nav category makes the second flush queue, but that's what we're trying
to test explicitly right? (e.g. verifying the parameers to chrome.tts.speak are
as we expect for each utterance).

[dmazzoni, 2014-09-26 22:12:55]

On 2014/09/25 21:03:00, David Tseng wrote:
> It's going to be nice to have a place to reference what categories are
available
> and whenever we want a new category, we should think about its necessity given
> the existing ones. Also, if you define it using @enum, you can use it for
> compile-time checks and @param {ENUMNAME} it.

Done, it's now an enum. I also uploaded a separate change to make queueMode an
enum.

> Hmm...ok, I'm just not really seeing how this test verifies nothing
interrupts.

Improved, take a look.

[David Tseng, 2014-09-26 22:42:20]

lgtm

Just a few suggestions.

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/host/testing/tts.js (right):

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:106: var
oldQueue = queue;
You could just
(pseudocode)
if utterance has category
queue = queue.filter(function(u) {
  return !u.properties || u.properties.category != utterance.category;
});

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:119:
queue.push(utterance);
Did you mean to push here?

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:122: var result
= [];
queue.map() seems like a good pattern here.

[dmazzoni, 2014-09-26 22:51:49]

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/host/testing/tts.js (right):

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:106: var
oldQueue = queue;
On 2014/09/26 22:42:20, David Tseng wrote:
> You could just
> (pseudocode)
> if utterance has category
> queue = queue.filter(function(u) {
>   return !u.properties || u.properties.category != utterance.category;
> });

Done.

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:119:
queue.push(utterance);
On 2014/09/26 22:42:20, David Tseng wrote:
> Did you mean to push here?

Yes

https://codereview.chromium.org/596033002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/testing/tts.js:122: var result
= [];
On 2014/09/26 22:42:20, David Tseng wrote:
> queue.map() seems like a good pattern here.

Done.

[commit-bot: I haz the power, 2014-09-26 22:56:08]

Patchset 4 (id:??) landed as
https://crrev.com/97c41bc2d0e367a9a3a28c221bf9bd052128061e
Cr-Commit-Position: refs/heads/master@{#297062}

[dmazzoni, 2014-09-26 22:56:08]

Committed patchset #4 (id:60001) manually as 97c41bc (presubmit successful).