1030470 - [AccessFu] Accessfu.properties localization needs to work in Gaia.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Accessfu message bundles are not localized in Gaia. We need a mechanism of having the l10n working on the FxOS device.

Note: consider accessibility service in gaia to potentially address this.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470 patch v1

First pass.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470 patch v2

Changed the name from accessfu-speech to accessfu-actions. Removed enqueue: true for speech presenter's pivot change.

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8461776 [details] [diff] [review]
1030470 patch v2

Review of attachment 8461776 [details] [diff] [review]:
-----------------------------------------------------------------

yzen, i have 3 thoughts about this patch
 1. it is hard to read with the jslint :P could you make that a preliminary patch?
 2. i think localization should get out of OutputGenerator entirely, it is confusing to have it both there and in presenter
 3. yeah, sorry i said otherwise before, but i think we need to think of a "protocol" format of the data we throw at gaia
 and gaia should do speech/earcon/haptic and do a native gaia toaster for announcements
<yzen> ya i can do that 
 icaaq is now known as icaaq|afk
<eeejay> i think the fields would be:
 1. output-type: that would be things like vc-change, state-change, text-change, announcement, etc
 and then gaia would determine what earcon it gets, if it gets haptic, etc.
 the special case would be announcement, it would trigger a toaster, which in turn would send another liveregion change event
<yzen> eeejay right so that's the idea
<yzen> im  a little confused what you mean about l10n in OutputGenerator, afaik, there's nothing there any more
<eeejay> hm
<yzen> eeejay ok let me split the patch from linting , it will be easier to read
<eeejay> yzen, also. i have second thoughts about the data format..
 yzen, maybe it is worth having the gaia end as dumb as possible so that we have more control in our own module
 yzen, don't want to ask for review from gaia devs when we want to change the haptic feedback
 yzen, the l10n in the generator is the _addLocalized{State,Role}
 yzen, at the end of the day (this could be done in one patch), we should have 3 presenters: Visual, Android, B2G
 And the Visual could one day go away
<yzen> eeejay right that's an old name, ill update
 eeejay ok ill update the pr with more changes
<eeejay> pr, haha

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.1 patch 1

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.2 patch 1

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 8462744 [details] [diff] [review]
1030470.1 patch 1

Review of attachment 8462744 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. I am not a fan of the details field being an array. It then makes retrieving details assume that there is a 0 index, like you do in the Visual presenter. Also, that was a flaw in the Speech presenter that I would like to see go away. Do we ever have more than one?

::: accessible/jsat/Presentation.jsm
@@ +440,3 @@
>    __proto__: Presenter.prototype,
> +  type: 'B2G',
> +  PIVOT_CHANGE_PATTERN: [40],

this is haptic feedback? gotta be a more descriptive name for that.

