Improve braille related message descriptions and clean up message handling in Chromevox.

Improve braille related message descriptions and clean up message handling in Chromevox.

- Add better descritions for braille related messages.

- Move non-translatable messages out of chromevox_strings.grd.

- Remove duplicates in chromevox_strings.grd.

- Move cvox.Msgs out into the global namespace, and simplify by having
  functions directly on the object instead of having an instance at
  cvox.ChromeVox.msgs.

- Move spoken_messages.js closer to where it si used in chromevox
  classic, removing a small amoutn of dead code.

BUG=None

Committed: https://crrev.com/6a1a9e2ec2e503f1ed5fc135dde1678f241e0f75
Cr-Commit-Position: refs/heads/master@{#351064}

[Peter Lundblad, 2015-09-25 15:10:04]

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

[Peter Lundblad, 2015-09-25 15:10:05]

Hi,

The braille related messages needed clarifications to be translated properly and
some of them don't actually need translation at all. This addresses that and
does some other cleanups.  Sorry for all the repetition, but translations happen
without more context than the descriptions.

[dmazzoni, 2015-09-25 16:23:35]

lgtm

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/chromevox/injected/loader.js
(right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/loader.js:15:
goog.require('Msgs');
nit: sort these

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/chromevox/injected/ui/spoken_messages.js
(right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/injected/ui/spoken_messages.js:55:
message.id);
nit: indentation

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/common/aria_util.js (right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/aria_util.js:43: 'link' :
'tag_link',
Slightly confusing to me because the tag name for a hyperlink isn't "link", and
there is a <link> tag that means something else

What about getting rid of the "aria_" from most of the messages for role names?
role_link is totally clear, and there are a few other cases where we have a role
that can come from an ARIA role or from native HTML.

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/common/dom_util.js (right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/dom_util.js:50: 'radio' :
'aria_role_radio',
This is another example. I think it'd be better if it was just role_radio.

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/common/msgs.js (right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/msgs.js:63: * Processes an
HTML DOM the text of "i18n" elements with translated messages.
Could you rewrite this doc? I know you didn't change it but it isn't very
clearly written.

Make it clear that i18n is a class name.

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/msgs.js:64: * This function
expects HTML elements with a i18n clean and a msgid attribute.
I think the word "clean" should be "class" here

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/common/msgs.js:86: Msgs.getNumber =
function(num) {
Get we delete this while we're at it?

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

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:369:
'tel': 'input_type_number',
Is this intentional? I think we should distinguish a telephone number field from
a number field

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:962:
<message desc="This is an abbreviated ARIA widget role name shown on a braille
display. When translating, try to find a contracted form of the translated word
'alert' according to local conventions. If reasonable, use all lowercase and
avoid punctuation to keep the number of characters as low as possible."
name="IDS_CHROMEVOX_ARIA_ROLE_ALERT_BRL">
Nice descriptions, very clear.

[Peter Lundblad, 2015-09-28 13:21:50]

dmazzoni@chromium.org writes:
> lgtm
> 
> 
> 
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File
> chrome/browser/resources/chromeos/chromevox/chromevox/injected/loader.js
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/chromevox/injected/loader.js:15:
> goog.require('Msgs');
> nit: sort these

Ha, I guess the presubmit was confused by the initial lines in this file
not being goog.provide/require. Done.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File
>
chrome/browser/resources/chromeos/chromevox/chromevox/injected/ui/spoken_messages.js
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
>
chrome/browser/resources/chromeos/chromevox/chromevox/injected/ui/spoken_messages.js:55:
> message.id);
> nit: indentation

Done.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/chromevox/common/aria_util.js
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/common/aria_util.js:43:
> 'link' : 'tag_link',
> Slightly confusing to me because the tag name for a hyperlink isn't
> "link", and there is a <link> tag that means something else

Changed this to role_link.  All tag_ message names are confusing. There are
some
we can clean up further, especially when cvox classic is gone.

> What about getting rid of the "aria_" from most of the messages for role
> names? role_link is totally clear, and there are a few other cases where
> we have a role that can come from an ARIA role or from native HTML.

Yes, changed these since in the automation tree we just have roles.
Note that changing the name in the grd does not affect the message id used
for translation, so this shouldn't lead to a lot of extra work for
the localizers.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/chromevox/common/dom_util.js
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/common/dom_util.js:50:
> 'radio' : 'aria_role_radio',
> This is another example. I think it'd be better if it was just
> role_radio.

Done.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/chromevox/common/msgs.js (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/common/msgs.js:63: *
> Processes an HTML DOM the text of "i18n" elements with translated
> messages.
> Could you rewrite this doc? I know you didn't change it but it isn't
> very clearly written.
> 
> Make it clear that i18n is a class name.

Yeah, that was an impressive one;)  Rewritten hopefully better now.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/common/msgs.js:64: * This
> function expects HTML elements with a i18n clean and a msgid attribute.
> I think the word "clean" should be "class" here

I think you're right.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/common/msgs.js:86:
> Msgs.getNumber = function(num) {
> Get we delete this while we're at it?

I think we should fix this to actually do localized formatting, alternatively
fix all call sites to do the right thing (toLocaleString?).  leaving for
a follow-up cl just to put a limit somewhere ;)

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:369:
> 'tel': 'input_type_number',
> Is this intentional? I think we should distinguish a telephone number
> field from a number field

So from 2013, the distinction has been 'Edit text numeric only' vs.
'Edit text, number entry'.  That distinction is very unclear.  I have a vague
memory of this change being made because type='tel' was misused in some places
(for example for the 2 factor entry field) which was very confusing.
David, who made the change for type='tel' might remember this better than
me. In any case, removing this emssage and if we want to start distinguishing
these in a meaningful way, we can figure out a better emssage and readd
in a followup cl.

>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
> File
> chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
> (right):
> 
>
https://codereview.chromium.org/1362223003/diff/1/chrome/browser/resources/ch...
>
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:962:
> <message desc="This is an abbreviated ARIA widget role name shown on a
> braille display. When translating, try to find a contracted form of the
> translated word 'alert' according to local conventions. If reasonable,
> use all lowercase and avoid punctuation to keep the number of characters
> as low as possible." name="IDS_CHROMEVOX_ARIA_ROLE_ALERT_BRL">
> Nice descriptions, very clear.

That'll we'll see when the next round of questions come in from
l10n people... Ack.

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

-- 

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

[Peter Lundblad, 2015-09-28 13:34:07]

The CQ bit was checked by plundblad@chromium.org

[Peter Lundblad, 2015-09-28 13:34:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1362223003/#ps40001
(title: "Move another braille message to Msgs.Untranslated")

[commit-bot: I haz the power, 2015-09-28 13:34:47]

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

[commit-bot: I haz the power, 2015-09-28 14:03:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 14:04:38]

Patchset 3 (id:??) landed as
https://crrev.com/6a1a9e2ec2e503f1ed5fc135dde1678f241e0f75
Cr-Commit-Position: refs/heads/master@{#351064}