Add a role info dictionary to output.js to resolve role msgs.

Add a role info dictionary to output.js to resolve role msgs.

Currently, ChromeVox Next uses the programmatic string when whenever $role gets used. This cl makes it so $role resolves to a msg id, if one exists. Additionally, the role info object also lists the arcon associated with that role.

This allows us to remove a few of the output rules (button, textField).

Committed: https://crrev.com/cdd27239efbd158480fc8bb2f21cf76297aea51f
Cr-Commit-Position: refs/heads/master@{#322423}

[David Tseng, 2015-03-16 22:58:01]

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

[Peter Lundblad, 2015-03-17 10:34:09]

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

https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:103:
msgId: 'LIST_WITH_ITEMS'
nit: lower case.

https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:103:
msgId: 'LIST_WITH_ITEMS'
This message takes one parameter, how is that handled?

https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:521: var
msgId = node.role;
If we want this, then the msgIds should be qualified, e.g.
  msgId = 'role_' + node.role;
to not polute the namespace with role names.

https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:534:
this.addToSpannable_(
Should the msgId be suffixed by _brl if such a message exists to get the
correct role for braille?

[David Tseng, 2015-03-17 16:49:37]

plundblad@chromium.org writes:

>
https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (right):
>
>
https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:103:
> msgId: 'LIST_WITH_ITEMS'
> nit: lower case.
>
Done.

>
https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:103:
> msgId: 'LIST_WITH_ITEMS'
> This message takes one parameter, how is that handled?
It's not handled; removed this block. Added something to the list ->
enter rule.

>
https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:521:
> var msgId = node.role;
> If we want this, then the msgIds should be qualified, e.g.
>    msgId = 'role_' + node.role;
> to not polute the namespace with role names.
>
Revised to use unlocalized programmatic role and print an error.

>
https://codereview.chromium.org/1011913002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:534:
> this.addToSpannable_(
> Should the msgId be suffixed by _brl if such a message exists to get the
> correct role for braille?
>
Done.

> https://codereview.chromium.org/1011913002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.


-- 
Foo X. Bar
http://www.example.com

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

[Peter Lundblad, 2015-03-18 15:56:36]

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

https://codereview.chromium.org/1011913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:81: *
@const {Object<string, {msgId: string, earcon: string}>}
This should cause compliation errors since earcon is treated as optional below.
Do you know why not?

https://codereview.chromium.org/1011913002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:525: if
(this.formatOptions_.braille)
Indent.

[David Tseng, 2015-03-18 16:22:49]

plundblad@chromium.org writes:

>
https://codereview.chromium.org/1011913002/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (right):
>
>
https://codereview.chromium.org/1011913002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:81:
> * @const {Object<string, {msgId: string, earcon: string}>}
> This should cause compliation errors since earcon is treated as optional
> below. Do you know why not?
>
Not sure; is there even a syntax to say earcon is optional here? Seems
like Closure treats keys annotated this way as optional.


>
https://codereview.chromium.org/1011913002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:525:
> if (this.formatOptions_.braille)
> Indent.
>
Done.

> https://codereview.chromium.org/1011913002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.


-- 
Foo X. Bar
http://www.example.com

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

[Peter Lundblad, 2015-03-20 11:26:34]

Let me know when you've uploaded the latest patchset reflecting your latest
comments.

[David Tseng, 2015-03-20 16:04:46]

Did you have a response to the optional object key annotation? The
indented-response comment patchset is now uploaded.

On Fri, Mar 20, 2015 at 4:26 AM, <plundblad@chromium.org> wrote:

> Let me know when you've uploaded the latest patchset reflecting your latest
> comments.
>
>
> https://codereview.chromium.org/1011913002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[Peter Lundblad, 2015-03-25 08:32:56]

lgtm

(Sorry, had reviewed this yesterday but the code review site 500'd)

For the type annotation in the role map, I played with it and it seems like
the compiler's type checking is incomplete here (or I am missing something).

https://codereview.chromium.org/1011913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1011913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:83:
Output.ROLE_INFO = {
Why is this not private?

[David Tseng, 2015-03-25 22:02:35]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-03-25 22:02:36]

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/1011913002/#ps120001
(title: "@private")

[commit-bot: I haz the power, 2015-03-25 22:02:59]

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

[commit-bot: I haz the power, 2015-03-25 22:42:49]

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

[commit-bot: I haz the power, 2015-03-25 22:42:51]

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_...)

[David Tseng, 2015-03-26 17:35:35]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-03-26 17:35:36]

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/1011913002/#ps140001
(title: "fix test")

[commit-bot: I haz the power, 2015-03-26 17:36:21]

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

[commit-bot: I haz the power, 2015-03-26 18:10:52]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-26 18:11:27]

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

[Peter Lundblad, 2015-03-27 15:33:25]

Something in this CL made the role be displayed before the value in an edit
field. Was that intentional?

[David Tseng, 2015-03-27 19:25:03]

Not intended. This is because we removed the text field output rule so the
default rule applies and puts the role before value. I can change the
default rule if that makes sense.

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