Merge text from all changes to the same live region into one utterance.

Merge text from all changes to the same live region into one utterance.

The problem was that if there were two objects that both changed in the
same live region, like 'apple' and 'orange', they should really be
spoken in sequence, but there was no way to enforce that - another
utterance can be inserted in-between them. Until we have a way to enforce it,
combining the text is a better user experience.

To match other screen readers, read text and control values, but not
roles and states.

BUG=408821
Committed: https://crrev.com/07528460dfe5e3420e6b61871340658b0ee7223c
Cr-Commit-Position: refs/heads/master@{#296070}

[dmazzoni, 2014-08-28 23:00:02]

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

[David Tseng, 2014-08-29 17:06:27]

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

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:414:
// to group the nav descriptions together, collapse them into one.
TODO, referencing a bug please.

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:418:
return prev + ' ' + navDescription.text + ' ' + navDescription.userValue;
This messes up the prosity of the NavDescriptions (e.g. pausing between the
utterances).
So, perhaps pseudocode
arrayOfStrings = map(descriptions)
text = arrayOfStrings.join(', ')

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:420:
combinedText = cvox.DomUtil.collapseWhitespace(combinedText);
Not really needed?

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:424:

Perhaps needs a test.

[dmazzoni, 2014-09-18 20:58:21]

PTAL. I ported the live region tests and added new tests for this change.

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

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:414:
// to group the nav descriptions together, collapse them into one.
On 2014/08/29 17:06:26, David Tseng wrote:
> TODO, referencing a bug please.

Done.

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:418:
return prev + ' ' + navDescription.text + ' ' + navDescription.userValue;
On 2014/08/29 17:06:26, David Tseng wrote:
> This messes up the prosity of the NavDescriptions (e.g. pausing between the
> utterances).
> So, perhaps pseudocode
> arrayOfStrings = map(descriptions)
> text = arrayOfStrings.join(', ')

Good catch. Added a comma in-between nav descriptions.

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:420:
combinedText = cvox.DomUtil.collapseWhitespace(combinedText);
On 2014/08/29 17:06:26, David Tseng wrote:
> Not really needed?

Gone.

https://codereview.chromium.org/522433002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:424:

On 2014/08/29 17:06:26, David Tseng wrote:
> Perhaps needs a test.

Done.

[David Tseng, 2014-09-19 19:32:08]

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs
(right):

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs:276:
}, 20);
All of these are potentially flakey right? I guess if it doesn't fail on the
bots, then this should be ok, but it seems like we should be triggering live
region/focus pretty much in parallel to ensure the bug has actually been fixed.

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs:302:
}, 400);
Why the variation in timeout?

[dmazzoni, 2014-09-22 07:33:04]

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs
(right):

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs:276:
}, 20);
On 2014/09/19 19:32:08, David Tseng wrote:
> All of these are potentially flakey right? I guess if it doesn't fail on the
> bots, then this should be ok, but it seems like we should be triggering live
> region/focus pretty much in parallel to ensure the bug has actually been
fixed.

Spend some more time on these and all setTimeouts are gone, just waitForCalm
now.

[David Tseng, 2014-09-22 15:54:02]

lgtm

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs
(right):

https://codereview.chromium.org/522433002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs:276:
}, 20);
On 2014/09/19 19:32:08, David Tseng wrote:
> All of these are potentially flakey right? I guess if it doesn't fail on the
> bots, then this should be ok, but it seems like we should be triggering live
> region/focus pretty much in parallel to ensure the bug has actually been
fixed.

Well, I was thinking we could just have the expectation be looser (i.e. check
both events were spoken, but not necessarily the order).

[dmazzoni, 2014-09-22 21:18:52]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-22 21:20:15]

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

[commit-bot: I haz the power, 2014-09-22 22:21:40]

Committed patchset #4 (id:60001) as fc39dd3bf59f920c8a80f6ffc54bb6214c960e9a

[commit-bot: I haz the power, 2014-09-22 22:22:35]

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