@@ +450,5 @@
> +      details: [{
> +        eventType: 'vc-change',
> +        data: UtteranceGenerator.genForContext(aContext),
> +        options: {
> +          enqueue: false,

maybe enqueue should implicitly always be false unless it is present/true?

@@ +452,5 @@
> +        data: UtteranceGenerator.genForContext(aContext),
> +        options: {
> +          enqueue: false,
> +          pattern: this.PIVOT_CHANGE_PATTERN,
> +          isKey: aContext.accessible.role === Roles.KEY

how about adding vc change reason as well. We may want to present differently if it is ebt, swipe, or auto-moved.

::: accessible/jsat/Utils.jsm
@@ +188,5 @@
>      this.isContentProcess =
>        Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
>      return this.isContentProcess;
>    },
> +  sendChromeEvent: function sendChromeEvent(aDetails) {

1. The name is misleading, you are doing more here than simply sending an event with given details.
2. Isn't this only used in AccessFu.jsm? If so, I think it is more appropriate for this to be Output.B2G.

@@ +195,5 @@
> +      details: JSON.stringify(aDetails)
> +    };
> +    if (this.win.shell) {
> +      this.win.shell.sendChromeEvent(details);
> +    } else {

Assuming this is for supporting desktop browser, should be a comment here with that.

@@ +206,5 @@
> +  },
> +
> +  localize: function localize(aOutput) {
> +    if (!Array.isArray(aOutput)) {
> +      aOutput = [aOutput];

don't re-assign arguments.

@@ +219,2 @@
>    get stringBundle() {
> +    let bundle = Services.strings.createBundle(

You stopped caching the bundle. Is that intentional?

@@ +241,5 @@
> +          if (count) {
> +            str = PluralForm.get(count, str);
> +            str = str.replace('#1', count);
> +          }
> +          return str;

What if there is both a 'count' and an 'args' field? Looks like the count will clobber the args string. Should they co-exist. Also, I'll sleep better if str is initialized as an empty string.

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +184,5 @@
>  quicknav_Separator   = Separators
>  quicknav_Table       = Tables
>  quicknav_Checkbox    = Check boxes
>  # Shortened role names for braille
> +menubarAbbr        =       menu bar

Do we really need to copy all the roles here even when they are not abbreviated? Can't we programmatically fall back on the non-abbreviated role name when an abbreviated one does not exist?

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 8462745 [details] [diff] [review]
1030470.2 patch 1

Review of attachment 8462745 [details] [diff] [review]:
-----------------------------------------------------------------

Auto-approved jslinted files.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> Comment on attachment 8462744 [details] [diff] [review]
> 1030470.1 patch 1
> 
> Review of attachment 8462744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good. I am not a fan of the details field being an array. It
> then makes retrieving details assume that there is a 0 index, like you do in
> the Visual presenter. Also, that was a flaw in the Speech presenter that I
> would like to see go away. Do we ever have more than one?

As discussed in IRC, we'll normalize with Android and will get rid of Visual presenter as soon as possible.

> 
> ::: accessible/jsat/Presentation.jsm
> @@ +440,3 @@
> >    __proto__: Presenter.prototype,
> > +  type: 'B2G',
> > +  PIVOT_CHANGE_PATTERN: [40],
> 
> this is haptic feedback? gotta be a more descriptive name for that.

Added comment.

> 
> @@ +450,5 @@
> > +      details: [{
> > +        eventType: 'vc-change',
> > +        data: UtteranceGenerator.genForContext(aContext),
> > +        options: {
> > +          enqueue: false,
> 
> maybe enqueue should implicitly always be false unless it is present/true?

Done.

> 
> @@ +452,5 @@
> > +        data: UtteranceGenerator.genForContext(aContext),
> > +        options: {
> > +          enqueue: false,
> > +          pattern: this.PIVOT_CHANGE_PATTERN,
> > +          isKey: aContext.accessible.role === Roles.KEY
> 
> how about adding vc change reason as well. We may want to present
> differently if it is ebt, swipe, or auto-moved.

Done.

> 
> ::: accessible/jsat/Utils.jsm
> @@ +188,5 @@
> >      this.isContentProcess =
> >        Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
> >      return this.isContentProcess;
> >    },
> > +  sendChromeEvent: function sendChromeEvent(aDetails) {
> 
> 1. The name is misleading, you are doing more here than simply sending an
> event with given details.
> 2. Isn't this only used in AccessFu.jsm? If so, I think it is more
> appropriate for this to be Output.B2G.

Done.

> 
> @@ +195,5 @@
> > +      details: JSON.stringify(aDetails)
> > +    };
> > +    if (this.win.shell) {
> > +      this.win.shell.sendChromeEvent(details);
> > +    } else {
> 
> Assuming this is for supporting desktop browser, should be a comment here
> with that.

Done.

> 
> @@ +206,5 @@
> > +  },
> > +
> > +  localize: function localize(aOutput) {
> > +    if (!Array.isArray(aOutput)) {
> > +      aOutput = [aOutput];
> 
> don't re-assign arguments.

Fixed.

> 
> @@ +219,2 @@
> >    get stringBundle() {
> > +    let bundle = Services.strings.createBundle(
> 
> You stopped caching the bundle. Is that intentional?

From what I know , unless you delete the property in the getter it is only evaluated once?

> 
> @@ +241,5 @@
> > +          if (count) {
> > +            str = PluralForm.get(count, str);
> > +            str = str.replace('#1', count);
> > +          }
> > +          return str;
> 
> What if there is both a 'count' and an 'args' field? Looks like the count
> will clobber the args string. Should they co-exist. 

Not entirely sure.. Args are used to expand %S placeholders, and Plural form expands #1. It also looks like there's never a case where args and count are used together..

> Also, I'll sleep better
> if str is initialized as an empty string.

Fixed.

> 
> ::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
> @@ +184,5 @@
> >  quicknav_Separator   = Separators
> >  quicknav_Table       = Tables
> >  quicknav_Checkbox    = Check boxes
> >  # Shortened role names for braille
> > +menubarAbbr        =       menu bar
> 
> Do we really need to copy all the roles here even when they are not
> abbreviated? Can't we programmatically fall back on the non-abbreviated role
> name when an abbreviated one does not exist?

We'd have to localize it as part of the logic and have the same code both in gecko and gaia. I think it's OK to have Abbr version for all roles but if you definitely prefer the other approach I can change that?

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.1 patch 2

Comments addressed

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.2 patch 2

Carrying over r+ from Eitan.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.1 patch v3

Comments addressed.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.2 patch v3

Carrying over the r+

Comment 13

[Eitan Isaacson [:eeejay]]

Comment on attachment 8463372 [details] [diff] [review]
1030470.1 patch v3

Review of attachment 8463372 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. I have no further feedback! We should wait for the gaia bits to land and settle before you land this.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.1 patch v4

Carrying over r+ from eeejay.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1030470.2 patch v4

Carrying over r+ from eeejay.

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

https://tbpl.mozilla.org/?tree=Try&rev=e7511c2f621c

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6059e806022
https://hg.mozilla.org/integration/mozilla-inbound/rev/860146d2669f

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

Should keep it open until it lands in m-c

Comment 19

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/d6059e806022
https://hg.mozilla.org/mozilla-central/rev/860146d2669f